Bug 1566684 - Make sure to unlink rule declarations before unlinking css::Rule. r=smaug,emilio

Rules and their declarations are a single object as far as the CC is concerned.  They have a single nsCycleCollectionISupports and they are represented by a single node in the CC graph.  That single object has two nsWrapperCache instances in it that point to different JS objects, and we need to make sure that the ordering of the unlink operations for those nsWrapperCache instances is handled correctly.

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

--HG--
extra : moz-landing-system : lando

dom/base/nsWrapperCache.cpp

@@ -45,6 +45,8 @@ void nsWrapperCache::SetWrapperJSObject(JSObject* aWrapper) {
 }
 
 void nsWrapperCache::ReleaseWrapper(void* aScriptObjectHolder) {
+  // If the behavior here changes in a substantive way, you may need
+  // to update css::Rule::UnlinkDeclarationWrapper as well.
   if (PreservingWrapper()) {
     SetPreservingWrapper(false);
     cyclecollector::DropJSObjectsImpl(aScriptObjectHolder);

layout/style/CSSFontFaceRule.cpp

@@ -150,15 +150,14 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(CSSFontFaceRule,
   tmp->mDecl.TraceWrapper(aCallbacks, aClosure);
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(CSSFontFaceRule,
-                                                mozilla::css::Rule)
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CSSFontFaceRule)
   // Keep this in sync with IsCCLeaf.
 
-  // Unlink the wrapper for our declaraton.  This just expands out
-  // NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER which we can't use
-  // directly because the wrapper is on the declaration, not on us.
-  tmp->mDecl.ReleaseWrapper(static_cast<nsISupports*>(p));
-NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+  // Unlink the wrapper for our declaration.
+  //
+  // Note that this has to happen before unlinking css::Rule.
+  tmp->UnlinkDeclarationWrapper(tmp->mDecl);
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(mozilla::css::Rule)
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(CSSFontFaceRule,
                                                   mozilla::css::Rule)

layout/style/CSSPageRule.cpp

@@ -103,15 +103,15 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(CSSPageRule, css::Rule)
   tmp->mDecls.TraceWrapper(aCallbacks, aClosure);
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(CSSPageRule, css::Rule)
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CSSPageRule)
   // Keep this in sync with IsCCLeaf.
 
-  // Unlink the wrapper for our declaraton.  This just expands out
-  // NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER which we can't use
-  // directly because the wrapper is on the declaration, not on us.
-  tmp->mDecls.ReleaseWrapper(static_cast<nsISupports*>(p));
+  // Unlink the wrapper for our declaraton.
+  //
+  // Note that this has to happen before unlinking css::Rule.
+  tmp->UnlinkDeclarationWrapper(tmp->mDecls);
   tmp->mDecls.mDecls->SetOwningRule(nullptr);
-NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(css::Rule)
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(CSSPageRule, css::Rule)
   // Keep this in sync with IsCCLeaf.

layout/style/CSSStyleRule.cpp

@@ -105,14 +105,14 @@ NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED(CSSStyleRule, css::Rule)
   tmp->mDecls.TraceWrapper(aCallbacks, aClosure);
 NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
-NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(CSSStyleRule, css::Rule)
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(CSSStyleRule)
   // Keep this in sync with IsCCLeaf.
 
-  // Unlink the wrapper for our declaraton.  This just expands out
-  // NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER which we can't use
-  // directly because the wrapper is on the declaration, not on us.
-  tmp->mDecls.ReleaseWrapper(static_cast<nsISupports*>(p));
-NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+  // Unlink the wrapper for our declaraton.
+  //
+  // Note that this has to happen before unlinking css::Rule.
+  tmp->UnlinkDeclarationWrapper(tmp->mDecls);
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END_INHERITED(css::Rule)
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(CSSStyleRule, css::Rule)
   // Keep this in sync with IsCCLeaf.

layout/style/Rule.cpp

@@ -50,6 +50,25 @@ bool Rule::IsKnownLive() const {
       GetComposedDoc()->GetMarkedCCGeneration());
 }
 
+void Rule::UnlinkDeclarationWrapper(nsWrapperCache& aDecl) {
+  // We have to be a bit careful here.  We have two separate nsWrapperCache
+  // instances, aDecl and this, that both correspond to the same CC participant:
+  // this.  If we just used ReleaseWrapper() on one of them, that would
+  // unpreserve that one wrapper, then trace us with a tracer that clears JS
+  // things, and we would clear the wrapper on the cache that has not
+  // unpreserved the wrapper yet.  That would violate the invariant that the
+  // cache keeps caching the wrapper until the wrapper dies.
+  //
+  // So we reimplement a modified version of nsWrapperCache::ReleaseWrapper here
+  // that unpreserves both wrappers before doing any clearing.
+  bool needDrop = PreservingWrapper() || aDecl.PreservingWrapper();
+  SetPreservingWrapper(false);
+  aDecl.SetPreservingWrapper(false);
+  if (needDrop) {
+    DropJSObjects(this);
+  }
+}
+
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(Rule)
   return tmp->IsCCLeaf() || tmp->IsKnownLive();
 NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END

layout/style/Rule.h

@@ -115,6 +115,10 @@ class Rule : public nsISupports, public nsWrapperCache {
   // True if we're known-live for cycle collection purposes.
   bool IsKnownLive() const;
 
+  // Hook subclasses can use to properly unlink the nsWrapperCache of
+  // their declarations.
+  void UnlinkDeclarationWrapper(nsWrapperCache& aDecl);
+
   // mSheet should only ever be null when we create a synthetic CSSFontFaceRule
   // for an InspectorFontFace.
   //

layout/style/crashtests/1566684.html

@@ -0,0 +1,16 @@
+<!doctype html>
+<style>
+  x { }
+</style>
+<script>
+  var sheet = document.styleSheets[0];
+  var rule = sheet.cssRules[0];
+  var decl = rule.style;
+  rule.expando = 5;
+  decl.expando = 6;
+  sheet.deleteRule(0);
+  rule = decl = null;
+  SpecialPowers.forceGC();
+  SpecialPowers.forceCC();
+  SpecialPowers.forceGC();
+</script>

layout/style/crashtests/crashtests.list

@@ -307,3 +307,4 @@ load 1545177.html
 skip-if(geckoview&&webrender) skip-if(Android&&!isDebugBuild) load 1546255.html # Bug 1563020 for GV+WR & Bug 1553971
 pref(layout.css.resizeobserver.enabled,true) load 1552911.html
 load 1562361.html
+load 1566684.html