Bug 1593865 - Simplify code for keeping alive shared memory until all sheets go away. r=jwatt
☠☠ backed out by 4dfacb8bd61a ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 06 Nov 2019 16:17:15 +0000
changeset 500878 6fd25e6c3d1a58782e34e8d7f53e995e03683036
parent 500877 9d78accbdc5108a7347af693ccdf4505180ca567
child 500879 40de560a7cadb4f2c5cb388aac0afebdab650827
push id114166
push userapavel@mozilla.com
push dateThu, 07 Nov 2019 10:04:01 +0000
treeherdermozilla-inbound@d271c572a9bc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1593865
milestone72.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 1593865 - Simplify code for keeping alive shared memory until all sheets go away. r=jwatt The existing code wasn't sound, as CSSOM objects also needed to go away before the shared memory goes away (as they keep references to them). This is sound assuming no presence of reference cycles introduced by CSSOM. We may want to live with this and rely on chrome code not writing cycles like this with UA stylesheet DOM objects. We could explicitly drop all potentially-static objects... That seems pretty error prone though. Or we could also just leak the shared memory buffer, is there any reason why we may not want to do that? Differential Revision: https://phabricator.services.mozilla.com/D51870
layout/style/StyleSheet.cpp
layout/style/StyleSheet.h
layout/style/StyleSheetInfo.h
layout/style/nsLayoutStylesheetCache.cpp
layout/style/nsLayoutStylesheetCache.h
servo/components/servo_arc/lib.rs
servo/ports/geckolib/glue.rs
--- a/layout/style/StyleSheet.cpp
+++ b/layout/style/StyleSheet.cpp
@@ -296,56 +296,45 @@ StyleSheetInfo::StyleSheetInfo(StyleShee
       mIntegrity(aCopy.mIntegrity),
       mFirstChild(),  // We don't rebuild the child because we're making a copy
                       // without children.
       mSourceMapURL(aCopy.mSourceMapURL),
       mSourceMapURLFromComment(aCopy.mSourceMapURLFromComment),
       mSourceURL(aCopy.mSourceURL),
       mContents(Servo_StyleSheet_Clone(aCopy.mContents.get(), aPrimarySheet)
                     .Consume()),
-      // Cloning aCopy.mContents will still leave us with some references to
-      // data in shared memory (for example, any SelectorList objects will still
-      // be shared), so continue to keep it alive.
-      mSharedMemory(aCopy.mSharedMemory),
       mURLData(aCopy.mURLData)
 #ifdef DEBUG
       ,
       mPrincipalSet(aCopy.mPrincipalSet)
 #endif
 {
   AddSheet(aPrimarySheet);
 
   // Our child list is fixed up by our parent.
   MOZ_COUNT_CTOR(StyleSheetInfo);
 }
 
 StyleSheetInfo::~StyleSheetInfo() {
   MOZ_COUNT_DTOR(StyleSheetInfo);
-
-  // Drop the sheet contents before the shared memory.
-  mContents = nullptr;
 }
 
 StyleSheetInfo* StyleSheetInfo::CloneFor(StyleSheet* aPrimarySheet) {
   return new StyleSheetInfo(*this, aPrimarySheet);
 }
 
 MOZ_DEFINE_MALLOC_SIZE_OF(ServoStyleSheetMallocSizeOf)
 MOZ_DEFINE_MALLOC_ENCLOSING_SIZE_OF(ServoStyleSheetMallocEnclosingSizeOf)
 
 size_t StyleSheetInfo::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const {
   size_t n = aMallocSizeOf(this);
 
-  // If this sheet came from shared memory, then it will be counted by
-  // nsLayoutStylesheetCache in the parent process.
-  if (!mSharedMemory) {
-    n += Servo_StyleSheet_SizeOfIncludingThis(
-        ServoStyleSheetMallocSizeOf, ServoStyleSheetMallocEnclosingSizeOf,
-        mContents);
-  }
+  n += Servo_StyleSheet_SizeOfIncludingThis(
+      ServoStyleSheetMallocSizeOf, ServoStyleSheetMallocEnclosingSizeOf,
+      mContents);
 
   return n;
 }
 
 void StyleSheetInfo::AddSheet(StyleSheet* aSheet) {
   mSheets.AppendElement(aSheet);
 }
 
@@ -1218,28 +1207,21 @@ nsresult StyleSheet::InsertRuleIntoGroup
   MOZ_ASSERT(rules->GetParentRule() == aGroup);
   return rules->InsertRule(aRule, aIndex);
 }
 
 StyleOrigin StyleSheet::GetOrigin() const {
   return Servo_StyleSheet_GetOrigin(Inner().mContents);
 }
 
-void StyleSheet::SetSharedContents(nsLayoutStylesheetCache::Shm* aSharedMemory,
-                                   const ServoCssRules* aSharedRules) {
-  MOZ_ASSERT(aSharedMemory);
+void StyleSheet::SetSharedContents(const ServoCssRules* aSharedRules) {
   MOZ_ASSERT(!IsComplete());
 
   SetURLExtraData();
 
-  // Hold a strong reference to the shared memory that aSharedRules comes
-  // from, so that we don't end up releasing the shared memory before the
-  // StyleSheetInner.
-  Inner().mSharedMemory = aSharedMemory;
-
   Inner().mContents =
       Servo_StyleSheet_FromSharedData(Inner().mURLData, aSharedRules).Consume();
 
   // Don't call FinishParse(), since that tries to set source map URLs,
   // which we don't have.
 }
 
 const ServoCssRules* StyleSheet::ToShared(
--- a/layout/style/StyleSheet.h
+++ b/layout/style/StyleSheet.h
@@ -385,18 +385,17 @@ class StyleSheet final : public nsICSSLo
   // Copy the contents of this style sheet into the shared memory buffer managed
   // by aBuilder.  Returns the pointer into the buffer that the sheet contents
   // were stored at.  (The returned pointer is to an Arc<Locked<Rules>> value.)
   const ServoCssRules* ToShared(RawServoSharedMemoryBuilder* aBuilder);
 
   // Sets the contents of this style sheet to the specified aSharedRules
   // pointer, which must be a pointer somewhere in the aSharedMemory buffer
   // as previously returned by a ToShared() call.
-  void SetSharedContents(nsLayoutStylesheetCacheShm* aSharedMemory,
-                         const ServoCssRules* aSharedRules);
+  void SetSharedContents(const ServoCssRules* aSharedRules);
 
   // Whether this style sheet should not allow any modifications.
   //
   // This is true for any User Agent sheets once they are complete.
   bool IsReadOnly() const;
 
  private:
   dom::ShadowRoot* GetContainingShadow() const;
--- a/layout/style/StyleSheetInfo.h
+++ b/layout/style/StyleSheetInfo.h
@@ -72,22 +72,16 @@ struct StyleSheetInfo final {
   // come from a response header.
   nsString mSourceMapURLFromComment;
   // This stores any source URL that might have been seen in a comment
   // in the style sheet.
   nsString mSourceURL;
 
   RefPtr<const RawServoStyleSheetContents> mContents;
 
-  // The shared memory buffer that stores the rules in the style sheet, if
-  // this style sheet was loaded from the style sheet cache's shared memory.
-  //
-  // We need to hold on to this so it doesn't go away before we do.
-  RefPtr<nsLayoutStylesheetCacheShm> mSharedMemory;
-
   // XXX We already have mSheetURI, mBaseURI, and mPrincipal.
   //
   // Can we somehow replace them with URLExtraData directly? The issue
   // is currently URLExtraData is immutable, but URIs in StyleSheetInfo
   // seems to be mutable, so we probably cannot set them altogether.
   // Also, this is mostly a duplicate reference of the same url data
   // inside RawServoStyleSheet. We may want to just use that instead.
   RefPtr<URLExtraData> mURLData;
--- a/layout/style/nsLayoutStylesheetCache.cpp
+++ b/layout/style/nsLayoutStylesheetCache.cpp
@@ -283,39 +283,39 @@ nsLayoutStylesheetCache::nsLayoutStylesh
   // lazily load our own copies of the sheets later.
   if (mSharedMemory) {
     if (auto header = static_cast<Header*>(mSharedMemory->mShm.memory())) {
       MOZ_RELEASE_ASSERT(header->mMagic == Header::kMagic);
 
 #define STYLE_SHEET(identifier_, url_, shared_)                           \
   if (shared_) {                                                          \
     LoadSheetFromSharedMemory(url_, &m##identifier_##Sheet,               \
-                              eAgentSheetFeatures, mSharedMemory, header, \
+                              eAgentSheetFeatures, header, \
                               UserAgentStyleSheetID::identifier_);        \
   }
 #include "mozilla/UserAgentStyleSheetList.h"
 #undef STYLE_SHEET
     }
   }
 }
 
 void nsLayoutStylesheetCache::LoadSheetFromSharedMemory(
     const char* aURL, RefPtr<StyleSheet>* aSheet, SheetParsingMode aParsingMode,
-    Shm* aSharedMemory, Header* aHeader, UserAgentStyleSheetID aSheetID) {
+    Header* aHeader, UserAgentStyleSheetID aSheetID) {
   auto i = size_t(aSheetID);
 
   auto sheet =
       MakeRefPtr<StyleSheet>(aParsingMode, CORS_NONE, dom::SRIMetadata());
 
   nsCOMPtr<nsIURI> uri;
   MOZ_ALWAYS_SUCCEEDS(NS_NewURI(getter_AddRefs(uri), aURL));
 
   sheet->SetPrincipal(nsContentUtils::GetSystemPrincipal());
   sheet->SetURIs(uri, uri, uri);
-  sheet->SetSharedContents(aSharedMemory, aHeader->mSheets[i]);
+  sheet->SetSharedContents(aHeader->mSheets[i]);
   sheet->SetComplete();
 
   nsCOMPtr<nsIReferrerInfo> referrerInfo =
       dom::ReferrerInfo::CreateForExternalCSSResources(sheet);
   sheet->SetReferrerInfo(referrerInfo);
   URLExtraData::sShared[i] = sheet->URLData();
 
   *aSheet = sheet.forget();
@@ -426,16 +426,27 @@ void nsLayoutStylesheetCache::InitShared
   // committed.
   size_t pageSize = ipc::SharedMemory::SystemPageSize();
   mUsedSharedMemory =
       (Servo_SharedMemoryBuilder_GetLength(builder.get()) + pageSize - 1) &
       ~(pageSize - 1);
 }
 
 nsLayoutStylesheetCache::~nsLayoutStylesheetCache() {
+  // Ensure stylesheets are released _before_ the shared memory.
+#define STYLE_SHEET(identifier_, url_, shared_) \
+  m##identifier_##Sheet = nullptr;
+#include "mozilla/UserAgentStyleSheetList.h"
+#undef STYLE_SHEET
+
+  mChromePreferenceSheet = nullptr;
+  mContentPreferenceSheet = nullptr;
+  mUserChromeSheet = nullptr;
+  mUserContentSheet = nullptr;
+
   mozilla::UnregisterWeakMemoryReporter(this);
 }
 
 void nsLayoutStylesheetCache::InitMemoryReporter() {
   mozilla::RegisterWeakMemoryReporter(this);
 }
 
 /* static */
--- a/layout/style/nsLayoutStylesheetCache.h
+++ b/layout/style/nsLayoutStylesheetCache.h
@@ -119,19 +119,18 @@ class nsLayoutStylesheetCache final : pu
       mozilla::css::FailureAction aFailureAction);
   RefPtr<mozilla::StyleSheet> LoadSheetFile(
       nsIFile* aFile, mozilla::css::SheetParsingMode aParsingMode);
   RefPtr<mozilla::StyleSheet> LoadSheet(
       nsIURI* aURI, mozilla::css::SheetParsingMode aParsingMode,
       mozilla::css::FailureAction aFailureAction);
   void LoadSheetFromSharedMemory(const char* aURL,
                                  RefPtr<mozilla::StyleSheet>* aSheet,
-                                 mozilla::css::SheetParsingMode aParsingMode,
-                                 Shm* aSharedMemory, Header* aHeader,
-                                 mozilla::UserAgentStyleSheetID aSheetID);
+                                 mozilla::css::SheetParsingMode,
+                                 Header*, mozilla::UserAgentStyleSheetID);
   void BuildPreferenceSheet(RefPtr<mozilla::StyleSheet>* aSheet,
                             const mozilla::PreferenceSheet::Prefs&);
 
   static mozilla::StaticRefPtr<nsLayoutStylesheetCache> gStyleCache;
   static mozilla::StaticRefPtr<mozilla::css::Loader> gCSSLoader;
   static mozilla::StaticRefPtr<nsIURI> gUserContentSheetURL;
 
 #define STYLE_SHEET(identifier_, url_, shared_) \
--- a/servo/components/servo_arc/lib.rs
+++ b/servo/components/servo_arc/lib.rs
@@ -480,36 +480,41 @@ impl<T: ?Sized> Arc<T> {
                 // See make_mut() for documentation of the threadsafety here.
                 Some(&mut (*this.ptr()).data)
             }
         } else {
             None
         }
     }
 
+    /// Whether or not the `Arc` is a static reference.
+    #[inline]
+    pub fn is_static(&self) -> bool {
+        // Using a relaxed ordering to check for STATIC_REFCOUNT is safe, since
+        // `count` never changes between STATIC_REFCOUNT and other values.
+        self.inner().count.load(Relaxed) == STATIC_REFCOUNT
+    }
+
     /// Whether or not the `Arc` is uniquely owned (is the refcount 1?) and not
     /// a static reference.
     #[inline]
     pub fn is_unique(&self) -> bool {
         // See the extensive discussion in [1] for why this needs to be Acquire.
         //
         // [1] https://github.com/servo/servo/issues/21186
         self.inner().count.load(Acquire) == 1
     }
 }
 
 impl<T: ?Sized> Drop for Arc<T> {
     #[inline]
     fn drop(&mut self) {
         // NOTE(emilio): If you change anything here, make sure that the
         // implementation in layout/style/ServoStyleConstsInlines.h matches!
-        //
-        // Using a relaxed ordering to check for STATIC_REFCOUNT is safe, since
-        // `count` never changes between STATIC_REFCOUNT and other values.
-        if self.inner().count.load(Relaxed) == STATIC_REFCOUNT {
+        if self.is_static() {
             return;
         }
 
         // Because `fetch_sub` is already atomic, we do not need to synchronize
         // with other threads unless we are going to delete the object.
         if self.inner().count.fetch_sub(1, Release) != 1 {
             return;
         }
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -1968,17 +1968,21 @@ pub extern "C" fn Servo_StyleSheet_SizeO
 ) -> usize {
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
     let mut ops = MallocSizeOfOps::new(
         malloc_size_of.unwrap(),
         Some(malloc_enclosing_size_of.unwrap()),
         None,
     );
-    StylesheetContents::as_arc(&sheet).size_of(&guard, &mut ops)
+    let arc = StylesheetContents::as_arc(&sheet);
+    if arc.with_arc(|arc| arc.is_static()) {
+        return 0;
+    }
+    arc.size_of(&guard, &mut ops)
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_StyleSheet_GetOrigin(sheet: &RawServoStyleSheetContents) -> Origin {
     StylesheetContents::as_arc(&sheet).origin
 }
 
 #[no_mangle]