Bug 1110277 patch 6 - Make the lifetime of the ReframingStyleContexts object longer. r=heycam
authorL. David Baron <dbaron@dbaron.org>
Tue, 13 Jan 2015 21:03:13 -0800
changeset 250821 30666c55966ead13b7093be8be56820b3f9cfe76
parent 250820 b232139eb2c79e32980e40d85315e524c41e1bc1
child 250822 2a1f0e8d1fc923bc51ac43e027584bc3a19a8259
push id4610
push userjlund@mozilla.com
push dateMon, 30 Mar 2015 18:32:55 +0000
treeherdermozilla-beta@4df54044d9ef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1110277, 1115812, 1111451
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 1110277 patch 6 - Make the lifetime of the ReframingStyleContexts object longer. r=heycam This makes the ReframingStyleContexts live across the lifetime of the processing of a full queue of posted restyles. This depends on bug 1115812 to behave sensibly (and not assert) when rebuilding the rule tree (RebuildAllStyleData, etc.). This handles the form of lazy frame construction that is done in nsCSSFrameConstructor::RecreateFramesForContent, which posts a restyle. Patch 7 handles any use of the lazy frame construction mechanism. This patch (with patches 4 and 5 under it, but without patches 1-3) fixes the original testcase in bug 1110277, except for some flashing of the final position as the transition starts. Also fixes bug 1111451.
layout/base/RestyleManager.cpp
layout/base/RestyleTracker.cpp
--- a/layout/base/RestyleManager.cpp
+++ b/layout/base/RestyleManager.cpp
@@ -924,16 +924,17 @@ RestyleManager::ProcessRestyledFrames(ns
 
 void
 RestyleManager::RestyleElement(Element*        aElement,
                                nsIFrame*       aPrimaryFrame,
                                nsChangeHint    aMinHint,
                                RestyleTracker& aRestyleTracker,
                                nsRestyleHint   aRestyleHint)
 {
+  MOZ_ASSERT(mReframingStyleContexts, "should have rsc");
   NS_ASSERTION(aPrimaryFrame == aElement->GetPrimaryFrame(),
                "frame/content mismatch");
   if (aPrimaryFrame && aPrimaryFrame->GetContent() != aElement) {
     // XXXbz this is due to image maps messing with the primary frame pointer
     // of <area>s.  See bug 135040.  We can remove this block once that's fixed.
     aPrimaryFrame = nullptr;
   }
   NS_ASSERTION(!aPrimaryFrame || aPrimaryFrame->GetContent() == aElement,
@@ -3909,37 +3910,31 @@ ElementRestyler::SendAccessibilityNotifi
 }
 
 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.
-  ReframingStyleContexts reframingStyleContexts(this);
-
+  MOZ_ASSERT(mReframingStyleContexts, "should have rsc");
   nsStyleChangeList changeList;
   ElementRestyler::ComputeStyleChangeFor(aFrame, &changeList, aMinChange,
                                          aRestyleTracker, aRestyleHint);
   ProcessRestyledFrames(changeList);
 }
 
 void
 RestyleManager::ComputeAndProcessStyleChange(nsStyleContext*    aNewContext,
                                              Element*           aElement,
                                              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.
-  ReframingStyleContexts reframingStyleContexts(this);
-
+  MOZ_ASSERT(mReframingStyleContexts, "should have rsc");
   MOZ_ASSERT(aNewContext->StyleDisplay()->mDisplay == NS_STYLE_DISPLAY_CONTENTS);
   nsIFrame* frame = GetNearestAncestorFrame(aElement);
   MOZ_ASSERT(frame, "display:contents node in map although it's a "
                     "display:none descendant?");
   TreeMatchContext treeMatchContext(true,
                                     nsRuleWalker::eRelevantLinkUnvisited,
                                     frame->PresContext()->Document());
   nsIContent* parent = aElement->GetParent();
--- a/layout/base/RestyleTracker.cpp
+++ b/layout/base/RestyleTracker.cpp
@@ -190,140 +190,163 @@ RestyleTracker::ProcessOneRestyle(Elemen
 }
 
 void
 RestyleTracker::DoProcessRestyles()
 {
   PROFILER_LABEL("RestyleTracker", "ProcessRestyles",
     js::ProfileEntry::Category::CSS);
 
-  mRestyleManager->BeginProcessingRestyles(*this);
+  // Create a ReframingStyleContexts struct on the stack and put it in our
+  // mReframingStyleContexts for almost all of the remaining scope of
+  // this function.
+  //
+  // It needs to be *in* scope during BeginProcessingRestyles, which
+  // might (if mDoRebuildAllStyleData is true) do substantial amounts of
+  // restyle processing.
+  //
+  // However, it needs to be *out* of scope during
+  // EndProcessingRestyles, since we should release the style contexts
+  // it holds prior to any EndReconstruct call that
+  // EndProcessingRestyles makes.  This is because in EndReconstruct we
+  // try to destroy the old rule tree using the GC mechanism, which
+  // means it only gets destroyed if it's unreferenced (and if it's
+  // referenced, we assert).  So we want the ReframingStyleContexts
+  // (which holds old style contexts) to be destroyed before the
+  // EndReconstruct so those style contexts go away before
+  // EndReconstruct.
+  {
+    RestyleManager::ReframingStyleContexts
+      reframingStyleContexts(mRestyleManager);
 
-  LOG_RESTYLE("Processing %d pending %srestyles with %d restyle roots for %s",
-              mPendingRestyles.Count(),
-              mRestyleManager->IsProcessingAnimationStyleChange()
-                ? (const char*) "animation " : (const char*) "",
-              static_cast<int>(mRestyleRoots.Length()),
-              GetDocumentURI(Document()).get());
-  LOG_RESTYLE_INDENT();
+    mRestyleManager->BeginProcessingRestyles(*this);
 
-  // loop so that we process any restyle events generated by processing
-  while (mPendingRestyles.Count()) {
-    if (mHaveLaterSiblingRestyles) {
-      // Convert them to individual restyles on all the later siblings
-      nsAutoTArray<nsRefPtr<Element>, RESTYLE_ARRAY_STACKSIZE> laterSiblingArr;
-      LaterSiblingCollector siblingCollector = { this, &laterSiblingArr };
-      mPendingRestyles.Enumerate(CollectLaterSiblings, &siblingCollector);
-      for (uint32_t i = 0; i < laterSiblingArr.Length(); ++i) {
-        Element* element = laterSiblingArr[i];
-        for (nsIContent* sibling = element->GetNextSibling();
-             sibling;
-             sibling = sibling->GetNextSibling()) {
-          if (sibling->IsElement()) {
-            LOG_RESTYLE("adding pending restyle for %s due to "
-                        "eRestyle_LaterSiblings hint on %s",
-                        FrameTagToString(sibling->AsElement()).get(),
-                        FrameTagToString(element->AsElement()).get());
-            if (AddPendingRestyle(sibling->AsElement(), eRestyle_Subtree,
-                                  NS_STYLE_HINT_NONE)) {
-                // Nothing else to do here; we'll handle the following
-                // siblings when we get to |sibling| in laterSiblingArr.
-              break;
+    LOG_RESTYLE("Processing %d pending %srestyles with %d restyle roots for %s",
+                mPendingRestyles.Count(),
+                mRestyleManager->IsProcessingAnimationStyleChange()
+                  ? (const char*) "animation " : (const char*) "",
+                static_cast<int>(mRestyleRoots.Length()),
+                GetDocumentURI(Document()).get());
+    LOG_RESTYLE_INDENT();
+
+    // loop so that we process any restyle events generated by processing
+    while (mPendingRestyles.Count()) {
+      if (mHaveLaterSiblingRestyles) {
+        // Convert them to individual restyles on all the later siblings
+        nsAutoTArray<nsRefPtr<Element>, RESTYLE_ARRAY_STACKSIZE> laterSiblingArr;
+        LaterSiblingCollector siblingCollector = { this, &laterSiblingArr };
+        mPendingRestyles.Enumerate(CollectLaterSiblings, &siblingCollector);
+        for (uint32_t i = 0; i < laterSiblingArr.Length(); ++i) {
+          Element* element = laterSiblingArr[i];
+          for (nsIContent* sibling = element->GetNextSibling();
+               sibling;
+               sibling = sibling->GetNextSibling()) {
+            if (sibling->IsElement()) {
+              LOG_RESTYLE("adding pending restyle for %s due to "
+                          "eRestyle_LaterSiblings hint on %s",
+                          FrameTagToString(sibling->AsElement()).get(),
+                          FrameTagToString(element->AsElement()).get());
+              if (AddPendingRestyle(sibling->AsElement(), eRestyle_Subtree,
+                                    NS_STYLE_HINT_NONE)) {
+                  // Nothing else to do here; we'll handle the following
+                  // siblings when we get to |sibling| in laterSiblingArr.
+                break;
+              }
             }
           }
         }
-      }
 
-      // Now remove all those eRestyle_LaterSiblings bits
-      for (uint32_t i = 0; i < laterSiblingArr.Length(); ++i) {
-        Element* element = laterSiblingArr[i];
-        NS_ASSERTION(element->HasFlag(RestyleBit()), "How did that happen?");
-        RestyleData* data;
+        // Now remove all those eRestyle_LaterSiblings bits
+        for (uint32_t i = 0; i < laterSiblingArr.Length(); ++i) {
+          Element* element = laterSiblingArr[i];
+          NS_ASSERTION(element->HasFlag(RestyleBit()), "How did that happen?");
+          RestyleData* data;
 #ifdef DEBUG
-        bool found =
+          bool found =
 #endif
-          mPendingRestyles.Get(element, &data);
-        NS_ASSERTION(found, "Where did our entry go?");
-        data->mRestyleHint =
-          nsRestyleHint(data->mRestyleHint & ~eRestyle_LaterSiblings);
+            mPendingRestyles.Get(element, &data);
+          NS_ASSERTION(found, "Where did our entry go?");
+          data->mRestyleHint =
+            nsRestyleHint(data->mRestyleHint & ~eRestyle_LaterSiblings);
+        }
+
+        LOG_RESTYLE("%d pending restyles after expanding out "
+                    "eRestyle_LaterSiblings", mPendingRestyles.Count());
+
+        mHaveLaterSiblingRestyles = false;
       }
 
-      LOG_RESTYLE("%d pending restyles after expanding out "
-                  "eRestyle_LaterSiblings", mPendingRestyles.Count());
+      uint32_t rootCount;
+      while ((rootCount = mRestyleRoots.Length())) {
+        // Make sure to pop the element off our restyle root array, so
+        // that we can freely append to the array as we process this
+        // element.
+        nsRefPtr<Element> element;
+        element.swap(mRestyleRoots[rootCount - 1]);
+        mRestyleRoots.RemoveElementAt(rootCount - 1);
 
-      mHaveLaterSiblingRestyles = false;
-    }
+        LOG_RESTYLE("processing style root %s at index %d",
+                    FrameTagToString(element).get(), rootCount - 1);
+        LOG_RESTYLE_INDENT();
 
-    uint32_t rootCount;
-    while ((rootCount = mRestyleRoots.Length())) {
-      // Make sure to pop the element off our restyle root array, so
-      // that we can freely append to the array as we process this
-      // element.
-      nsRefPtr<Element> element;
-      element.swap(mRestyleRoots[rootCount - 1]);
-      mRestyleRoots.RemoveElementAt(rootCount - 1);
+        // Do the document check before calling GetRestyleData, since we
+        // don't want to do the sibling-processing GetRestyleData does if
+        // the node is no longer relevant.
+        if (element->GetCrossShadowCurrentDoc() != Document()) {
+          // Content node has been removed from our document; nothing else
+          // to do here
+          LOG_RESTYLE("skipping, no longer in the document");
+          continue;
+        }
 
-      LOG_RESTYLE("processing style root %s at index %d",
-                  FrameTagToString(element).get(), rootCount - 1);
-      LOG_RESTYLE_INDENT();
+        nsAutoPtr<RestyleData> data;
+        if (!GetRestyleData(element, data)) {
+          LOG_RESTYLE("skipping, already restyled");
+          continue;
+        }
 
-      // Do the document check before calling GetRestyleData, since we
-      // don't want to do the sibling-processing GetRestyleData does if
-      // the node is no longer relevant.
-      if (element->GetCrossShadowCurrentDoc() != Document()) {
-        // Content node has been removed from our document; nothing else
-        // to do here
-        LOG_RESTYLE("skipping, no longer in the document");
+        ProcessOneRestyle(element, data->mRestyleHint, data->mChangeHint);
+        AddRestyleRootsIfAwaitingRestyle(data->mDescendants);
+      }
+
+      if (mHaveLaterSiblingRestyles) {
+        // Keep processing restyles for now
         continue;
       }
 
-      nsAutoPtr<RestyleData> data;
-      if (!GetRestyleData(element, data)) {
-        LOG_RESTYLE("skipping, already restyled");
-        continue;
-      }
-
-      ProcessOneRestyle(element, data->mRestyleHint, data->mChangeHint);
-      AddRestyleRootsIfAwaitingRestyle(data->mDescendants);
-    }
-
-    if (mHaveLaterSiblingRestyles) {
-      // Keep processing restyles for now
-      continue;
-    }
+      // Now we only have entries with change hints left.  To be safe in
+      // case of reentry from the handing of the change hint, use a
+      // scratch array instead of calling out to ProcessOneRestyle while
+      // enumerating the hashtable.  Use the stack if we can, otherwise
+      // fall back on heap-allocation.
+      nsAutoTArray<RestyleEnumerateData, RESTYLE_ARRAY_STACKSIZE> restyleArr;
+      RestyleEnumerateData* restylesToProcess =
+        restyleArr.AppendElements(mPendingRestyles.Count());
+      if (restylesToProcess) {
+        RestyleEnumerateData* lastRestyle = restylesToProcess;
+        RestyleCollector collector = { this, &lastRestyle };
+        mPendingRestyles.Enumerate(CollectRestyles, &collector);
 
-    // Now we only have entries with change hints left.  To be safe in
-    // case of reentry from the handing of the change hint, use a
-    // scratch array instead of calling out to ProcessOneRestyle while
-    // enumerating the hashtable.  Use the stack if we can, otherwise
-    // fall back on heap-allocation.
-    nsAutoTArray<RestyleEnumerateData, RESTYLE_ARRAY_STACKSIZE> restyleArr;
-    RestyleEnumerateData* restylesToProcess =
-      restyleArr.AppendElements(mPendingRestyles.Count());
-    if (restylesToProcess) {
-      RestyleEnumerateData* lastRestyle = restylesToProcess;
-      RestyleCollector collector = { this, &lastRestyle };
-      mPendingRestyles.Enumerate(CollectRestyles, &collector);
-
-      // Clear the hashtable now that we don't need it anymore
-      mPendingRestyles.Clear();
+        // Clear the hashtable now that we don't need it anymore
+        mPendingRestyles.Clear();
 
 #ifdef RESTYLE_LOGGING
-      uint32_t index = 0;
+        uint32_t index = 0;
 #endif
-      for (RestyleEnumerateData* currentRestyle = restylesToProcess;
-           currentRestyle != lastRestyle;
-           ++currentRestyle) {
-        LOG_RESTYLE("processing pending restyle %s at index %d/%d",
-                    FrameTagToString(currentRestyle->mElement).get(),
-                    index++, collector.count);
-        LOG_RESTYLE_INDENT();
-        ProcessOneRestyle(currentRestyle->mElement,
-                          currentRestyle->mRestyleHint,
-                          currentRestyle->mChangeHint);
+        for (RestyleEnumerateData* currentRestyle = restylesToProcess;
+             currentRestyle != lastRestyle;
+             ++currentRestyle) {
+          LOG_RESTYLE("processing pending restyle %s at index %d/%d",
+                      FrameTagToString(currentRestyle->mElement).get(),
+                      index++, collector.count);
+          LOG_RESTYLE_INDENT();
+          ProcessOneRestyle(currentRestyle->mElement,
+                            currentRestyle->mRestyleHint,
+                            currentRestyle->mChangeHint);
+        }
       }
     }
   }
 
   mRestyleManager->EndProcessingRestyles();
 }
 
 bool