style: Drop unused user-agent cascade datas when not holding the cache lock.

We want to drop the cascade data memory as soon as possible, so bug 1544546
introduced an UpdateStylistIfNeeded call from ShellDetachedFromDocument.

Unfortunately, this call can reenter into the global user-agent cascade data if
some of the CSS values kept alive by the cascade data keep alive an SVG
document, see the stack on this bug for an example. Make sure to drop the
user-agent cascade datas when not holding the cache lock to avoid this
situation.

Before bug 1535788, we just destroyed the stylist, so we kept holding a
reference from the cache, and that reference will be dropped sometime later when
other document updated their user-agent stylesheets (if they happened not to
match the cache of course).

Seems to me this doesn't ended up happening in our automation, but it could
happen in the wild, in theory at least.

It's nice that Rust made this a safe deadlock caught by our tests rather than a
very subtle and infrequent memory corruption.

The relevant SVG documents are probably the <input type=number> rules:

https://searchfox.org/mozilla-central/rev/d80f0a570736dce76a2eb184fb65517462089e8a/layout/style/res/forms.css#1050

Differential Revision: https://phabricator.services.mozilla.com/D28299

1 files changed, 34 insertions, 15 deletions

diff --git a/components/style/stylist.rs b/components/style/stylist.rs
index ad7f9fd8585..a22d850468e 100644
--- a/components/style/stylist.rs
+++ b/components/style/stylist.rs
@@ -48,7 +48,8 @@ use selectors::visitor::SelectorVisitor;
 use selectors::NthIndexCache;
 use servo_arc::{Arc, ArcBorrow};
 use smallbitvec::SmallBitVec;
-use std::ops;
+use smallvec::SmallVec;
+use std::{mem, ops};
 use std::sync::Mutex;
 use style_traits::viewport::ViewportConstraints;
 
@@ -128,15 +129,28 @@ impl UserAgentCascadeDataCache {
         Ok(new_data)
     }
 
-    fn expire_unused(&mut self) {
-        // is_unique() returns false for static references, but we never have
-        // static references to UserAgentCascadeDatas.  If we did, it may not
-        // make sense to put them in the cache in the first place.
-        self.entries.retain(|e| !e.is_unique())
+    /// Returns all the cascade datas that are not being used (that is, that are
+    /// held alive just by this cache).
+    ///
+    /// We return them instead of dropping in place because some of them may
+    /// keep alive some other documents (like the SVG documents kept alive by
+    /// URL references), and thus we don't want to drop them while locking the
+    /// cache to not deadlock.
+    fn take_unused(&mut self) -> SmallVec<[Arc<UserAgentCascadeData>; 3]> {
+        let mut unused = SmallVec::new();
+        for i in (0..self.entries.len()).rev() {
+            // is_unique() returns false for static references, but we never
+            // have static references to UserAgentCascadeDatas.  If we did, it
+            // may not make sense to put them in the cache in the first place.
+            if self.entries[i].is_unique() {
+                unused.push(self.entries.remove(i));
+            }
+        }
+        unused
     }
 
-    fn clear(&mut self) {
-        self.entries.clear();
+    fn take_all(&mut self) -> Vec<Arc<UserAgentCascadeData>> {
+        mem::replace(&mut self.entries, Vec::new())
     }
 
     #[cfg(feature = "gecko")]
@@ -254,13 +268,18 @@ impl DocumentCascadeData {
         // First do UA sheets.
         {
             if flusher.flush_origin(Origin::UserAgent).dirty() {
-                let mut ua_cache = UA_CASCADE_DATA_CACHE.lock().unwrap();
                 let origin_sheets = flusher.origin_sheets(Origin::UserAgent);
-                let ua_cascade_data =
-                    ua_cache.lookup(origin_sheets, device, quirks_mode, guards.ua_or_user)?;
-                ua_cache.expire_unused();
-                debug!("User agent data cache size {:?}", ua_cache.len());
-                self.user_agent = ua_cascade_data;
+                let _unused_cascade_datas = {
+                    let mut ua_cache = UA_CASCADE_DATA_CACHE.lock().unwrap();
+                    self.user_agent = ua_cache.lookup(
+                        origin_sheets,
+                        device,
+                        quirks_mode,
+                        guards.ua_or_user,
+                    )?;
+                    debug!("User agent data cache size {:?}", ua_cache.len());
+                    ua_cache.take_unused()
+                };
             }
         }
 
@@ -1368,7 +1387,7 @@ impl Stylist {
 
     /// Shutdown the static data that this module stores.
     pub fn shutdown() {
-        UA_CASCADE_DATA_CACHE.lock().unwrap().clear()
+        let _entries = UA_CASCADE_DATA_CACHE.lock().unwrap().take_all();
     }
 }