Bug 1258017 - Redesign and simplify rule tree GC. r=dbaron

The basic idea here is as follows:
* Rule nodes are reference-counted, but releasing them adds them to a linked
  list rather than freeing them. This allows for the reuse that motivated the
  original GC scheme.
* We get rid of the marking, and instead rely on the reference count.
* Sweeping no longer requires a complicated traversal. We just pop items
  off the free list until it's empty. When a child is destroyed, its parent
  may go onto the free list.
* We remove special handling for the root node, and use a regular reference-counted
  edge from the style set.
* The free list automatically asserts that it's empty (meaning all nodes have been
  freed) in its destructor, which runs when the style set is destroyed.
* We get rid of the list of style context roots on the style set. We still need
  a count though, because of the HasCachedStyleData check.

layout/base/nsPresShell.cpp

@@ -868,9 +868,11 @@ PresShell::Init(nsIDocument* aDocument,
   mPresContext = aPresContext;
   aPresContext->SetShell(this);
 
-  // Now we can initialize the style set.
-  aStyleSet->Init(aPresContext);
+  // Now we can initialize the style set. Make sure to set the member before
+  // calling Init, since various subroutines need to find the style set off
+  // the PresContext during initialization.
   mStyleSet = aStyleSet;
+  mStyleSet->Init(aPresContext);
 
   // Notify our prescontext that it now has a compatibility mode.  Note that
   // this MUST happen after we set up our style set but before we create any

layout/style/StyleSetHandle.h

@@ -158,6 +158,9 @@ public:
         dom::Element* aPseudoElement,
         EventStates aStateMask);
 
+    inline void RootStyleContextAdded();
+    inline void RootStyleContextRemoved();
+
   private:
     // Stores a pointer to an nsStyleSet or a ServoStyleSet.  The least
     // significant bit is 0 for the former, 1 for the latter.  This is

layout/style/StyleSetHandleInlines.h

@@ -234,6 +234,26 @@ StyleSetHandle::Ptr::HasStateDependentStyle(dom::Element* aElement,
                                    aStateMask));
 }
 
+void
+StyleSetHandle::Ptr::RootStyleContextAdded()
+{
+  if (IsGecko()) {
+    AsGecko()->RootStyleContextAdded();
+  } else {
+    // Not needed.
+  }
+}
+
+void
+StyleSetHandle::Ptr::RootStyleContextRemoved()
+{
+  if (IsGecko()) {
+    RootStyleContextAdded();
+  } else {
+    // Not needed.
+  }
+}
+
 } // namespace mozilla
 
 #undef FORWARD

layout/style/nsRuleNode.cpp

@@ -1449,50 +1449,8 @@ nsRuleNode::operator new(size_t sz, nsPresContext* aPresContext) CPP_THROW_NEW
 // Overridden to prevent the global delete from being called, since the memory
 // came out of an nsIArena instead of the global delete operator's heap.
 void
-nsRuleNode::DestroyInternal(nsRuleNode ***aDestroyQueueTail)
+nsRuleNode::Destroy()
 {
-  nsRuleNode *destroyQueue, **destroyQueueTail;
-  if (aDestroyQueueTail) {
-    destroyQueueTail = *aDestroyQueueTail;
-  } else {
-    destroyQueue = nullptr;
-    destroyQueueTail = &destroyQueue;
-  }
-
-  if (ChildrenAreHashed()) {
-    PLDHashTable *children = ChildrenHash();
-    for (auto iter = children->Iter(); !iter.Done(); iter.Next()) {
-      auto entry = static_cast<ChildrenHashEntry*>(iter.Get());
-      *destroyQueueTail = entry->mRuleNode;
-      destroyQueueTail = &entry->mRuleNode->mNextSibling;
-    }
-    *destroyQueueTail = nullptr; // ensure null-termination
-    delete children;
-  } else if (HaveChildren()) {
-    *destroyQueueTail = ChildrenList();
-    do {
-      destroyQueueTail = &(*destroyQueueTail)->mNextSibling;
-    } while (*destroyQueueTail);
-  }
-  mChildren.asVoid = nullptr;
-
-  if (aDestroyQueueTail) {
-    // Our caller destroys the queue.
-    *aDestroyQueueTail = destroyQueueTail;
-  } else {
-    // We have to do destroy the queue.  When we destroy each node, it
-    // will add its children to the queue.
-    while (destroyQueue) {
-      nsRuleNode *cur = destroyQueue;
-      destroyQueue = destroyQueue->mNextSibling;
-      if (!destroyQueue) {
-        NS_ASSERTION(destroyQueueTail == &cur->mNextSibling, "mangled list");
-        destroyQueueTail = &destroyQueue;
-      }
-      cur->DestroyInternal(&destroyQueueTail);
-    }
-  }
-
   // Destroy ourselves.
   this->~nsRuleNode();
 
@@ -1501,10 +1459,11 @@ nsRuleNode::DestroyInternal(nsRuleNode ***aDestroyQueueTail)
   mPresContext->PresShell()->FreeByObjectID(eArenaObjectID_nsRuleNode, this);
 }
 
-nsRuleNode* nsRuleNode::CreateRootNode(nsPresContext* aPresContext)
+already_AddRefed<nsRuleNode>
+nsRuleNode::CreateRootNode(nsPresContext* aPresContext)
 {
-  return new (aPresContext)
-    nsRuleNode(aPresContext, nullptr, nullptr, SheetType::Unknown, false);
+  return do_AddRef(new (aPresContext)
+    nsRuleNode(aPresContext, nullptr, nullptr, SheetType::Unknown, false));
 }
 
 nsRuleNode::nsRuleNode(nsPresContext* aContext, nsRuleNode* aParent,
@@ -1529,15 +1488,9 @@ nsRuleNode::nsRuleNode(nsPresContext* aContext, nsRuleNode* aParent,
 
   NS_ASSERTION(IsRoot() || GetLevel() == aLevel, "not enough bits");
   NS_ASSERTION(IsRoot() || IsImportantRule() == aIsImportant, "yikes");
-  /* If IsRoot(), then aContext->StyleSet() is typically null at this
-     point.  In any case, we don't want to treat the root rulenode as
-     unused.  */
-  if (!IsRoot()) {
-    mParent->AddRef();
-    MOZ_ASSERT(aContext->StyleSet()->IsGecko(),
-               "ServoStyleSets should not have rule nodes");
-    aContext->StyleSet()->AsGecko()->RuleNodeUnused();
-  }
+  MOZ_ASSERT(aContext->StyleSet()->IsGecko(),
+             "ServoStyleSets should not have rule nodes");
+  aContext->StyleSet()->AsGecko()->RuleNodeUnused(this, /* aMayGC = */ false);
 
   // nsStyleSet::GetContext depends on there being only one animation
   // rule.
@@ -1549,7 +1502,12 @@ nsRuleNode::nsRuleNode(nsPresContext* aContext, nsRuleNode* aParent,
 
 nsRuleNode::~nsRuleNode()
 {
+  MOZ_ASSERT(!HaveChildren());
   MOZ_COUNT_DTOR(nsRuleNode);
+  if (mParent) {
+    mParent->RemoveChild(this);
+  }
+
   if (mStyleData.mResetData || mStyleData.mInheritedData)
     mStyleData.Destroy(mDependentBits, mPresContext);
 }
@@ -1659,6 +1617,34 @@ nsRuleNode::ConvertChildrenToHash(int32_t aNumKids)
   SetChildrenHash(hash);
 }
 
+void
+nsRuleNode::RemoveChild(nsRuleNode* aNode)
+{
+  MOZ_ASSERT(HaveChildren());
+  if (ChildrenAreHashed()) {
+    PLDHashTable* children = ChildrenHash();
+    Key key = aNode->GetKey();
+    MOZ_ASSERT(children->Search(&key));
+    children->Remove(&key);
+    if (children->EntryCount() == 0) {
+      delete children;
+      mChildren.asVoid = nullptr;
+    }
+  } else {
+    // This linear traversal is unfortunate, but we do the same thing when
+    // adding nodes. The traversal is bounded by kMaxChildrenInList.
+    nsRuleNode** curr = &mChildren.asList;
+    while (*curr != aNode) {
+      curr = &((*curr)->mNextSibling);
+      MOZ_ASSERT(*curr);
+    }
+    *curr = (*curr)->mNextSibling;
+
+    // If there was one element in the list, this sets mChildren.asList
+    // to 0, and HaveChildren() will return false.
+  }
+}
+
 inline void
 nsRuleNode::PropagateNoneBit(uint32_t aBit, nsRuleNode* aHighestNode)
 {
@@ -10091,116 +10077,6 @@ nsRuleNode::GetStyleData(nsStyleStructID aSID,
   return data;
 }
 
-void
-nsRuleNode::Mark()
-{
-  for (nsRuleNode *node = this;
-       node && !(node->mDependentBits & NS_RULE_NODE_GC_MARK);
-       node = node->mParent)
-    node->mDependentBits |= NS_RULE_NODE_GC_MARK;
-}
-
-bool
-nsRuleNode::DestroyIfNotMarked()
-{
-  // If we're not marked, then we have to delete ourself.
-  // However, we never allow the root node to GC itself, because nsStyleSet
-  // wants to hold onto the root node and not worry about re-creating a
-  // rule walker if the root node is deleted.
-  MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
-             "ServoStyleSets should not have rule nodes");
-  if (!(mDependentBits & NS_RULE_NODE_GC_MARK) &&
-      // Skip this only if we're the *current* root and not an old one.
-      !(IsRoot() && mPresContext->StyleSet()->AsGecko()->GetRuleTree() == this)) {
-    Destroy();
-    return true;
-  }
-
-  // Clear our mark, for the next time around.
-  mDependentBits &= ~NS_RULE_NODE_GC_MARK;
-  return false;
-}
-
-void
-nsRuleNode::SweepChildren(nsTArray<nsRuleNode*>& aSweepQueue)
-{
-  NS_ASSERTION(!(mDependentBits & NS_RULE_NODE_GC_MARK),
-               "missing DestroyIfNotMarked() call");
-  NS_ASSERTION(HaveChildren(),
-               "why call SweepChildren with no children?");
-  uint32_t childrenDestroyed = 0;
-  nsRuleNode* survivorsWithChildren = nullptr;
-  if (ChildrenAreHashed()) {
-    PLDHashTable* children = ChildrenHash();
-    uint32_t oldChildCount = children->EntryCount();
-    for (auto iter = children->Iter(); !iter.Done(); iter.Next()) {
-      auto entry = static_cast<ChildrenHashEntry*>(iter.Get());
-      nsRuleNode* node = entry->mRuleNode;
-      if (node->DestroyIfNotMarked()) {
-        iter.Remove();
-      } else if (node->HaveChildren()) {
-        // When children are hashed mNextSibling is not normally used but we
-        // use it here to build a list of children that needs to be swept.
-        nsRuleNode** headQ = &survivorsWithChildren;
-        node->mNextSibling = *headQ;
-        *headQ = node;
-      }
-    }
-    childrenDestroyed = oldChildCount - children->EntryCount();
-    if (childrenDestroyed == oldChildCount) {
-      delete children;
-      mChildren.asVoid = nullptr;
-    }
-  } else {
-    for (nsRuleNode** children = ChildrenListPtr(); *children; ) {
-      nsRuleNode* next = (*children)->mNextSibling;
-      if ((*children)->DestroyIfNotMarked()) {
-        // This rule node was destroyed, unlink it from the list by
-        // making *children point to the next entry.
-        *children = next;
-        ++childrenDestroyed;
-      } else {
-        children = &(*children)->mNextSibling;
-      }
-    }
-    survivorsWithChildren = ChildrenList();
-  }
-  if (survivorsWithChildren) {
-    aSweepQueue.AppendElement(survivorsWithChildren);
-  }
-  NS_ASSERTION(childrenDestroyed <= mRefCnt, "wrong ref count");
-  mRefCnt -= childrenDestroyed;
-  NS_POSTCONDITION(IsRoot() || mRefCnt > 0,
-                   "We didn't get swept, so we'd better have style contexts "
-                   "pointing to us or to one of our descendants, which means "
-                   "we'd better have a nonzero mRefCnt here!");
-}
-
-bool
-nsRuleNode::Sweep()
-{
-  NS_ASSERTION(IsRoot(), "must start sweeping at a root");
-  NS_ASSERTION(!mNextSibling, "root must not have mNextSibling");
-
-  if (DestroyIfNotMarked()) {
-    return true;
-  }
-
-  AutoTArray<nsRuleNode*, 70> sweepQueue;
-  sweepQueue.AppendElement(this);
-  while (!sweepQueue.IsEmpty()) {
-    nsTArray<nsRuleNode*>::index_type last = sweepQueue.Length() - 1;
-    nsRuleNode* ruleNode = sweepQueue[last];
-    sweepQueue.RemoveElementAt(last);
-    for (; ruleNode; ruleNode = ruleNode->mNextSibling) {
-      if (ruleNode->HaveChildren()) {
-        ruleNode->SweepChildren(sweepQueue);
-      }
-    }
-  }
-  return false;
-}
-
 /* static */ bool
 nsRuleNode::HasAuthorSpecifiedRules(nsStyleContext* aStyleContext,
                                     uint32_t ruleTypeMask,

layout/style/nsRuleNode.h

@@ -12,6 +12,7 @@
 #define nsRuleNode_h___
 
 #include "mozilla/ArenaObjectID.h"
+#include "mozilla/LinkedList.h"
 #include "mozilla/PodOperations.h"
 #include "mozilla/RangedArray.h"
 #include "mozilla/RuleNodeCacheConditions.h"
@@ -346,11 +347,11 @@ struct nsCachedStyleData
  * indexed by style rules (implementations of nsIStyleRule).
  *
  * The rule tree is owned by the nsStyleSet and is destroyed when the
- * presentation of the document goes away.  It is garbage-collected
- * (using mark-and-sweep garbage collection) during the lifetime of the
- * document (when dynamic changes cause the destruction of enough style
- * contexts).  Rule nodes are marked if they are pointed to by a style
- * context or one of their descendants is.
+ * presentation of the document goes away. Its entries are reference-
+ * counted, with strong references held by child nodes, style structs
+ * and (for the root), the style set. Rule nodes are not immediately
+ * destroyed when their reference-count drops to zero, but are instead
+ * destroyed during a GC sweep.
  *
  * An nsStyleContext, which represents the computed style data for an
  * element, points to an nsRuleNode.  The path from the root of the rule
@@ -389,7 +390,10 @@ enum nsFontSizeType {
   eFontSize_CSS = 2
 };
 
-class nsRuleNode {
+// Note: This LinkedListElement is used for storing unused nodes in the
+// linked list on nsStyleSet. We use mNextSibling for the singly-linked
+// sibling list.
+class nsRuleNode : public mozilla::LinkedListElement<nsRuleNode> {
 public:
   enum RuleDetail {
     eRuleNone, // No props have been specified at all.
@@ -415,11 +419,11 @@ public:
 private:
   nsPresContext* const mPresContext; // Our pres context.
 
-  nsRuleNode* const mParent; // A pointer to the parent node in the tree.
-                             // This enables us to walk backwards from the
-                             // most specific rule matched to the least
-                             // specific rule (which is the optimal order to
-                             // use for lookups of style properties.
+  const RefPtr<nsRuleNode> mParent; // A pointer to the parent node in the tree.
+                                    // This enables us to walk backwards from the
+                                    // most specific rule matched to the least
+                                    // specific rule (which is the optimal order to
+                                    // use for lookups of style properties.
 
   const nsCOMPtr<nsIStyleRule> mRule; // A pointer to our specific rule.
 
@@ -457,9 +461,6 @@ private:
   static bool
   ChildrenHashMatchEntry(const PLDHashEntryHdr *aHdr, const void *aKey);
 
-  void SweepChildren(nsTArray<nsRuleNode*>& aSweepQueue);
-  bool DestroyIfNotMarked();
-
   static const PLDHashTableOps ChildrenHashOps;
 
   Key GetKey() const {
@@ -515,6 +516,8 @@ private:
   }
   void ConvertChildrenToHash(int32_t aNumKids);
 
+  void RemoveChild(nsRuleNode* aNode);
+
   nsCachedStyleData mStyleData;   // Any data we cached on the rule node.
 
   uint32_t mDependentBits; // Used to cache the fact that we can look up
@@ -538,22 +541,17 @@ private:
                       // Compute*Data functions don't initialize from
                       // inherited data.
 
-  // Reference count.  This just counts the style contexts that reference this
-  // rulenode.  And children the rulenode has had.  When this goes to 0 or
-  // stops being 0, we notify the style set.
-  // Note, in particular, that when a child is removed mRefCnt is NOT
-  // decremented.  This is on purpose; the notifications to the style set are
-  // only used to determine when it's worth running GC on the ruletree, and
-  // this setup makes it so we only count unused ruletree leaves for purposes
-  // of deciding when to GC.  We could more accurately count unused rulenodes
-  // by releasing/addrefing our parent when our refcount transitions to or from
-  // 0, but it doesn't seem worth it to do that.
+  // Reference count. Style contexts hold strong references to their rule node,
+  // and rule nodes hold strong references to their parent.
+  //
+  // When the refcount drops to zero, we don't necessarily free the node.
+  // Instead, we notify the style set, which performs periodic sweeps.
   uint32_t mRefCnt;
 
 public:
   // Infallible overloaded new operator that allocates from a presShell arena.
   void* operator new(size_t sz, nsPresContext* aContext) CPP_THROW_NEW;
-  void Destroy() { DestroyInternal(nullptr); }
+  void Destroy();
 
   // Implemented in nsStyleSet.h, since it needs to know about nsStyleSet.
   inline void AddRef();
@@ -562,7 +560,6 @@ public:
   inline void Release();
 
 protected:
-  void DestroyInternal(nsRuleNode ***aDestroyQueueTail);
   void PropagateDependentBit(nsStyleStructID aSID, nsRuleNode* aHighestNode,
                              void* aStruct);
   void PropagateNoneBit(uint32_t aBit, nsRuleNode* aHighestNode);
@@ -809,7 +806,7 @@ private:
 
 public:
   // This is infallible; it will never return nullptr.
-  static nsRuleNode* CreateRootNode(nsPresContext* aPresContext);
+  static already_AddRefed<nsRuleNode> CreateRootNode(nsPresContext* aPresContext);
 
   static void EnsureBlockDisplay(uint8_t& display,
                                  bool aConvertListItem = false);
@@ -960,17 +957,6 @@ public:
   #undef STYLE_STRUCT_RESET
   #undef STYLE_STRUCT_INHERITED
 
-  /*
-   * Garbage collection.  Mark walks up the tree, marking any unmarked
-   * ancestors until it reaches a marked one.  Sweep recursively sweeps
-   * the children, destroys any that are unmarked, and clears marks,
-   * returning true if the node on which it was called was destroyed.
-   * If children are hashed, the mNextSibling field on the children is
-   * temporarily used internally by Sweep.
-   */
-  void Mark();
-  bool Sweep();
-
   static bool
     HasAuthorSpecifiedRules(nsStyleContext* aStyleContext,
                             uint32_t ruleTypeMask,

layout/style/nsStyleContext.cpp

@@ -115,21 +115,11 @@ nsStyleContext::nsStyleContext(nsStyleContext* aParent,
       r2 = r2->GetParent();
     NS_ASSERTION(r1 == r2, "must be in the same rule tree as parent");
 #endif
+  } else {
+    mRuleNode->PresContext()->PresShell()->StyleSet()->RootStyleContextAdded();
   }
 
-  mRuleNode->AddRef();
   mRuleNode->SetUsedDirectly(); // before ApplyStyleFixups()!
-
-  if (!mParent) {
-    // Add as a root before ApplyStyleFixups, since ApplyStyleFixups
-    // can trigger rule tree GC.
-    nsStyleSet* styleSet =
-      mRuleNode->PresContext()->PresShell()->StyleSet()->GetAsGecko();
-    if (styleSet) {
-      styleSet->AddStyleContextRoot(this);
-    }
-  }
-
   ApplyStyleFixups(aSkipParentDisplayBasedStyleFixup);
 
   #define eStyleStruct_LastItem (nsStyleStructID_Length - 1)
@@ -143,11 +133,10 @@ nsStyleContext::~nsStyleContext()
   NS_ASSERTION((nullptr == mChild) && (nullptr == mEmptyChild), "destructing context with children");
 
   nsPresContext *presContext = mRuleNode->PresContext();
-  nsStyleSet* styleSet = presContext->PresShell()->StyleSet()->GetAsGecko();
-
-  NS_ASSERTION(!styleSet ||
-               styleSet->GetRuleTree() == mRuleNode->RuleTree() ||
-               styleSet->IsInRuleTreeReconstruct(),
+  StyleSetHandle styleSet = presContext->PresShell()->StyleSet();
+  NS_ASSERTION(!styleSet->IsGecko() ||
+               styleSet->AsGecko()->GetRuleTree() == mRuleNode->RuleTree() ||
+               styleSet->AsGecko()->IsInRuleTreeReconstruct(),
                "destroying style context from old rule tree too late");
 
 #ifdef DEBUG
@@ -168,14 +157,10 @@ nsStyleContext::~nsStyleContext()
   }
 #endif
 
-  mRuleNode->Release();
-
-  if (styleSet) {
-    styleSet->NotifyStyleContextDestroyed(this);
-  }
-
   if (mParent) {
     mParent->RemoveChild(this);
+  } else {
+    styleSet->RootStyleContextRemoved();
   }
 
   // Free up our data structs.
@@ -1127,30 +1112,6 @@ nsStyleContext::CalcStyleDifference(nsStyleContext* aOther,
   return NS_SubtractHint(hint, nsChangeHint_NeutralChange);
 }
 
-void
-nsStyleContext::Mark()
-{
-  // Mark our rule node.
-  mRuleNode->Mark();
-
-  // Mark our children (i.e., tell them to mark their rule nodes, etc.).
-  if (mChild) {
-    nsStyleContext* child = mChild;
-    do {
-      child->Mark();
-      child = child->mNextSibling;
-    } while (mChild != child);
-  }
-  
-  if (mEmptyChild) {
-    nsStyleContext* child = mEmptyChild;
-    do {
-      child->Mark();
-      child = child->mNextSibling;
-    } while (mEmptyChild != child);
-  }
-}
-
 #ifdef DEBUG
 void nsStyleContext::List(FILE* out, int32_t aIndent, bool aListDescendants)
 {

layout/style/nsStyleContext.h

@@ -37,8 +37,6 @@ enum class CSSPseudoElementType : uint8_t;
  *  1. the |nsIFrame|s that are using the style context and
  *  2. any *child* style contexts (this might be the reverse of
  *     expectation, but it makes sense in this case)
- * Style contexts participate in the mark phase of rule node garbage
- * collection.
  */
 
 class nsStyleContext final
@@ -278,12 +276,6 @@ public:
   nsRuleNode* RuleNode() { return mRuleNode; }
   void AddStyleBit(const uint64_t& aBit) { mBits |= aBit; }
 
-  /*
-   * Mark this style context's rule node (and its ancestors) to prevent
-   * it from being garbage collected.
-   */
-  void Mark();
-
   /*
    * Get the style data for a style struct.  This is the most important
    * member function of nsStyleContext.  It fills in a const pointer
@@ -590,7 +582,7 @@ private:
   // specific rule matched is the one whose rule node is a child of the
   // root of the rule tree, and the most specific rule matched is the
   // |mRule| member of |mRuleNode|.
-  nsRuleNode* const       mRuleNode;
+  const RefPtr<nsRuleNode> mRuleNode;
 
   // mCachedInheritedData and mCachedResetData point to both structs that
   // are owned by this style context and structs that are owned by one of

layout/style/nsStyleSet.cpp

@@ -187,11 +187,16 @@ nsStyleSet::nsStyleSet()
   : mRuleTree(nullptr),
     mBatching(0),
     mInShutdown(false),
+    mInGC(false),
     mAuthorStyleDisabled(false),
     mInReconstruct(false),
     mInitFontFeatureValuesLookup(true),
     mNeedsRestyleAfterEnsureUniqueInner(false),
     mDirty(0),
+    mRootStyleContextCount(0),
+#ifdef DEBUG
+    mOldRootNode(nullptr),
+#endif
     mUnusedRuleNodeCount(0)
 {
 }
@@ -246,9 +251,6 @@ nsStyleSet::SizeOfIncludingThis(MallocSizeOf aMallocSizeOf) const
   }
   n += mScopedDocSheetRuleProcessors.ShallowSizeOfExcludingThis(aMallocSizeOf);
 
-  n += mRoots.ShallowSizeOfExcludingThis(aMallocSizeOf);
-  n += mOldRuleTrees.ShallowSizeOfExcludingThis(aMallocSizeOf);
-
   return n;
 }
 
@@ -277,26 +279,21 @@ nsStyleSet::BeginReconstruct()
 {
   NS_ASSERTION(!mInReconstruct, "Unmatched begin/end?");
   NS_ASSERTION(mRuleTree, "Reconstructing before first construction?");
+  mInReconstruct = true;
 
   // Clear any ArenaRefPtr-managed style contexts, as we don't want them
   // held on to after the rule tree has been reconstructed.
   PresContext()->PresShell()->ClearArenaRefPtrs(eArenaObjectID_nsStyleContext);
 
-  // Create a new rule tree root
-  nsRuleNode* newTree = nsRuleNode::CreateRootNode(mRuleTree->PresContext());
+#ifdef DEBUG
+  MOZ_ASSERT(!mOldRootNode);
+  mOldRootNode = mRuleTree;
+#endif
 
-  // Save the old rule tree so we can destroy it later
-  if (!mOldRuleTrees.AppendElement(mRuleTree)) {
-    newTree->Destroy();
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
-
-  // We need to keep mRoots so that the rule tree GC will only free the
-  // rule trees that really aren't referenced anymore (which should be
-  // all of them, if there are no bugs in reresolution code).
-
-  mInReconstruct = true;
-  mRuleTree = newTree;
+  // Create a new rule tree root, dropping the reference to our old rule tree.
+  // After reconstruction, we will re-enable GC, and allow everything to be
+  // collected.
+  mRuleTree = nsRuleNode::CreateRootNode(mRuleTree->PresContext());
 
   return NS_OK;
 }
@@ -306,22 +303,6 @@ nsStyleSet::EndReconstruct()
 {
   NS_ASSERTION(mInReconstruct, "Unmatched begin/end?");
   mInReconstruct = false;
-#ifdef DEBUG
-  for (int32_t i = mRoots.Length() - 1; i >= 0; --i) {
-    // Since nsStyleContext's mParent and mRuleNode are immutable, and
-    // style contexts own their parents, and nsStyleContext asserts in
-    // its constructor that the style context and its parent are in the
-    // same rule tree, we don't need to check any of the children of
-    // mRoots; we only need to check the rule nodes of mRoots
-    // themselves.
-
-    NS_ASSERTION(mRoots[i]->RuleNode()->RuleTree() == mRuleTree,
-                 "style context has old rule node");
-  }
-#endif
-  // This *should* destroy the only element of mOldRuleTrees, but in
-  // case of some bugs (which would trigger the above assertions), it
-  // won't.
   GCRuleTrees();
 }
 
@@ -2197,87 +2178,39 @@ void
 nsStyleSet::BeginShutdown()
 {
   mInShutdown = 1;
-  mRoots.Clear(); // no longer valid, since we won't keep it up to date
 }
 
 void
 nsStyleSet::Shutdown()
 {
-  mRuleTree->Destroy();
   mRuleTree = nullptr;
-
-  // We can have old rule trees either because:
-  //   (1) we failed the assertions in EndReconstruct, or
-  //   (2) we're shutting down within a reconstruct (see bug 462392)
-  for (uint32_t i = mOldRuleTrees.Length(); i > 0; ) {
-    --i;
-    mOldRuleTrees[i]->Destroy();
-  }
-  mOldRuleTrees.Clear();
+  GCRuleTrees();
+  MOZ_ASSERT(mUnusedRuleNodeList.isEmpty());
+  MOZ_ASSERT(mUnusedRuleNodeCount == 0);
 }
 
-static const uint32_t kGCInterval = 300;
-
-// Notification that a style context with a null parent has been created.
-void
-nsStyleSet::AddStyleContextRoot(nsStyleContext* aStyleContext)
-{
-  // aStyleContext has not been fully initialized, but its parent and
-  // rule node are correct.
-  MOZ_ASSERT(!aStyleContext->GetParent());
-  mRoots.AppendElement(aStyleContext);
-}
-
-void
-nsStyleSet::NotifyStyleContextDestroyed(nsStyleContext* aStyleContext)
-{
-  if (mInShutdown)
-    return;
-
-  // Remove style contexts from mRoots even if mOldRuleTree is non-null.  This
-  // could be a style context from the new ruletree!
-  if (!aStyleContext->GetParent()) {
-    mRoots.RemoveElement(aStyleContext);
-  }
-
-  if (mInReconstruct)
-    return;
-
-  if (mUnusedRuleNodeCount >= kGCInterval) {
-    GCRuleTrees();
-  }
-}
 
 void
 nsStyleSet::GCRuleTrees()
 {
-  mUnusedRuleNodeCount = 0;
+  MOZ_ASSERT(!mInReconstruct);
+  MOZ_ASSERT(!mInGC);
+  mInGC = true;
 
-  // Mark the style context tree by marking all style contexts which
-  // have no parent, which will mark all descendants.  This will reach
-  // style contexts in the undisplayed map and "additional style
-  // contexts" since they are descendants of the roots.
-  for (int32_t i = mRoots.Length() - 1; i >= 0; --i) {
-    mRoots[i]->Mark();
-  }
-
-  // Sweep the rule tree.
+  while (!mUnusedRuleNodeList.isEmpty()) {
+    nsRuleNode* node = mUnusedRuleNodeList.popFirst();
 #ifdef DEBUG
-  bool deleted =
-#endif
-    mRuleTree->Sweep();
-  NS_ASSERTION(!deleted, "Root node must not be gc'd");
-
-  // Sweep the old rule trees.
-  for (uint32_t i = mOldRuleTrees.Length(); i > 0; ) {
-    --i;
-    if (mOldRuleTrees[i]->Sweep()) {
-      // It was deleted, as it should be.
-      mOldRuleTrees.RemoveElementAt(i);
-    } else {
-      NS_NOTREACHED("old rule tree still referenced");
+    if (node == mOldRootNode) {
+      // Flag that we've GCed the old root, if any.
+      mOldRootNode = nullptr;
     }
+#endif
+    node->Destroy();
   }
+
+  MOZ_ASSERT(!mOldRootNode, "Should have GCed old root node");
+  mUnusedRuleNodeCount = 0;
+  mInGC = false;
 }
 
 already_AddRefed<nsStyleContext>

layout/style/nsStyleSet.h

@@ -15,6 +15,7 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/CSSStyleSheet.h"
 #include "mozilla/EnumeratedArray.h"
+#include "mozilla/LinkedList.h"
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/SheetType.h"
 
@@ -261,14 +262,6 @@ class nsStyleSet final
   // Free all of the data associated with this style set.
   void Shutdown();
 
-  // Notification that a style context with a null parent has been created.
-  // The argument is a style context that has not been fully initialized,
-  // but its parent and rule node are correct.
-  void AddStyleContextRoot(nsStyleContext* aStyleContext);
-
-  // Notification that a style context is being destroyed.
-  void NotifyStyleContextDestroyed(nsStyleContext* aStyleContext);
-
   // Get a new style context that lives in a different parent
   // The new context will be the same as the old if the new parent is the
   // same as the old parent.
@@ -363,6 +356,14 @@ class nsStyleSet final
     return mInReconstruct;
   }
 
+  void RootStyleContextAdded() {
+    ++mRootStyleContextCount;
+  }
+  void RootStyleContextRemoved() {
+    MOZ_ASSERT(mRootStyleContextCount > 0);
+    --mRootStyleContextCount;
+  }
+
   // Return whether the rule tree has cached data such that we need to
   // do dynamic change handling for changes that change the results of
   // media queries or require rebuilding all style data.
@@ -370,18 +371,25 @@ class nsStyleSet final
   // they have cached rule cascades; getting the rule cascades again in
   // order to do rule matching will get the correct rule cascade.
   bool HasCachedStyleData() const {
-    return (mRuleTree && mRuleTree->TreeHasCachedData()) || !mRoots.IsEmpty();
+    return (mRuleTree && mRuleTree->TreeHasCachedData()) || mRootStyleContextCount > 0;
   }
 
   // Notify the style set that a rulenode is no longer in use, or was
   // just created and is not in use yet.
-  void RuleNodeUnused() {
+  static const uint32_t kGCInterval = 300;
+  void RuleNodeUnused(nsRuleNode* aNode, bool aMayGC) {
     ++mUnusedRuleNodeCount;
+    mUnusedRuleNodeList.insertBack(aNode);
+    if (aMayGC && mUnusedRuleNodeCount >= kGCInterval && !mInGC && !mInReconstruct) {
+      GCRuleTrees();
+    }
   }
 
   // Notify the style set that a rulenode that wasn't in use now is
-  void RuleNodeInUse() {
+  void RuleNodeInUse(nsRuleNode* aNode) {
+    MOZ_ASSERT(mUnusedRuleNodeCount > 0);
     --mUnusedRuleNodeCount;
+    aNode->removeFrom(mUnusedRuleNodeList);
   }
 
   // Returns true if a restyle of the document is needed due to cloning
@@ -411,7 +419,11 @@ private:
   nsStyleSet(const nsStyleSet& aCopy) = delete;
   nsStyleSet& operator=(const nsStyleSet& aCopy) = delete;
 
-  // Run mark-and-sweep GC on mRuleTree and mOldRuleTrees, based on mRoots.
+  // Free all the rules with reference-count zero. This continues iterating
+  // over the free list until it is empty, which allows immediate collection
+  // of nodes whose reference-count drops to zero during the destruction of
+  // a child node. This allows the collection of entire trees at once, since
+  // children hold their parents alive.
   void GCRuleTrees();
 
   nsresult DirtyRuleProcessors(mozilla::SheetType aType);
@@ -498,21 +510,36 @@ private:
 
   RefPtr<nsBindingManager> mBindingManager;
 
-  nsRuleNode* mRuleTree; // This is the root of our rule tree.  It is a
-                         // lexicographic tree of matched rules that style
-                         // contexts use to look up properties.
+  RefPtr<nsRuleNode> mRuleTree; // This is the root of our rule tree.  It is a
+                                // lexicographic tree of matched rules that style
+                                // contexts use to look up properties.
 
   uint16_t mBatching;
 
   unsigned mInShutdown : 1;
+  unsigned mInGC : 1;
   unsigned mAuthorStyleDisabled: 1;
   unsigned mInReconstruct : 1;
   unsigned mInitFontFeatureValuesLookup : 1;
   unsigned mNeedsRestyleAfterEnsureUniqueInner : 1;
   unsigned mDirty : int(mozilla::SheetType::Count);  // one bit per sheet type
 
-  uint32_t mUnusedRuleNodeCount; // used to batch rule node GC
-  nsTArray<nsStyleContext*> mRoots; // style contexts with no parent
+  uint32_t mRootStyleContextCount;
+
+#ifdef DEBUG
+  // In debug builds, we stash a weak pointer here to the old root during
+  // reconstruction. During GC, we check for this pointer, and null it out
+  // when we encounter it. This allows us to assert that the old root (and
+  // thus all of its subtree) was GCed after reconstruction, which implies
+  // that there are no style contexts holding on to old rule nodes.
+  nsRuleNode* mOldRootNode;
+#endif
+
+  // Track our rule nodes with zero refcount. When this hits a threshold, we
+  // sweep and free. Keeping unused rule nodes around for a bit allows us to
+  // reuse them in many cases.
+  mozilla::LinkedList<nsRuleNode> mUnusedRuleNodeList;
+  uint32_t mUnusedRuleNodeCount;
 
   // Empty style rules to force things that restrict which properties
   // apply into different branches of the rule tree.
@@ -526,11 +553,6 @@ private:
   // <svg:text> elements to disable the effect of text zooming.
   RefPtr<nsDisableTextZoomStyleRule> mDisableTextZoomStyleRule;
 
-  // Old rule trees, which should only be non-empty between
-  // BeginReconstruct and EndReconstruct, but in case of bugs that cause
-  // style contexts to exist too long, may last longer.
-  nsTArray<nsRuleNode*> mOldRuleTrees;
-
   // whether font feature values lookup object needs initialization
   RefPtr<gfxFontFeatureValueSet> mFontFeatureValuesLookup;
 };
@@ -539,20 +561,20 @@ private:
 inline
 void nsRuleNode::AddRef()
 {
-  if (mRefCnt++ == 0 && !IsRoot()) {
+  if (mRefCnt++ == 0) {
     MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
                "ServoStyleSets should not have rule nodes");
-    mPresContext->StyleSet()->AsGecko()->RuleNodeInUse();
+    mPresContext->StyleSet()->AsGecko()->RuleNodeInUse(this);
   }
 }
 
 inline
 void nsRuleNode::Release()
 {
-  if (--mRefCnt == 0 && !IsRoot()) {
+  if (--mRefCnt == 0) {
     MOZ_ASSERT(mPresContext->StyleSet()->IsGecko(),
                "ServoStyleSets should not have rule nodes");
-    mPresContext->StyleSet()->AsGecko()->RuleNodeUnused();
+    mPresContext->StyleSet()->AsGecko()->RuleNodeUnused(this, /* aMayGC = */ true);
   }
 }
 #endif

layout/style/nsStyleStruct.h

@@ -84,7 +84,7 @@ struct nsStyleVisibility;
 
 // Additional bits for nsRuleNode's mDependentBits:
 #define NS_RULE_NODE_IS_ANIMATION_RULE      0x01000000
-#define NS_RULE_NODE_GC_MARK                0x02000000
+// Free bit here!
 #define NS_RULE_NODE_USED_DIRECTLY          0x04000000
 #define NS_RULE_NODE_IS_IMPORTANT           0x08000000
 #define NS_RULE_NODE_LEVEL_MASK             0xf0000000