Bug 1545979 - Drop unused user-agent cascade datas when not holding the cache lock. r=bholley
authorEmilio Cobos Álvarez <emilio@crisal.io>
Mon, 22 Apr 2019 19:45:06 +0000
changeset 470407 24161afba7afa66dd487203298e568610bfe1191
parent 470406 b48ddc1c59ba06cb99c1eed21ab4edf1ffd5686f
child 470408 9c712980e9709732e4966d404f50267566d3b149
push id35905
push userdvarga@mozilla.com
push dateTue, 23 Apr 2019 09:53:27 +0000
treeherdermozilla-central@831918f009f6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1545979, 1544546, 1535788
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1545979 - Drop unused user-agent cascade datas when not holding the cache lock. r=bholley 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
layout/style/ServoStyleSet.cpp
servo/components/style/stylist.rs
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -126,23 +126,17 @@ void ServoStyleSet::ShellDetachedFromDoc
   // Remove all our stylesheets...
   for (const Origin origin : kOrigins) {
     for (size_t count = SheetCount(origin); count--;) {
       RemoveStyleSheet(origin, SheetAt(origin, count));
     }
   }
 
   // And remove all the CascadeDatas from memory.
-  //
-  // FIXME(emilio, bug 1545979): This line should be uncommented, but causes
-  // some issues if dropping our cascade data of the user-agent origin ends up
-  // destroying another pres shell, since it that case it could reenter there
-  // and deadlock. Proper fix will be written in that bug.
-  //
-  // UpdateStylistIfNeeded();
+  UpdateStylistIfNeeded();
 
   // Also GC the ruletree if it got big now that the DOM no longer has
   // references to styles around anymore.
   MaybeGCRuleTree();
 }
 
 void ServoStyleSet::RecordShadowStyleChange(ShadowRoot& aShadowRoot) {
   // TODO(emilio): We could keep track of the actual shadow roots that need
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -43,17 +43,18 @@ use selectors::bloom::BloomFilter;
 use selectors::matching::VisitedHandlingMode;
 use selectors::matching::{matches_selector, ElementSelectorFlags, MatchingContext, MatchingMode};
 use selectors::parser::{AncestorHashes, Combinator, Component, Selector};
 use selectors::parser::{SelectorIter, Visit};
 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;
 
 /// The type of the stylesheets that the stylist contains.
 #[cfg(feature = "servo")]
 pub type StylistSheet = crate::stylesheets::DocumentStyleSheet;
 
 /// The type of the stylesheets that the stylist contains.
@@ -123,25 +124,38 @@ impl UserAgentCascadeDataCache {
             )?;
         }
 
         let new_data = Arc::new(new_data);
         self.entries.push(new_data.clone());
         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")]
     fn add_size_of(&self, ops: &mut MallocSizeOfOps, sizes: &mut ServoStyleSetSizes) {
         sizes.mOther += self.entries.shallow_size_of(ops);
         for arc in self.entries.iter() {
             // These are primary Arc references that can be measured
             // unconditionally.
@@ -249,23 +263,28 @@ impl DocumentCascadeData {
         guards: &StylesheetGuards,
     ) -> Result<(), FailedAllocationError>
     where
         S: StylesheetInDocument + ToMediaListKey + PartialEq + 'static,
     {
         // 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()
+                };
             }
         }
 
         // Now do the user sheets.
         self.user.rebuild(
             device,
             quirks_mode,
             flusher.flush_origin(Origin::User),
@@ -1363,17 +1382,17 @@ impl Stylist {
         self.cascade_data.add_size_of(ops, sizes);
         sizes.mRuleTree += self.rule_tree.size_of(ops);
 
         // We may measure other fields in the future if DMD says it's worth it.
     }
 
     /// 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();
     }
 }
 
 /// This struct holds data which users of Stylist may want to extract
 /// from stylesheets which can be done at the same time as updating.
 #[derive(Debug, Default)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 pub struct ExtraStyleData {