Bug 243519. Fix the crashtest so that it doesn't bomb out with a JS error without completing the actual test. And make it (and Acid3) actually pass without screwing up the frame tree.
authorRobert O'Callahan <robert@ocallahan.org>
Sat, 06 Sep 2008 22:21:37 +1200
changeset 18887 14ce7619e9c186286e15419bb7800b3fd5c3e34f
parent 18886 cc98ae28c215e415f19ff4151a52386e40124c1c
child 18888 75c0f6a369410cc4ea6f3e1446cb650b73cc4e03
push id1797
push userrocallahan@mozilla.com
push dateSat, 06 Sep 2008 10:21:49 +0000
treeherdermozilla-central@14ce7619e9c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs243519
milestone1.9.1b1pre
Bug 243519. Fix the crashtest so that it doesn't bomb out with a JS error without completing the actual test. And make it (and Acid3) actually pass without screwing up the frame tree.
layout/base/crashtests/243519-1.html
layout/base/nsCSSFrameConstructor.cpp
layout/base/nsCSSFrameConstructor.h
--- a/layout/base/crashtests/243519-1.html
+++ b/layout/base/crashtests/243519-1.html
@@ -2,26 +2,29 @@
 <html>
 <body>
   <div style="position:absolute;">Hello</div>
   <div style="position:fixed;">Kitty</div>
   <script>
     document.body.offsetTop;
     document.documentElement.style.display = "table";
     document.body.offsetTop;
-    document.documentElement..style.display = "";
+    document.documentElement.style.display = "";
     document.body.offsetTop;
 
     document.documentElement.style.position = "absolute";
     document.body.offsetTop;
     document.documentElement.style.display = "table";
     document.body.offsetTop;
-    document.documentElement..style.display = "";
+    document.documentElement.style.display = "";
     document.body.offsetTop;
 
     document.documentElement.style.position = "fixed";
     document.body.offsetTop;
     document.documentElement.style.display = "table";
     document.body.offsetTop;
-    document.documentElement..style.display = "";
+    document.documentElement.style.display = "";
+    
+    document.documentElement.style.position = "";
+    document.body.offsetTop;
   </script>
 </body>
 </html>
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -7558,37 +7558,55 @@ nsCSSFrameConstructor::ReconstructDocEle
         // rooted at docElementFrame
         ::DeletingFrameSubtree(state.mFrameManager, docElementFrame);
       }
 
       // Remove any existing fixed items: they are always on the
       // FixedContainingBlock.  Note that this has to be done before we call
       // ClearPlaceholderFrameMap(), since RemoveFixedItems uses the
       // placeholder frame map.
-      rv = RemoveFixedItems(state);
+      rv = RemoveFixedItems(state, docElementFrame);
+
       if (NS_SUCCEEDED(rv)) {
+        nsPlaceholderFrame* placeholderFrame = nsnull;
+        if (docElementFrame &&
+            (docElementFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {
+          // Get the placeholder frame now, before we tear down the
+          // placeholder frame map
+          placeholderFrame =
+            state.mFrameManager->GetPlaceholderFrameFor(docElementFrame);
+          NS_ASSERTION(placeholderFrame, "No placeholder for out-of-flow?");
+        }
+
         // Clear the hash tables that map from content to frame and out-of-flow
         // frame to placeholder frame
         state.mFrameManager->ClearPrimaryFrameMap();
         state.mFrameManager->ClearPlaceholderFrameMap();
         state.mFrameManager->ClearUndisplayedContentMap();
 
         if (docElementFrame) {
           // Take the docElementFrame, and remove it from its parent.
-        
           // XXXbz So why can't we reuse ContentRemoved?
 
           // Notify self that we will destroy the entire frame tree, this blocks
           // RemoveMappingsForFrameSubtree() which would otherwise lead to a
           // crash since we cleared the placeholder map above (bug 398982).
           PRBool wasDestroyingFrameTree = mIsDestroyingFrameTree;
           WillDestroyFrameTree();
-          // Remove the old document element hierarchy
+
           rv = state.mFrameManager->RemoveFrame(docElementFrame->GetParent(),
                     GetChildListNameFor(docElementFrame), docElementFrame);
+          
+          if (placeholderFrame) {
+            // Remove the placeholder frame first (XXX second for now) (so
+            // that it doesn't retain a dangling pointer to memory)
+            rv |= state.mFrameManager->RemoveFrame(placeholderFrame->GetParent(),
+                                            nsnull, placeholderFrame);
+          }
+
           mIsDestroyingFrameTree = wasDestroyingFrameTree;
           if (NS_FAILED(rv)) {
             return rv;
           }
         }
         
         mInitialContainingBlock = nsnull;
         mRootElementStyleFrame = nsnull;
@@ -12833,24 +12851,32 @@ nsCSSFrameConstructor::ReframeContaining
       }
     }
   }
 
   // If we get here, we're screwed!
   return ReconstructDocElementHierarchyInternal();
 }
 
-nsresult nsCSSFrameConstructor::RemoveFixedItems(const nsFrameConstructorState& aState)
+nsresult
+nsCSSFrameConstructor::RemoveFixedItems(const nsFrameConstructorState& aState,
+                                        nsIFrame *aRootElementFrame)
 {
   nsresult rv=NS_OK;
 
   if (mFixedContainingBlock) {
     nsIFrame *fixedChild = nsnull;
     do {
       fixedChild = mFixedContainingBlock->GetFirstChild(nsGkAtoms::fixedList);
+      if (fixedChild == aRootElementFrame) {
+        // Skip the root element frame, if it happens to be fixed-positioned
+        // It will be explicitly removed later in
+        // ReconstructDocElementHierarchyInternal
+        fixedChild = fixedChild->GetNextSibling();
+      }
       if (fixedChild) {
         // Remove the placeholder so it doesn't end up sitting about pointing
         // to the removed fixed frame.
         nsPlaceholderFrame *placeholderFrame =
           aState.mFrameManager->GetPlaceholderFrameFor(fixedChild);
         NS_ASSERTION(placeholderFrame, "no placeholder for fixed-pos frame");
         NS_ASSERTION(placeholderFrame->GetType() ==
                      nsGkAtoms::placeholderFrame,
--- a/layout/base/nsCSSFrameConstructor.h
+++ b/layout/base/nsCSSFrameConstructor.h
@@ -1044,17 +1044,18 @@ private:
 
   nsresult InsertFirstLineFrames(nsFrameConstructorState& aState,
                                  nsIContent*              aContent,
                                  nsIFrame*                aBlockFrame,
                                  nsIFrame**               aParentFrame,
                                  nsIFrame*                aPrevSibling,
                                  nsFrameItems&            aFrameItems);
 
-  nsresult RemoveFixedItems(const nsFrameConstructorState& aState);
+  nsresult RemoveFixedItems(const nsFrameConstructorState& aState,
+                            nsIFrame*                      aRootElementFrame);
 
   // Find the right frame to use for aContent when looking for sibling
   // frames for aTargetContent.  If aPrevSibling is true, this
   // will look for last continuations, etc, as necessary.  This calls
   // IsValidSibling as needed; if that returns false it returns null.
   //
   // @param aTargetContentDisplay the CSS display enum for aTargetContent if
   // already known, UNSET_DISPLAY otherwise.