Bug 795221 part 1. Implement cycle collection for nsCSSStyleSheet objects, so we don't leak through them. r=smaug,dbaron
authorBoris Zbarsky <bzbarsky@mit.edu>
Sun, 07 Oct 2012 22:39:08 -0400
changeset 109963 4a209968a7347e290206f7a17bed8fc8b1eacee9
parent 109962 2ec21afa445bfde8e432a15fcfab11efa2d4a768
child 109964 a2ab9f871f2f24f433eac3b7949a1aac7bb7b0ca
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewerssmaug, dbaron
bugs795221
milestone18.0a1
Bug 795221 part 1. Implement cycle collection for nsCSSStyleSheet objects, so we don't leak through them. r=smaug,dbaron Each nsCSSStyleSheet has a pointer to a nsCSSStyleSheetInner. The nsCSSStyleSheetInner is shared across multiple stylesheets, in general. The nsCSSStyleSheetInner owns the rules and the child stylesheets. What this means is that a given rule object is effectively owned by multiple sheets. However, cycles can only form through rule objects that have been JS-wrapped, and if we're JS-wrapping a rule object that means we have ensured that it's owned by only one stylesheet. Therefore, we only traverse and unlink mInner if it's uniquely owned by our sheet. Similarly, if our child sheets or any of their rules have been JS-wrapped, that means that we must have an mInner that we own outright.
layout/style/Rule.h
layout/style/StyleRule.cpp
layout/style/StyleRule.h
layout/style/nsCSSRules.cpp
layout/style/nsCSSRules.h
layout/style/nsCSSStyleSheet.cpp
layout/style/nsCSSStyleSheet.h
--- a/layout/style/Rule.h
+++ b/layout/style/Rule.h
@@ -20,19 +20,20 @@ class nsHTMLCSSStyleSheet;
 
 namespace mozilla {
 namespace css {
 class GroupRule;
 
 #define DECL_STYLE_RULE_INHERIT_NO_DOMRULE  \
 virtual void MapRuleInfoInto(nsRuleData* aRuleData);
 
-#define DECL_STYLE_RULE_INHERIT  \
-DECL_STYLE_RULE_INHERIT_NO_DOMRULE \
-virtual nsIDOMCSSRule* GetDOMRule();
+#define DECL_STYLE_RULE_INHERIT                   \
+  DECL_STYLE_RULE_INHERIT_NO_DOMRULE              \
+  virtual nsIDOMCSSRule* GetDOMRule();            \
+  virtual nsIDOMCSSRule* GetExistingDOMRule();
 
 class Rule : public nsIStyleRule {
 protected:
   Rule()
     : mSheet(0),
       mParentRule(nullptr)
   {
   }
@@ -102,16 +103,19 @@ public:
    * Clones |this|. Never returns NULL.
    */
   virtual already_AddRefed<Rule> Clone() const = 0;
 
   // Note that this returns null for inline style rules since they aren't
   // supposed to have a DOM rule representation (and our code wouldn't work).
   virtual nsIDOMCSSRule* GetDOMRule() = 0;
 
+  // Like GetDOMRule(), but won't create one if we don't have one yet
+  virtual nsIDOMCSSRule* GetExistingDOMRule() = 0;
+
   // to implement methods on nsIDOMCSSRule
   nsresult GetParentRule(nsIDOMCSSRule** aParentRule);
   nsresult GetParentStyleSheet(nsIDOMCSSStyleSheet** aSheet);
 
   // This is pure virtual because all of Rule's data members are non-owning and
   // thus measured elsewhere.
   virtual NS_MUST_OVERRIDE size_t
     SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const = 0;
--- a/layout/style/StyleRule.cpp
+++ b/layout/style/StyleRule.cpp
@@ -1401,16 +1401,22 @@ StyleRule::GetDOMRule()
   }
   if (!mDOMRule) {
     mDOMRule = new DOMCSSStyleRule(this);
     NS_ADDREF(mDOMRule);
   }
   return mDOMRule;
 }
 
+/* virtual */ nsIDOMCSSRule*
+StyleRule::GetExistingDOMRule()
+{
+  return mDOMRule;
+}
+
 /* virtual */ already_AddRefed<StyleRule>
 StyleRule::DeclarationChanged(Declaration* aDecl,
                               bool aHandleContainer)
 {
   StyleRule* clone = new StyleRule(*this, aDecl);
   if (!clone) {
     return nullptr;
   }
--- a/layout/style/StyleRule.h
+++ b/layout/style/StyleRule.h
@@ -344,16 +344,18 @@ public:
   void SetSelectorText(const nsAString& aSelectorText);
 
   virtual int32_t GetType() const;
 
   virtual already_AddRefed<Rule> Clone() const;
 
   virtual nsIDOMCSSRule* GetDOMRule();
 
+  virtual nsIDOMCSSRule* GetExistingDOMRule();
+
   // The new mapping function.
   virtual void MapRuleInfoInto(nsRuleData* aRuleData);
 
 #ifdef DEBUG
   virtual void List(FILE* out = stdout, int32_t aIndent = 0) const;
 #endif
 
   virtual size_t SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf) const;
--- a/layout/style/nsCSSRules.cpp
+++ b/layout/style/nsCSSRules.cpp
@@ -35,17 +35,19 @@
 #include "nsCSSParser.h"
 #include "nsPrintfCString.h"
 #include "nsDOMClassInfoID.h"
 #include "mozilla/dom/CSSStyleDeclarationBinding.h"
 
 namespace css = mozilla::css;
 
 #define IMPL_STYLE_RULE_INHERIT_GET_DOM_RULE_WEAK(class_, super_) \
-/* virtual */ nsIDOMCSSRule* class_::GetDOMRule() \
+  /* virtual */ nsIDOMCSSRule* class_::GetDOMRule()               \
+  { return this; }                                                \
+  /* virtual */ nsIDOMCSSRule* class_::GetExistingDOMRule()       \
   { return this; }
 #define IMPL_STYLE_RULE_INHERIT_MAP_RULE_INFO_INTO(class_, super_) \
 /* virtual */ void class_::MapRuleInfoInto(nsRuleData* aRuleData) \
   { NS_ABORT_IF_FALSE(false, "should not be called"); }
 
 #define IMPL_STYLE_RULE_INHERIT(class_, super_) \
 IMPL_STYLE_RULE_INHERIT_GET_DOM_RULE_WEAK(class_, super_) \
 IMPL_STYLE_RULE_INHERIT_MAP_RULE_INFO_INTO(class_, super_)
--- a/layout/style/nsCSSRules.h
+++ b/layout/style/nsCSSRules.h
@@ -60,16 +60,20 @@ public:
   // Rule methods
   virtual void SetStyleSheet(nsCSSStyleSheet* aSheet); //override GroupRule
   virtual int32_t GetType() const;
   virtual already_AddRefed<Rule> Clone() const;
   virtual nsIDOMCSSRule* GetDOMRule()
   {
     return this;
   }
+  virtual nsIDOMCSSRule* GetExistingDOMRule()
+  {
+    return this;
+  }
 
   // nsIDOMCSSRule interface
   NS_DECL_NSIDOMCSSRULE
 
   // nsIDOMCSSMediaRule interface
   NS_DECL_NSIDOMCSSMEDIARULE
 
   // rest of GroupRule
@@ -105,16 +109,20 @@ public:
 
   // Rule methods
   virtual int32_t GetType() const;
   virtual already_AddRefed<Rule> Clone() const;
   virtual nsIDOMCSSRule* GetDOMRule()
   {
     return this;
   }
+  virtual nsIDOMCSSRule* GetExistingDOMRule()
+  {
+    return this;
+  }
 
   // nsIDOMCSSRule interface
   NS_DECL_NSIDOMCSSRULE
 
   // nsIDOMCSSMozDocumentRule interface
   NS_DECL_NSIDOMCSSMOZDOCUMENTRULE
 
   // rest of GroupRule
@@ -395,16 +403,20 @@ public:
 
   // Rule methods
   virtual int32_t GetType() const;
   virtual already_AddRefed<mozilla::css::Rule> Clone() const;
   virtual nsIDOMCSSRule* GetDOMRule()
   {
     return this;
   }
+  virtual nsIDOMCSSRule* GetExistingDOMRule()
+  {
+    return this;
+  }
 
   // nsIDOMCSSRule interface
   NS_DECL_NSIDOMCSSRULE
 
   // nsIDOMMozCSSKeyframesRule interface
   NS_DECL_NSIDOMMOZCSSKEYFRAMESRULE
 
   // rest of GroupRule
@@ -439,16 +451,20 @@ public:
   virtual int32_t GetType() const;
   virtual already_AddRefed<mozilla::css::Rule> Clone() const;
   virtual bool UseForPresentation(nsPresContext* aPresContext,
                                   nsMediaQueryResultCacheKey& aKey);
   virtual nsIDOMCSSRule* GetDOMRule()
   {
     return this;
   }
+  virtual nsIDOMCSSRule* GetExistingDOMRule()
+  {
+    return this;
+  }
 
   NS_DECL_ISUPPORTS_INHERITED
 
   // nsIDOMCSSRule interface
   NS_DECL_NSIDOMCSSRULE
 
   // nsIDOMCSSSupportsRule interface
   NS_DECL_NSIDOMCSSSUPPORTSRULE
--- a/layout/style/nsCSSStyleSheet.cpp
+++ b/layout/style/nsCSSStyleSheet.cpp
@@ -1070,53 +1070,139 @@ nsCSSStyleSheet::~nsCSSStyleSheet()
        child = child->mNext) {
     // XXXbz this is a little bogus; see the XXX comment where we
     // declare mFirstChild.
     if (child->mParent == this) {
       child->mParent = nullptr;
       child->mDocument = nullptr;
     }
   }
-  if (nullptr != mRuleCollection) {
-    mRuleCollection->DropReference();
-    NS_RELEASE(mRuleCollection);
-  }
-  if (mMedia) {
-    mMedia->SetStyleSheet(nullptr);
-    mMedia = nullptr;
-  }
+  DropRuleCollection();
+  DropMedia();
   mInner->RemoveSheet(this);
   // XXX The document reference is not reference counted and should
   // not be released. The document will let us know when it is going
   // away.
   if (mRuleProcessors) {
     NS_ASSERTION(mRuleProcessors->Length() == 0, "destructing sheet with rule processor reference");
     delete mRuleProcessors; // weak refs, should be empty here anyway
   }
 }
 
+void
+nsCSSStyleSheet::DropRuleCollection()
+{
+  if (mRuleCollection) {
+    mRuleCollection->DropReference();
+    NS_RELEASE(mRuleCollection);
+  }
+}
+
+void
+nsCSSStyleSheet::DropMedia()
+{
+  if (mMedia) {
+    mMedia->SetStyleSheet(nullptr);
+    mMedia = nullptr;
+  }
+}
+
+void
+nsCSSStyleSheet::UnlinkInner()
+{
+  // We can only have a cycle through our inner if we have a unique inner,
+  // because otherwise there are no JS wrappers for anything in the inner.
+  if (mInner->mSheets.Length() != 1) {
+    return;
+  }
+
+  mInner->mOrderedRules.EnumerateForwards(SetStyleSheetReference, nullptr);
+  mInner->mOrderedRules.Clear();
+
+  // Have to be a bit careful with child sheets, because we want to
+  // drop their mNext pointers and null out their mParent and
+  // mDocument, but don't want to work with deleted objects.  And we
+  // don't want to do any addrefing in the process, just to make sure
+  // we don't confuse the cycle collector (though on the face of it,
+  // addref/release pairs during unlink should probably be ok).
+  nsRefPtr<nsCSSStyleSheet> child;
+  child.swap(mInner->mFirstChild);
+  while (child) {
+    MOZ_ASSERT(child->mParent == this, "We have a unique inner!");
+    child->mParent = nullptr;
+    child->mDocument = nullptr;
+    nsRefPtr<nsCSSStyleSheet> next;
+    // Null out child->mNext, but don't let it die yet
+    next.swap(child->mNext);
+    // Switch to looking at the old value of child->mNext next iteration
+    child.swap(next);
+    // "next" is now our previous value of child; it'll get released
+    // as we loop around.
+  }
+}
+
+void
+nsCSSStyleSheet::TraverseInner(nsCycleCollectionTraversalCallback &cb)
+{
+  // We can only have a cycle through our inner if we have a unique inner,
+  // because otherwise there are no JS wrappers for anything in the inner.
+  if (mInner->mSheets.Length() != 1) {
+    return;
+  }
+
+  nsRefPtr<nsCSSStyleSheet>* childSheetSlot = &mInner->mFirstChild;
+  while (*childSheetSlot) {
+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "child sheet");
+    cb.NoteXPCOMChild(NS_ISUPPORTS_CAST(nsIStyleSheet*, childSheetSlot->get()));
+    childSheetSlot = &(*childSheetSlot)->mNext;
+  }
+
+  const nsCOMArray<css::Rule>& rules = mInner->mOrderedRules;
+  for (int32_t i = 0, count = rules.Count(); i < count; ++i) {
+    NS_CYCLE_COLLECTION_NOTE_EDGE_NAME(cb, "mOrderedRules[i]");
+    cb.NoteXPCOMChild(rules[i]->GetExistingDOMRule());
+  }
+}
 
 DOMCI_DATA(CSSStyleSheet, nsCSSStyleSheet)
 
 // QueryInterface implementation for nsCSSStyleSheet
-NS_INTERFACE_MAP_BEGIN(nsCSSStyleSheet)
+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsCSSStyleSheet)
   NS_INTERFACE_MAP_ENTRY(nsIStyleSheet)
   NS_INTERFACE_MAP_ENTRY(nsIDOMStyleSheet)
   NS_INTERFACE_MAP_ENTRY(nsIDOMCSSStyleSheet)
   NS_INTERFACE_MAP_ENTRY(nsICSSLoaderObserver)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIStyleSheet)
   NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(CSSStyleSheet)
   if (aIID.Equals(NS_GET_IID(nsCSSStyleSheet)))
     foundInterface = reinterpret_cast<nsISupports*>(this);
   else
 NS_INTERFACE_MAP_END
 
 
-NS_IMPL_ADDREF(nsCSSStyleSheet)
-NS_IMPL_RELEASE(nsCSSStyleSheet)
+NS_IMPL_CYCLE_COLLECTING_ADDREF(nsCSSStyleSheet)
+NS_IMPL_CYCLE_COLLECTING_RELEASE(nsCSSStyleSheet)
+
+NS_IMPL_CYCLE_COLLECTION_CLASS(nsCSSStyleSheet)
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(nsCSSStyleSheet)
+  tmp->DropMedia();
+  // We do not unlink mNext; our parent will handle that.  If we
+  // unlinked it here, our parent would not be able to walk its list
+  // of child sheets and null out the back-references to it, if we got
+  // unlinked before it does.
+  tmp->DropRuleCollection();
+  tmp->UnlinkInner();
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(nsCSSStyleSheet)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mMedia)
+  // We do not traverse mNext; our parent will handle that.  See
+  // comments in Unlink for why.
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_RAWPTR(mRuleCollection)
+  tmp->TraverseInner(cb);
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
 
 
 nsresult
 nsCSSStyleSheet::AddRuleProcessor(nsCSSRuleProcessor* aProcessor)
 {
   if (! mRuleProcessors) {
     mRuleProcessors = new nsAutoTArray<nsCSSRuleProcessor*, 8>();
     if (!mRuleProcessors)
--- a/layout/style/nsCSSStyleSheet.h
+++ b/layout/style/nsCSSStyleSheet.h
@@ -16,16 +16,17 @@
 #include "nsAutoPtr.h"
 #include "nsIStyleSheet.h"
 #include "nsIDOMCSSStyleSheet.h"
 #include "nsICSSLoaderObserver.h"
 #include "nsCOMArray.h"
 #include "nsTArray.h"
 #include "nsString.h"
 #include "mozilla/CORSMode.h"
+#include "nsCycleCollectionParticipant.h"
 
 class nsXMLNameSpaceMap;
 class nsCSSRuleProcessor;
 class nsMediaList;
 class nsIPrincipal;
 class nsIURI;
 class nsMediaList;
 class nsMediaQueryResultCacheKey;
@@ -105,17 +106,19 @@ struct ChildSheetListBuilder;
 
 class nsCSSStyleSheet MOZ_FINAL : public nsIStyleSheet,
                                   public nsIDOMCSSStyleSheet,
                                   public nsICSSLoaderObserver
 {
 public:
   nsCSSStyleSheet(mozilla::CORSMode aCORSMode);
 
-  NS_DECL_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
+  NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsCSSStyleSheet,
+                                           nsIStyleSheet)
 
   NS_DECLARE_STATIC_IID_ACCESSOR(NS_CSS_STYLE_SHEET_IMPL_CID)
 
   // nsIStyleSheet interface
   virtual nsIURI* GetSheetURI() const;
   virtual nsIURI* GetBaseURI() const MOZ_OVERRIDE;
   virtual void GetTitle(nsString& aTitle) const MOZ_OVERRIDE;
   virtual void GetType(nsString& aType) const MOZ_OVERRIDE;
@@ -267,16 +270,27 @@ protected:
   // inner, error otherwise.  This will also succeed if the subject has
   // UniversalXPConnect or if access is allowed by CORS.  In the latter case,
   // it will set the principal of the inner to the subject principal.
   nsresult SubjectSubsumesInnerPrincipal();
 
   // Add the namespace mapping from this @namespace rule to our namespace map
   nsresult RegisterNamespaceRule(mozilla::css::Rule* aRule);
 
+  // Drop our reference to mRuleCollection
+  void DropRuleCollection();
+
+  // Drop our reference to mMedia
+  void DropMedia();
+
+  // Unlink our inner, if needed, for cycle collection
+  void UnlinkInner();
+  // Traverse our inner, if needed, for cycle collection
+  void TraverseInner(nsCycleCollectionTraversalCallback &);
+
 protected:
   nsString              mTitle;
   nsRefPtr<nsMediaList> mMedia;
   nsRefPtr<nsCSSStyleSheet> mNext;
   nsCSSStyleSheet*      mParent;    // weak ref
   mozilla::css::ImportRule* mOwnerRule; // weak ref
 
   CSSRuleListImpl*      mRuleCollection;