Bug 786740. When destroying all the frames in a line list, keep the line list and frame list valid at each step in case someone tries to walk the frame tree during frame destruction. r=mats
authorRobert O'Callahan <robert@ocallahan.org>
Mon, 15 Oct 2012 14:34:23 +1300
changeset 110383 9edabc0ddc99d86d18378cdccf9c7fc0bb3b34e4
parent 110382 65652fcb58dcb1417f6cefc7191ce54a6676639d
child 110384 fa3a84d645fc4a44ee064e6397f334ef54aee1e1
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersmats
bugs786740
milestone19.0a1
Bug 786740. When destroying all the frames in a line list, keep the line list and frame list valid at each step in case someone tries to walk the frame tree during frame destruction. r=mats
layout/generic/crashtests/786740-1.html
layout/generic/crashtests/crashtests.list
layout/generic/nsBlockFrame.cpp
layout/generic/nsLineBox.cpp
layout/generic/nsLineBox.h
new file mode 100644
--- /dev/null
+++ b/layout/generic/crashtests/786740-1.html
@@ -0,0 +1,31 @@
+<!DOCTYPE HTML>
+<html class="reftest-wait">
+<head>
+<style>
+#d {
+  transition:opacity 1s;
+}
+#p {
+  position:absolute;
+}
+</style>
+</head>
+<body>
+<div id="d">
+  Hello
+  <span id="s"><div id="p">Kitty</div></span>
+</div>
+<script>
+var d = document.getElementById("d");
+d.getBoundingClientRect();
+d.style.opacity = 0.3;
+window.addEventListener("MozReftestInvalidate",
+  function() {
+    setTimeout(function() {
+      document.body.removeChild(d);
+      document.documentElement.removeAttribute("class");
+    }, 50);
+  }, false);
+</script>
+</body>
+</html>
--- a/layout/generic/crashtests/crashtests.list
+++ b/layout/generic/crashtests/crashtests.list
@@ -387,8 +387,9 @@ load text-overflow-bug713610.html
 load 700031.xhtml
 load 718516.html
 load first-letter-638937.html
 load first-letter-638937-2.html
 load 737313-1.html
 load 737313-2.html
 load 737313-3.html
 load 762764-1.html
+load 786740-1.html
--- a/layout/generic/nsBlockFrame.cpp
+++ b/layout/generic/nsBlockFrame.cpp
@@ -274,31 +274,29 @@ nsBlockFrame::~nsBlockFrame()
 
 void
 nsBlockFrame::DestroyFrom(nsIFrame* aDestructRoot)
 {
   ClearLineCursor();
   DestroyAbsoluteFrames(aDestructRoot);
   mFloats.DestroyFramesFrom(aDestructRoot);
   nsPresContext* presContext = PresContext();
-  nsLineBox::DeleteLineList(presContext, mLines, aDestructRoot);
-
-  // Now clear mFrames, since we've destroyed all the frames in it.
-  mFrames.Clear();
+  nsLineBox::DeleteLineList(presContext, mLines, aDestructRoot,
+                            &mFrames);
 
   nsFrameList* pushedFloats = RemovePushedFloats();
   if (pushedFloats) {
     pushedFloats->DestroyFrom(aDestructRoot);
   }
 
   // destroy overflow lines now
   FrameLines* overflowLines = RemoveOverflowLines();
   if (overflowLines) {
     nsLineBox::DeleteLineList(presContext, overflowLines->mLines,
-                              aDestructRoot);
+                              aDestructRoot, &overflowLines->mFrames);
     delete overflowLines;
   }
 
   {
     nsAutoOOFFrameList oofs(this);
     oofs.mList.DestroyFramesFrom(aDestructRoot);
     // oofs is now empty and will remove the frame list property
   }
--- a/layout/generic/nsLineBox.cpp
+++ b/layout/generic/nsLineBox.cpp
@@ -348,48 +348,38 @@ nsLineBox::CachedIsEmpty()
 
   mFlags.mEmptyCacheValid = true;
   mFlags.mEmptyCacheState = result;
   return result;
 }
 
 void
 nsLineBox::DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines,
-                          nsIFrame* aDestructRoot)
+                          nsIFrame* aDestructRoot, nsFrameList* aFrames)
 {
-  if (! aLines.empty()) {
-    // Delete our child frames before doing anything else. In particular
-    // we do all of this before our base class releases it's hold on the
-    // view.
-#ifdef DEBUG
-    int32_t numFrames = 0;
-#endif
-    for (nsIFrame* child = aLines.front()->mFirstChild; child; ) {
-      nsIFrame* nextChild = child->GetNextSibling();
-      child->SetNextSibling(nullptr);
-      child->DestroyFrom((aDestructRoot) ? aDestructRoot : child);
-      child = nextChild;
-#ifdef DEBUG
-      numFrames++;
-#endif
+  nsIPresShell* shell = aPresContext->PresShell();
+
+  // Keep our line list and frame list up to date as we
+  // remove frames, in case something wants to traverse the
+  // frame tree while we're destroying.
+  while (!aLines.empty()) {
+    nsLineBox* line = aLines.front();
+    if (NS_UNLIKELY(line->mFlags.mHasHashedFrames)) {
+      line->SwitchToCounter();  // Avoid expensive has table removals.
+    }
+    while (line->GetChildCount() > 0) {
+      nsIFrame* child = aFrames->RemoveFirstChild();
+      MOZ_ASSERT(child == line->mFirstChild, "Lines out of sync");
+      line->mFirstChild = aFrames->FirstChild();
+      line->NoteFrameRemoved(child);
+      child->DestroyFrom(aDestructRoot);
     }
 
-    nsIPresShell *shell = aPresContext->PresShell();
-
-    do {
-      nsLineBox* line = aLines.front();
-#ifdef DEBUG
-      numFrames -= line->GetChildCount();
-#endif
-      aLines.pop_front();
-      line->Destroy(shell);
-    } while (! aLines.empty());
-#ifdef DEBUG
-    NS_ASSERTION(numFrames == 0, "number of frames deleted does not match");
-#endif
+    aLines.pop_front();
+    line->Destroy(shell);
   }
 }
 
 bool
 nsLineBox::RFindLineContaining(nsIFrame* aFrame,
                                const nsLineList::iterator& aBegin,
                                nsLineList::iterator& aEnd,
                                nsIFrame* aLastFrameBeforeEnd,
--- a/layout/generic/nsLineBox.h
+++ b/layout/generic/nsLineBox.h
@@ -479,17 +479,17 @@ public:
   nscoord GetAscent() const { return mAscent; }
   void SetAscent(nscoord aAscent) { mAscent = aAscent; }
 
   nscoord GetHeight() const {
     return mBounds.height;
   }
 
   static void DeleteLineList(nsPresContext* aPresContext, nsLineList& aLines,
-                             nsIFrame* aDestructRoot);
+                             nsIFrame* aDestructRoot, nsFrameList* aFrames);
 
   // search from end to beginning of [aBegin, aEnd)
   // Returns true if it found the line and false if not.
   // Moves aEnd as it searches so that aEnd points to the resulting line.
   // aLastFrameBeforeEnd is the last frame before aEnd (so if aEnd is
   // the end of the line list, it's just the last frame in the frame
   // list).
   static bool RFindLineContaining(nsIFrame* aFrame,