Changeset 175641 in webkit

Timestamp:

Nov 5, 2014 1:26:28 PM (9 years ago)

Author:

hyatt@apple.com

Message:

Descendant ends up in wrong flow thread with nested columns and spans.
​https://bugs.webkit.org/show_bug.cgi?id=137273

Reviewed by Simon Fraser.

Unskipped the two problematic span tests.

Source/WebCore:

• rendering/RenderMultiColumnFlowThread.cpp:

(WebCore::isValidColumnSpanner):
Remove the guard and comment and added the assertion back in.

(WebCore::RenderMultiColumnFlowThread::flowThreadDescendantInserted):
Changed to no longer use handleSpannerRemoval. Because the spanner was removed from the flow thread's map,
handleSpannerRemoval was a no-op. So instead I just removed the placeholder by hand.

The second fix was to stop destroying the placeholder. Since the placeholder can just have been inserted, you
can't delete it, since otherwise code further up the stack will access the deleted object. For now, we just
leak the placeholder.

The third fix is to make sure the subtreeRoot is properly updated to be the new placeholder.

LayoutTests:

• TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (2 diffs)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2014-11-04  David Hyatt  <hyatt@apple.com>
+
+        Descendant ends up in wrong flow thread with nested columns and spans.
+        https://bugs.webkit.org/show_bug.cgi?id=137273
+
+        Reviewed by Simon Fraser.
+
+        Unskipped the two problematic span tests.
+
+        * TestExpectations:
+
 2014-11-05  Bem Jones-Bey  <bjonesbe@adobe.com>
 

trunk/LayoutTests/TestExpectations

@@ -104 +104 @@
 # Expando properties on attribute nodes disappear
 webkit.org/b/88045 fast/dom/gc-attribute-node.html [ Failure Pass ]
-
-webkit.org/b/137316 fast/multicol/newmulticol/spanner-crash.html [ Crash Pass ]
 
 # These tests are incorrect in the CSS test suite and should be fixed there first.
@@ -233 +231 @@
 webkit.org/b/137883 transitions/created-while-suspended.html [ Failure Pass ]
 
-webkit.org/b/138145 fast/multicol/multicol-crazy-nesting.html [ Skip ]
-
 # Temporarily turn this off. Back on soon.
 webkit.org/b/138358 http/tests/multipart/stop-crash.html [ Skip ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2014-11-04  David Hyatt  <hyatt@apple.com>
+
+        Descendant ends up in wrong flow thread with nested columns and spans.
+        https://bugs.webkit.org/show_bug.cgi?id=137273
+
+        Reviewed by Simon Fraser.
+
+        Unskipped the two problematic span tests.
+
+        * rendering/RenderMultiColumnFlowThread.cpp:
+        (WebCore::isValidColumnSpanner):
+        Remove the guard and comment and added the assertion back in.
+
+        (WebCore::RenderMultiColumnFlowThread::flowThreadDescendantInserted):
+        Changed to no longer use handleSpannerRemoval. Because the spanner was removed from the flow thread's map,
+        handleSpannerRemoval was a no-op. So instead I just removed the placeholder by hand.
+
+        The second fix was to stop destroying the placeholder. Since the placeholder can just have been inserted, you
+        can't delete it, since otherwise code further up the stack will access the deleted object. For now, we just
+        leak the placeholder.
+
+        The third fix is to make sure the subtreeRoot is properly updated to be the new placeholder.
+
 2014-11-05  Andreas Kling  <akling@apple.com>
 

trunk/Source/WebCore/rendering/RenderMultiColumnFlowThread.cpp

@@ -243 +243 @@
 {
     // We assume that we're inside the flow thread. This function is not to be called otherwise.
-    // ASSERT(descendant->isDescendantOf(flowThread));
-    // FIXME: Put this back in when we figure out why spanner-crash.html is triggering it.
-    // See https://bugs.webkit.org/show_bug.cgi?id=137273
-    if (!descendant->isDescendantOf(flowThread))
-        return false;
-    
+    ASSERT(descendant->isDescendantOf(flowThread));
+
     // First make sure that the renderer itself has the right properties for becoming a spanner.
     RenderStyle& style = descendant->style();
@@ -387 +383 @@
                 // We have to nuke the placeholder, since the ancestor already lost the mapping to it when
                 // we shifted the placeholder down into this flow thread.
-                ancestorBlock.multiColumnFlowThread()->handleSpannerRemoval(spanner);
-                placeholder.destroy();
-                
+                if (subtreeRoot == descendant)
+                    subtreeRoot = spanner;
+                placeholder.parent()->removeChild(placeholder);
+
                 // Now we process the spanner.
                 descendant = processPossibleSpannerDescendant(subtreeRoot, spanner);