Source/WebCore: Issues with merging ruby bases.
authorinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Sep 2011 20:20:13 +0000 (20:20 +0000)
committerinferno@chromium.org <inferno@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Sep 2011 20:20:13 +0000 (20:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=67240

Reviewed by James Robinson.

1) Change fromBeforeChild to beforeChild to match
webkit rendering naming conventions.
2) Add assert to verify ruby base is indeed emptied
after collecting all children in a single base.
3) Fix condition in mergeBlockChildren to bail out only
when we have no children and there is no work to merge
children to toBase.

Test: fast/ruby/ruby-overhang-crash.html

* rendering/RenderRubyBase.cpp:
(WebCore::RenderRubyBase::moveChildren):
(WebCore::RenderRubyBase::moveInlineChildren):
(WebCore::RenderRubyBase::moveBlockChildren):
(WebCore::RenderRubyBase::mergeBlockChildren):
* rendering/RenderRubyBase.h:
* rendering/RenderRubyRun.cpp:
(WebCore::RenderRubyRun::removeChild):

LayoutTests: Issues with merging ruby bases.
https://bugs.webkit.org/show_bug.cgi?id=67240

Reviewed by James Robinson.

ASSERTION FAILED: !needsLayout() in RenderRubyRun::getOverhang.

* fast/ruby/ruby-overhang-crash-expected.txt: Added.
* fast/ruby/ruby-overhang-crash.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@95462 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/ruby/ruby-overhang-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/ruby/ruby-overhang-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderRubyBase.cpp
Source/WebCore/rendering/RenderRubyBase.h
Source/WebCore/rendering/RenderRubyRun.cpp

index aa51554..233771e 100644 (file)
@@ -1,3 +1,15 @@
+2011-09-19  Abhishek Arya  <inferno@chromium.org>
+
+        Issues with merging ruby bases.
+        https://bugs.webkit.org/show_bug.cgi?id=67240        
+
+        Reviewed by James Robinson.
+
+        ASSERTION FAILED: !needsLayout() in RenderRubyRun::getOverhang.
+
+        * fast/ruby/ruby-overhang-crash-expected.txt: Added.
+        * fast/ruby/ruby-overhang-crash.html: Added.
+
 2011-09-16  Abhishek Arya  <inferno@chromium.org>
 
         Child not placed correctly when beforeChild (table part)
 2011-09-16  Abhishek Arya  <inferno@chromium.org>
 
         Child not placed correctly when beforeChild (table part)
diff --git a/LayoutTests/fast/ruby/ruby-overhang-crash-expected.txt b/LayoutTests/fast/ruby/ruby-overhang-crash-expected.txt
new file mode 100644 (file)
index 0000000..8a28a0d
--- /dev/null
@@ -0,0 +1,2 @@
+Test passes if it does not crash. 
+
diff --git a/LayoutTests/fast/ruby/ruby-overhang-crash.html b/LayoutTests/fast/ruby/ruby-overhang-crash.html
new file mode 100644 (file)
index 0000000..cdd7725
--- /dev/null
@@ -0,0 +1,23 @@
+<html>\r
+<body>\r
+Test passes if it does not crash.\r
+<ruby><svg><table><span><div><tr></tr><rt id="test"></rt></ruby>\r
+<script>\r
+function runTest()\r
+{\r
+    var child = document.getElementById('test');\r
+    child.parentNode.removeChild(child);\r
+    \r
+    if (window.layoutTestController)\r
+        layoutTestController.notifyDone();\r
+}\r
+\r
+if (window.layoutTestController) {\r
+    layoutTestController.dumpAsText();\r
+    layoutTestController.waitUntilDone();\r
+}\r
+\r
+setTimeout("runTest()", 0);\r
+</script>\r
+</body>\r
+</html>\r
index e3d4761..8101729 100644 (file)
@@ -1,3 +1,29 @@
+2011-09-19  Abhishek Arya  <inferno@chromium.org>
+
+        Issues with merging ruby bases.
+        https://bugs.webkit.org/show_bug.cgi?id=67240
+
+        Reviewed by James Robinson.
+
+        1) Change fromBeforeChild to beforeChild to match
+        webkit rendering naming conventions.
+        2) Add assert to verify ruby base is indeed emptied
+        after collecting all children in a single base.
+        3) Fix condition in mergeBlockChildren to bail out only
+        when we have no children and there is no work to merge
+        children to toBase.
+
+        Test: fast/ruby/ruby-overhang-crash.html
+
+        * rendering/RenderRubyBase.cpp:
+        (WebCore::RenderRubyBase::moveChildren):
+        (WebCore::RenderRubyBase::moveInlineChildren):
+        (WebCore::RenderRubyBase::moveBlockChildren):
+        (WebCore::RenderRubyBase::mergeBlockChildren):
+        * rendering/RenderRubyBase.h:
+        * rendering/RenderRubyRun.cpp:
+        (WebCore::RenderRubyRun::removeChild):
+
 2011-09-16  Abhishek Arya  <inferno@chromium.org>
 
         Child not placed correctly when beforeChild (table part)
 2011-09-16  Abhishek Arya  <inferno@chromium.org>
 
         Child not placed correctly when beforeChild (table part)
index 464346d..2d50ef8 100644 (file)
@@ -65,7 +65,7 @@ bool RenderRubyBase::hasOnlyWrappedInlineChildren(RenderObject* beforeChild) con
     return true;
 }
 
     return true;
 }
 
-void RenderRubyBase::moveChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild)
+void RenderRubyBase::moveChildren(RenderRubyBase* toBase, RenderObject* beforeChild)
 {
     // This function removes all children that are before (!) beforeChild
     // and appends them to toBase.
 {
     // This function removes all children that are before (!) beforeChild
     // and appends them to toBase.
@@ -75,19 +75,19 @@ void RenderRubyBase::moveChildren(RenderRubyBase* toBase, RenderObject* fromBefo
     // Inline children might be wrapped in an anonymous block if there's a continuation.
     // Theoretically, in ruby bases, this can happen with only the first such a child,
     // so it should be OK to just climb the tree.
     // Inline children might be wrapped in an anonymous block if there's a continuation.
     // Theoretically, in ruby bases, this can happen with only the first such a child,
     // so it should be OK to just climb the tree.
-    while (fromBeforeChild && fromBeforeChild->parent() != this)
-        fromBeforeChild = fromBeforeChild->parent();
+    while (beforeChild && beforeChild->parent() != this)
+        beforeChild = beforeChild->parent();
 
     if (childrenInline())
 
     if (childrenInline())
-        moveInlineChildren(toBase, fromBeforeChild);
+        moveInlineChildren(toBase, beforeChild);
     else
     else
-        moveBlockChildren(toBase, fromBeforeChild);
+        moveBlockChildren(toBase, beforeChild);
 
     setNeedsLayoutAndPrefWidthsRecalc();
     toBase->setNeedsLayoutAndPrefWidthsRecalc();
 }
 
 
     setNeedsLayoutAndPrefWidthsRecalc();
     toBase->setNeedsLayoutAndPrefWidthsRecalc();
 }
 
-void RenderRubyBase::moveInlineChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild)
+void RenderRubyBase::moveInlineChildren(RenderRubyBase* toBase, RenderObject* beforeChild)
 {
     RenderBlock* toBlock;
 
 {
     RenderBlock* toBlock;
 
@@ -106,17 +106,17 @@ void RenderRubyBase::moveInlineChildren(RenderRubyBase* toBase, RenderObject* fr
         }
     }
     // Move our inline children into the target block we determined above.
         }
     }
     // Move our inline children into the target block we determined above.
-    moveChildrenTo(toBlock, firstChild(), fromBeforeChild);
+    moveChildrenTo(toBlock, firstChild(), beforeChild);
 }
 
 }
 
-void RenderRubyBase::moveBlockChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild)
+void RenderRubyBase::moveBlockChildren(RenderRubyBase* toBase, RenderObject* beforeChild)
 {
     if (toBase->childrenInline()) {
         // First check whether we move only wrapped inline objects.
 {
     if (toBase->childrenInline()) {
         // First check whether we move only wrapped inline objects.
-        if (hasOnlyWrappedInlineChildren(fromBeforeChild)) {
+        if (hasOnlyWrappedInlineChildren(beforeChild)) {
             // The reason why the base is in block flow must be after beforeChild.
             // We therefore can extract the inline objects and move them to toBase.
             // The reason why the base is in block flow must be after beforeChild.
             // We therefore can extract the inline objects and move them to toBase.
-            for (RenderObject* child = firstChild(); child != fromBeforeChild; child = firstChild()) {
+            for (RenderObject* child = firstChild(); child != beforeChild; child = firstChild()) {
                 if (child->isAnonymousBlock()) {
                     RenderBlock* anonBlock = toRenderBlock(child);
                     ASSERT(anonBlock->childrenInline());
                 if (child->isAnonymousBlock()) {
                     RenderBlock* anonBlock = toRenderBlock(child);
                     ASSERT(anonBlock->childrenInline());
@@ -133,7 +133,7 @@ void RenderRubyBase::moveBlockChildren(RenderRubyBase* toBase, RenderObject* fro
             // Moving block children -> have to set toBase as block flow
             toBase->makeChildrenNonInline();
             // Move children, potentially collapsing anonymous block wrappers.
             // Moving block children -> have to set toBase as block flow
             toBase->makeChildrenNonInline();
             // Move children, potentially collapsing anonymous block wrappers.
-            mergeBlockChildren(toBase, fromBeforeChild);
+            mergeBlockChildren(toBase, beforeChild);
 
             // Now we need to check if the leftover children are all inline.
             // If so, make this base inline again.
 
             // Now we need to check if the leftover children are all inline.
             // If so, make this base inline again.
@@ -157,24 +157,24 @@ void RenderRubyBase::moveBlockChildren(RenderRubyBase* toBase, RenderObject* fro
             }
         }
     } else
             }
         }
     } else
-        mergeBlockChildren(toBase, fromBeforeChild);
+        mergeBlockChildren(toBase, beforeChild);
 }
 
 }
 
-void RenderRubyBase::mergeBlockChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild)
+void RenderRubyBase::mergeBlockChildren(RenderRubyBase* toBase, RenderObject* beforeChild)
 {
 {
-    // This function removes all children that are before fromBeforeChild and appends them to toBase.
+    // This function removes all children that are before beforeChild and appends them to toBase.
     ASSERT(!childrenInline());
     ASSERT(toBase);
     ASSERT(!toBase->childrenInline());
 
     // Quick check whether we have anything to do, to simplify the following code.
     ASSERT(!childrenInline());
     ASSERT(toBase);
     ASSERT(!toBase->childrenInline());
 
     // Quick check whether we have anything to do, to simplify the following code.
-    if (fromBeforeChild != firstChild())
+    if (!firstChild())
         return;
 
     // If an anonymous block would be put next to another such block, then merge those.
     RenderObject* firstChildHere = firstChild();
     RenderObject* lastChildThere = toBase->lastChild();
         return;
 
     // If an anonymous block would be put next to another such block, then merge those.
     RenderObject* firstChildHere = firstChild();
     RenderObject* lastChildThere = toBase->lastChild();
-    if (firstChildHere && firstChildHere->isAnonymousBlock() && firstChildHere->childrenInline() 
+    if (firstChildHere->isAnonymousBlock() && firstChildHere->childrenInline() 
             && lastChildThere && lastChildThere->isAnonymousBlock() && lastChildThere->childrenInline()) {            
         RenderBlock* anonBlockHere = toRenderBlock(firstChildHere);
         RenderBlock* anonBlockThere = toRenderBlock(lastChildThere);
             && lastChildThere && lastChildThere->isAnonymousBlock() && lastChildThere->childrenInline()) {            
         RenderBlock* anonBlockHere = toRenderBlock(firstChildHere);
         RenderBlock* anonBlockThere = toRenderBlock(lastChildThere);
@@ -183,7 +183,7 @@ void RenderRubyBase::mergeBlockChildren(RenderRubyBase* toBase, RenderObject* fr
         anonBlockHere->destroy();
     }
     // Move all remaining children normally.
         anonBlockHere->destroy();
     }
     // Move all remaining children normally.
-    moveChildrenTo(toBase, firstChild(), fromBeforeChild);
+    moveChildrenTo(toBase, firstChild(), beforeChild);
 }
 
 RenderRubyRun* RenderRubyBase::rubyRun() const
 }
 
 RenderRubyRun* RenderRubyBase::rubyRun() const
index 850ab65..aaf1473 100644 (file)
@@ -54,10 +54,10 @@ private:
 
     bool hasOnlyWrappedInlineChildren(RenderObject* beforeChild = 0) const;
 
 
     bool hasOnlyWrappedInlineChildren(RenderObject* beforeChild = 0) const;
 
-    void moveChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild = 0);
-    void moveInlineChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild = 0);
-    void moveBlockChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild = 0);
-    void mergeBlockChildren(RenderRubyBase* toBase, RenderObject* fromBeforeChild = 0);
+    void moveChildren(RenderRubyBase* toBase, RenderObject* beforeChild = 0);
+    void moveInlineChildren(RenderRubyBase* toBase, RenderObject* beforeChild = 0);
+    void moveBlockChildren(RenderRubyBase* toBase, RenderObject* beforeChild = 0);
+    void mergeBlockChildren(RenderRubyBase* toBase, RenderObject* beforeChild = 0);
 
     RenderRubyRun* rubyRun() const;
 
 
     RenderRubyRun* rubyRun() const;
 
index 0dee637..530c6bd 100644 (file)
@@ -168,6 +168,7 @@ void RenderRubyRun::removeChild(RenderObject* child)
                 moveChildTo(rightRun, base);
                 rightRun->moveChildTo(this, rightBase);
                 // The now empty ruby base will be removed below.
                 moveChildTo(rightRun, base);
                 rightRun->moveChildTo(this, rightBase);
                 // The now empty ruby base will be removed below.
+                ASSERT(!rubyBase()->firstChild());
             }
         }
     }
             }
         }
     }