From 8d47bdf9ba14df4addb698786f86cb630cb4eeb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Mon, 22 Jul 2019 18:33:29 +0000 Subject: [PATCH] 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 | 2 ++ layout/style/CSSFontFaceRule.cpp | 13 ++++++------- layout/style/CSSPageRule.cpp | 12 ++++++------ layout/style/CSSStyleRule.cpp | 12 ++++++------ layout/style/Rule.cpp | 19 +++++++++++++++++++ layout/style/Rule.h | 4 ++++ layout/style/crashtests/1566684.html | 16 ++++++++++++++++ layout/style/crashtests/crashtests.list | 1 + 8 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 layout/style/crashtests/1566684.html diff --git a/dom/base/nsWrapperCache.cpp b/dom/base/nsWrapperCache.cpp index 49eb76a93daa..8117d225cbd0 100644 --- a/dom/base/nsWrapperCache.cpp +++ b/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); diff --git a/layout/style/CSSFontFaceRule.cpp b/layout/style/CSSFontFaceRule.cpp index 9a5fb287ed50..071b738f7d66 100644 --- a/layout/style/CSSFontFaceRule.cpp +++ b/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(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) diff --git a/layout/style/CSSPageRule.cpp b/layout/style/CSSPageRule.cpp index f581ec40d472..4dad89155aa1 100644 --- a/layout/style/CSSPageRule.cpp +++ b/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(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. diff --git a/layout/style/CSSStyleRule.cpp b/layout/style/CSSStyleRule.cpp index 1c550674db0e..f392cd774813 100644 --- a/layout/style/CSSStyleRule.cpp +++ b/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(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. diff --git a/layout/style/Rule.cpp b/layout/style/Rule.cpp index 59273c2208fd..14e1fdd6da30 100644 --- a/layout/style/Rule.cpp +++ b/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 diff --git a/layout/style/Rule.h b/layout/style/Rule.h index 7951a22e1b63..db1d08510039 100644 --- a/layout/style/Rule.h +++ b/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. // diff --git a/layout/style/crashtests/1566684.html b/layout/style/crashtests/1566684.html new file mode 100644 index 000000000000..328ec46a6b75 --- /dev/null +++ b/layout/style/crashtests/1566684.html @@ -0,0 +1,16 @@ + + + diff --git a/layout/style/crashtests/crashtests.list b/layout/style/crashtests/crashtests.list index 276f0fcf5066..eca63138fb06 100644 --- a/layout/style/crashtests/crashtests.list +++ b/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