Bug 1127198 - Part 4: Clear cached structs only after we have fully processed a restyle. r=dbaron a=abillings
authorCameron McCormack <cam@mcc.id.au>
Wed, 18 Feb 2015 09:29:05 +1100
changeset 249773 4b95c24edb895312c49acb01d0c651b54dfb24c4
parent 249772 bf7a3467b5a6b309b2574a82a67706c986b4c16d
child 249774 e62dd2ded2fc5aa2c2ec1028defc85abc9e54807
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, abillings
bugs1127198
milestone37.0a2
Bug 1127198 - Part 4: Clear cached structs only after we have fully processed a restyle. r=dbaron a=abillings
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -2350,31 +2350,36 @@ RestyleManager::ReparentStyleContext(nsI
 
 ElementRestyler::ElementRestyler(nsPresContext* aPresContext,
                                  nsIFrame* aFrame,
                                  nsStyleChangeList* aChangeList,
                                  nsChangeHint aHintsHandledByAncestors,
                                  RestyleTracker& aRestyleTracker,
                                  TreeMatchContext& aTreeMatchContext,
                                  nsTArray<nsIContent*>&
-                                   aVisibleKidsOfHiddenElement)
+                                   aVisibleKidsOfHiddenElement,
+                                 nsTArray<ContextToClear>& aContextsToClear,
+                                 nsTArray<nsRefPtr<nsStyleContext>>&
+                                   aSwappedStructOwners)
   : mPresContext(aPresContext)
   , mFrame(aFrame)
   , mParentContent(nullptr)
     // XXXldb Why does it make sense to use aParentContent?  (See
     // comment above assertion at start of ElementRestyler::Restyle.)
   , mContent(mFrame->GetContent() ? mFrame->GetContent() : mParentContent)
   , mChangeList(aChangeList)
   , mHintsHandled(NS_SubtractHint(aHintsHandledByAncestors,
                   NS_HintsNotHandledForDescendantsIn(aHintsHandledByAncestors)))
   , mParentFrameHintsNotHandledForDescendants(nsChangeHint(0))
   , mHintsNotHandledForDescendants(nsChangeHint(0))
   , mRestyleTracker(aRestyleTracker)
   , mTreeMatchContext(aTreeMatchContext)
   , mResolvedChild(nullptr)
+  , mContextsToClear(aContextsToClear)
+  , mSwappedStructOwners(aSwappedStructOwners)
 #ifdef ACCESSIBILITY
   , mDesiredA11yNotifications(eSendAllNotifications)
   , mKidsDesiredA11yNotifications(mDesiredA11yNotifications)
   , mOurA11yNotification(eDontNotify)
   , mVisibleKidsOfHiddenElement(aVisibleKidsOfHiddenElement)
 #endif
 #ifdef RESTYLE_LOGGING
   , mLoggingDepth(aRestyleTracker.LoggingDepth() + 1)
@@ -2395,16 +2400,18 @@ ElementRestyler::ElementRestyler(const E
   , mHintsHandled(NS_SubtractHint(aParentRestyler.mHintsHandled,
                   NS_HintsNotHandledForDescendantsIn(aParentRestyler.mHintsHandled)))
   , mParentFrameHintsNotHandledForDescendants(
       aParentRestyler.mHintsNotHandledForDescendants)
   , mHintsNotHandledForDescendants(nsChangeHint(0))
   , mRestyleTracker(aParentRestyler.mRestyleTracker)
   , mTreeMatchContext(aParentRestyler.mTreeMatchContext)
   , mResolvedChild(nullptr)
+  , mContextsToClear(aParentRestyler.mContextsToClear)
+  , mSwappedStructOwners(aParentRestyler.mSwappedStructOwners)
 #ifdef ACCESSIBILITY
   , mDesiredA11yNotifications(aParentRestyler.mKidsDesiredA11yNotifications)
   , mKidsDesiredA11yNotifications(mDesiredA11yNotifications)
   , mOurA11yNotification(eDontNotify)
   , mVisibleKidsOfHiddenElement(aParentRestyler.mVisibleKidsOfHiddenElement)
 #endif
 #ifdef RESTYLE_LOGGING
   , mLoggingDepth(aParentRestyler.mLoggingDepth + 1)
@@ -2439,16 +2446,18 @@ ElementRestyler::ElementRestyler(ParentC
                   NS_HintsNotHandledForDescendantsIn(aParentRestyler.mHintsHandled)))
   , mParentFrameHintsNotHandledForDescendants(
       // assume the worst
       nsChangeHint_Hints_NotHandledForDescendants)
   , mHintsNotHandledForDescendants(nsChangeHint(0))
   , mRestyleTracker(aParentRestyler.mRestyleTracker)
   , mTreeMatchContext(aParentRestyler.mTreeMatchContext)
   , mResolvedChild(nullptr)
+  , mContextsToClear(aParentRestyler.mContextsToClear)
+  , mSwappedStructOwners(aParentRestyler.mSwappedStructOwners)
 #ifdef ACCESSIBILITY
   , mDesiredA11yNotifications(aParentRestyler.mDesiredA11yNotifications)
   , mKidsDesiredA11yNotifications(mDesiredA11yNotifications)
   , mOurA11yNotification(eDontNotify)
   , mVisibleKidsOfHiddenElement(aParentRestyler.mVisibleKidsOfHiddenElement)
 #endif
 #ifdef RESTYLE_LOGGING
   , mLoggingDepth(aParentRestyler.mLoggingDepth + 1)
@@ -2458,29 +2467,34 @@ ElementRestyler::ElementRestyler(ParentC
 
 ElementRestyler::ElementRestyler(nsPresContext* aPresContext,
                                  nsIContent* aContent,
                                  nsStyleChangeList* aChangeList,
                                  nsChangeHint aHintsHandledByAncestors,
                                  RestyleTracker& aRestyleTracker,
                                  TreeMatchContext& aTreeMatchContext,
                                  nsTArray<nsIContent*>&
-                                 aVisibleKidsOfHiddenElement)
+                                   aVisibleKidsOfHiddenElement,
+                                 nsTArray<ContextToClear>& aContextsToClear,
+                                 nsTArray<nsRefPtr<nsStyleContext>>&
+                                   aSwappedStructOwners)
   : mPresContext(aPresContext)
   , mFrame(nullptr)
   , mParentContent(nullptr)
   , mContent(aContent)
   , mChangeList(aChangeList)
   , mHintsHandled(NS_SubtractHint(aHintsHandledByAncestors,
                   NS_HintsNotHandledForDescendantsIn(aHintsHandledByAncestors)))
   , mParentFrameHintsNotHandledForDescendants(nsChangeHint(0))
   , mHintsNotHandledForDescendants(nsChangeHint(0))
   , mRestyleTracker(aRestyleTracker)
   , mTreeMatchContext(aTreeMatchContext)
   , mResolvedChild(nullptr)
+  , mContextsToClear(aContextsToClear)
+  , mSwappedStructOwners(aSwappedStructOwners)
 #ifdef ACCESSIBILITY
   , mDesiredA11yNotifications(eSendAllNotifications)
   , mKidsDesiredA11yNotifications(mDesiredA11yNotifications)
   , mOurA11yNotification(eDontNotify)
   , mVisibleKidsOfHiddenElement(aVisibleKidsOfHiddenElement)
 #endif
 {
 }
@@ -2716,16 +2730,29 @@ ElementRestyler::Restyle(nsRestyleHint a
     nsIFrame* unused;
     nsStyleContext* newParent = mFrame->GetParentStyleContext(&unused);
     if (oldContext->GetParent() != newParent) {
       // If we received eRestyleResult_Stop, then the old style context was
       // left on mFrame.  Since we ended up restyling our parent, change
       // this old style context to point to its new parent.
       LOG_RESTYLE("moving style context %p from old parent %p to new parent %p",
                   oldContext.get(), oldContext->GetParent(), newParent);
+      // We keep strong references to the new parent around until the end
+      // of the restyle, in case:
+      //   (a) we swapped structs between the old and new parent,
+      //   (b) some descendants of the old parent are not getting restyled
+      //       (which is the reason for the existence of
+      //       ClearCachedInheritedStyleDataOnDescendants),
+      //   (d) something under ProcessPendingRestyles (which notably is called
+      //       *before* ClearCachedInheritedStyleDataOnDescendants is called
+      //       on the old context) causes the new parent to be destroyed, thus
+      //       destroying its owned structs, and
+      //   (c) something under ProcessPendingRestyles then wants to use of those
+      //       now destroyed structs (through the old parent's descendants).
+      mSwappedStructOwners.AppendElement(newParent);
       oldContext->MoveTo(newParent);
     }
 
     // Send the accessibility notifications that RestyleChildren otherwise
     // would have sent.
     if (!(mHintsHandled & nsChangeHint_ReconstructFrame)) {
       InitializeAccessibilityNotifications(mFrame->StyleContext());
       SendAccessibilityNotifications();
@@ -2754,27 +2781,39 @@ ElementRestyler::Restyle(nsRestyleHint a
   // context could trigger assertions about mismatched rule trees.
   if (!(mHintsHandled & nsChangeHint_ReconstructFrame)) {
     RestyleChildren(childRestyleHint);
   }
 
   if (oldContext && !oldContext->HasSingleReference()) {
     // If we swapped some structs out of oldContext in the RestyleSelf call
     // and after the RestyleChildren call we still have other strong references
-    // to it, we need to make ensure its descendants don't cached any of the
+    // to it, we need to make ensure its descendants don't cache any of the
     // structs that were swapped out.
     //
-    // Most of the time we will not get in here; we do for example when the
-    // style context is being held on to by an nsComputedDOMStyle object.
+    // Much of the time we will not get in here; we do for example when the
+    // style context is shared with a later IB split sibling (which we won't
+    // restyle until a bit later) or if other code is holding a strong reference
+    // to the style context (as is done by nsTransformedTextRun objects, which
+    // can be referenced by a text frame's mTextRun longer than the frame's
+    // mStyleContext).
     //
-    // Strictly we only have to do this if we have a child whose old or new
-    // style context is shared (as in that case we would not have swapped that
-    // child's structs and it would have kept its now out of date cached
-    // structs).  For now we don't bother tracking that.
-    oldContext->ClearCachedInheritedStyleDataOnDescendants(swappedStructs);
+    // We coalesce entries in mContextsToClear when we detect that the last
+    // style context appended has oldContext as its parent, as
+    // ClearCachedInheritedStyleDataOnDescendants handles a whole subtree
+    // of style contexts.
+    if (!mContextsToClear.IsEmpty() &&
+        mContextsToClear.LastElement().mStyleContext->GetParent() == oldContext &&
+        mContextsToClear.LastElement().mStructs == swappedStructs) {
+      mContextsToClear.LastElement().mStyleContext = Move(oldContext);
+    } else {
+      ContextToClear* toClear = mContextsToClear.AppendElement();
+      toClear->mStyleContext = Move(oldContext);
+      toClear->mStructs = swappedStructs;
+    }
   }
 
   mRestyleTracker.AddRestyleRootsIfAwaitingRestyle(descendants);
 }
 
 /**
  * Depending on the details of the frame we are restyling or its old style
  * context, we may or may not be able to stop restyling after this frame if
@@ -3449,33 +3488,38 @@ ElementRestyler::RestyleChildrenOfDispla
     for ( ; !lists.IsDone(); lists.Next()) {
       nsFrameList::Enumerator childFrames(lists.CurrentList());
       for (; !childFrames.AtEnd(); childFrames.Next()) {
         nsIFrame* f = childFrames.get();
         if (nsContentUtils::ContentIsDescendantOf(f->GetContent(), mContent) &&
             !f->GetPrevContinuation()) {
           if (!(f->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {
             ComputeStyleChangeFor(f, mChangeList, aMinHint, aRestyleTracker,
-                                  aRestyleHint);
+                                  aRestyleHint, mContextsToClear,
+                                  mSwappedStructOwners);
           }
         }
       }
     }
   }
   if (!(mHintsHandled & nsChangeHint_ReconstructFrame)) {
     SendAccessibilityNotifications();
   }
 }
 
 void
 ElementRestyler::ComputeStyleChangeFor(nsIFrame*          aFrame,
                                        nsStyleChangeList* aChangeList,
                                        nsChangeHint       aMinChange,
                                        RestyleTracker&    aRestyleTracker,
-                                       nsRestyleHint      aRestyleHint)
+                                       nsRestyleHint      aRestyleHint,
+                                       nsTArray<ContextToClear>&
+                                         aContextsToClear,
+                                       nsTArray<nsRefPtr<nsStyleContext>>&
+                                         aSwappedStructOwners)
 {
   PROFILER_LABEL("ElementRestyler", "ComputeStyleChangeFor",
     js::ProfileEntry::Category::CSS);
 
   nsIContent* content = aFrame->GetContent();
   if (aMinChange) {
     aChangeList->AppendChange(aFrame, content, aMinChange);
   }
@@ -3513,17 +3557,18 @@ ElementRestyler::ComputeStyleChangeFor(n
         // continuation.
         continue;
       }
 
       // Inner loop over next-in-flows of the current frame
       ElementRestyler restyler(presContext, cont, aChangeList,
                                aMinChange, aRestyleTracker,
                                treeMatchContext,
-                               visibleKidsOfHiddenElement);
+                               visibleKidsOfHiddenElement,
+                               aContextsToClear, aSwappedStructOwners);
 
       restyler.Restyle(aRestyleHint);
 
       if (restyler.HintsHandledForFrame() & nsChangeHint_ReconstructFrame) {
         // If it's going to cause a framechange, then don't bother
         // with the continuations or ib-split siblings since they'll be
         // clobbered by the frame reconstruct anyway.
         NS_ASSERTION(!cont->GetPrevContinuation(),
@@ -3888,33 +3933,57 @@ ElementRestyler::SendAccessibilityNotifi
                                          childContent->GetNextSibling());
       }
       mVisibleKidsOfHiddenElement.Clear();
     }
   }
 #endif
 }
 
+static void
+ClearCachedInheritedStyleDataOnDescendants(
+    nsTArray<ElementRestyler::ContextToClear>& aContextsToClear)
+{
+  for (size_t i = 0; i < aContextsToClear.Length(); i++) {
+    auto& entry = aContextsToClear[i];
+    if (!entry.mStyleContext->HasSingleReference()) {
+      entry.mStyleContext->ClearCachedInheritedStyleDataOnDescendants(
+          entry.mStructs);
+    }
+    entry.mStyleContext = nullptr;
+  }
+}
+
 void
 RestyleManager::ComputeAndProcessStyleChange(nsIFrame*          aFrame,
                                              nsChangeHint       aMinChange,
                                              RestyleTracker&    aRestyleTracker,
                                              nsRestyleHint      aRestyleHint)
 {
   // Create a ReframingStyleContexts struct on the stack and put it in
   // our mReframingStyleContexts for the scope of this function.
   MOZ_ASSERT(!mReframingStyleContexts, "shouldn't call recursively");
   AutoRestore<ReframingStyleContexts*> ar(mReframingStyleContexts);
   ReframingStyleContexts reframingStyleContexts;
   mReframingStyleContexts = &reframingStyleContexts;
 
   nsStyleChangeList changeList;
-  ElementRestyler::ComputeStyleChangeFor(aFrame, &changeList, aMinChange,
-                                         aRestyleTracker, aRestyleHint);
-  ProcessRestyledFrames(changeList);
+  nsTArray<ElementRestyler::ContextToClear> contextsToClear;
+  {
+    // swappedStructOwners needs to be kept alive until after
+    // ProcessRestyledFrames; see comment in ElementRestyler::Restyle.
+    // (Destroying it before the ClearCachedInheritedStyleDataOnDescendants call
+    // helps minimize the work done by that function.)
+    nsTArray<nsRefPtr<nsStyleContext>> swappedStructOwners;
+    ElementRestyler::ComputeStyleChangeFor(aFrame, &changeList, aMinChange,
+                                           aRestyleTracker, aRestyleHint,
+                                           contextsToClear, swappedStructOwners);
+    ProcessRestyledFrames(changeList);
+  }
+  ClearCachedInheritedStyleDataOnDescendants(contextsToClear);
 }
 
 void
 RestyleManager::ComputeAndProcessStyleChange(nsStyleContext*    aNewContext,
                                              Element*           aElement,
                                              nsChangeHint       aMinChange,
                                              RestyleTracker&    aRestyleTracker,
                                              nsRestyleHint      aRestyleHint)
@@ -3933,23 +4002,33 @@ RestyleManager::ComputeAndProcessStyleCh
   TreeMatchContext treeMatchContext(true,
                                     nsRuleWalker::eRelevantLinkUnvisited,
                                     frame->PresContext()->Document());
   nsIContent* parent = aElement->GetParent();
   Element* parentElement =
     parent && parent->IsElement() ? parent->AsElement() : nullptr;
   treeMatchContext.InitAncestors(parentElement);
   nsTArray<nsIContent*> visibleKidsOfHiddenElement;
-  nsStyleChangeList changeList;
-  ElementRestyler r(frame->PresContext(), aElement, &changeList, aMinChange,
-                    aRestyleTracker, treeMatchContext,
-                    visibleKidsOfHiddenElement);
-  r.RestyleChildrenOfDisplayContentsElement(frame, aNewContext, aMinChange,
-                                            aRestyleTracker, aRestyleHint);
-  ProcessRestyledFrames(changeList);
+  nsTArray<ElementRestyler::ContextToClear> contextsToClear;
+  {
+    // swappedStructOwners needs to be kept alive until after
+    // ProcessRestyledFrames; see comment in ElementRestyler::Restyle.
+    // (Destroying it before the ClearCachedInheritedStyleDataOnDescendants call
+    // helps minimize the work done by that function.)
+    nsTArray<nsRefPtr<nsStyleContext>> swappedStructOwners;
+    nsStyleChangeList changeList;
+    ElementRestyler r(frame->PresContext(), aElement, &changeList, aMinChange,
+                      aRestyleTracker, treeMatchContext,
+                      visibleKidsOfHiddenElement, contextsToClear,
+                      swappedStructOwners);
+    r.RestyleChildrenOfDisplayContentsElement(frame, aNewContext, aMinChange,
+                                              aRestyleTracker, aRestyleHint);
+    ProcessRestyledFrames(changeList);
+  }
+  ClearCachedInheritedStyleDataOnDescendants(contextsToClear);
 }
 
 AutoDisplayContentsAncestorPusher::AutoDisplayContentsAncestorPusher(
   TreeMatchContext& aTreeMatchContext, nsPresContext* aPresContext,
   nsIContent* aParent)
   : mTreeMatchContext(aTreeMatchContext)
   , mPresContext(aPresContext)
 {
--- a/layout/base/RestyleManager.h
+++ b/layout/base/RestyleManager.h
@@ -513,24 +513,31 @@ private:
  * An ElementRestyler is created for *each* element in a subtree that we
  * recompute styles for.
  */
 class ElementRestyler MOZ_FINAL
 {
 public:
   typedef mozilla::dom::Element Element;
 
+  struct ContextToClear {
+    nsRefPtr<nsStyleContext> mStyleContext;
+    uint32_t mStructs;
+  };
+
   // Construct for the root of the subtree that we're restyling.
   ElementRestyler(nsPresContext* aPresContext,
                   nsIFrame* aFrame,
                   nsStyleChangeList* aChangeList,
                   nsChangeHint aHintsHandledByAncestors,
                   RestyleTracker& aRestyleTracker,
                   TreeMatchContext& aTreeMatchContext,
-                  nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement);
+                  nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement,
+                  nsTArray<ContextToClear>& aContextsToClear,
+                  nsTArray<nsRefPtr<nsStyleContext>>& aSwappedStructOwners);
 
   // Construct for an element whose parent is being restyled.
   enum ConstructorFlags {
     FOR_OUT_OF_FLOW_CHILD = 1<<0
   };
   ElementRestyler(const ElementRestyler& aParentRestyler,
                   nsIFrame* aFrame,
                   uint32_t aConstructorFlags);
@@ -548,17 +555,19 @@ public:
 
   // For restyling undisplayed content only (mFrame==null).
   ElementRestyler(nsPresContext* aPresContext,
                   nsIContent* aContent,
                   nsStyleChangeList* aChangeList,
                   nsChangeHint aHintsHandledByAncestors,
                   RestyleTracker& aRestyleTracker,
                   TreeMatchContext& aTreeMatchContext,
-                  nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement);
+                  nsTArray<nsIContent*>& aVisibleKidsOfHiddenElement,
+                  nsTArray<ContextToClear>& aContextsToClear,
+                  nsTArray<nsRefPtr<nsStyleContext>>& aSwappedStructOwners);
 
   /**
    * Restyle our frame's element and its subtree.
    *
    * Use eRestyle_Self for the aRestyleHint argument to mean
    * "reresolve our style context but not kids", use eRestyle_Subtree
    * to mean "reresolve our style context and kids", and use
    * nsRestyleHint(0) to mean recompute a new style context for our
@@ -588,17 +597,20 @@ public:
   /**
    * Re-resolve the style contexts for a frame tree, building aChangeList
    * based on the resulting style changes, plus aMinChange applied to aFrame.
    */
   static void ComputeStyleChangeFor(nsIFrame*          aFrame,
                                     nsStyleChangeList* aChangeList,
                                     nsChangeHint       aMinChange,
                                     RestyleTracker&    aRestyleTracker,
-                                    nsRestyleHint      aRestyleHint);
+                                    nsRestyleHint      aRestyleHint,
+                                    nsTArray<ContextToClear>& aContextsToClear,
+                                    nsTArray<nsRefPtr<nsStyleContext>>&
+                                      aSwappedStructOwners);
 
 #ifdef RESTYLE_LOGGING
   bool ShouldLogRestyle() {
     return RestyleManager::ShouldLogRestyle(mPresContext);
   }
 #endif
 
 private:
@@ -717,16 +729,24 @@ private:
   // style comparision returns a hint other than one of these hints.
   nsChangeHint mHintsHandled;
   // See nsStyleContext::CalcStyleDifference
   nsChangeHint mParentFrameHintsNotHandledForDescendants;
   nsChangeHint mHintsNotHandledForDescendants;
   RestyleTracker& mRestyleTracker;
   TreeMatchContext& mTreeMatchContext;
   nsIFrame* mResolvedChild; // child that provides our parent style context
+  // Array of style context subtrees in which we need to clear out cached
+  // structs at the end of the restyle (after change hints have been
+  // processed).
+  nsTArray<ContextToClear>& mContextsToClear;
+  // Style contexts that had old structs swapped into it and which should
+  // stay alive until the end of the restyle.  (See comment in
+  // ElementRestyler::Restyle.)
+  nsTArray<nsRefPtr<nsStyleContext>>& mSwappedStructOwners;
 
 #ifdef ACCESSIBILITY
   const DesiredA11yNotifications mDesiredA11yNotifications;
   DesiredA11yNotifications mKidsDesiredA11yNotifications;
   A11yNotificationType mOurA11yNotification;
   nsTArray<nsIContent*>& mVisibleKidsOfHiddenElement;
   bool mWasFrameVisible;
 #endif