Bug 1127198 - Part 4: Clear cached structs only after we have fully processed a restyle. r=dbaron
authorCameron McCormack <cam@mcc.id.au>
Wed, 18 Feb 2015 09:28:53 +1100
changeset 229650 60d2f893feb93f77cb19b6fb43dd9e9afae2b518
parent 229649 1ac22d8f07fadcccb802def741dac4395fcced50
child 229651 f20cf9991555d47e8f6af2f46d2750c2a3ace079
push id28294
push userryanvm@gmail.com
push dateThu, 19 Feb 2015 01:30:38 +0000
treeherdermozilla-central@360b5f211180 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs1127198
milestone38.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1127198 - Part 4: Clear cached structs only after we have fully processed a restyle. r=dbaron
layout/base/RestyleManager.cpp
layout/base/RestyleManager.h
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -2430,31 +2430,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)
@@ -2475,16 +2480,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)
@@ -2519,16 +2526,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)
@@ -2538,29 +2547,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
 {
 }
@@ -2799,16 +2813,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();
@@ -2837,27 +2864,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
@@ -3526,33 +3565,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);
   }
@@ -3590,17 +3634,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(),
@@ -3965,27 +4010,51 @@ 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)
 {
   MOZ_ASSERT(mReframingStyleContexts, "should have rsc");
   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)
@@ -3998,23 +4067,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
@@ -494,24 +494,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);
@@ -529,17 +536,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
@@ -569,17 +578,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:
@@ -698,16 +710,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