Crash in updateFirstLetter() from unnecessary anonymous block

https://bugs.webkit.org/show_bug.cgi?id=72675

Patch by Ken Buchanan <kenrb@chromium.org> on 2012-01-27
Reviewed by David Hyatt.

Source/WebCore:

There was a problem with anonymous blocks not getting removed when
their only block flow siblings are removed if they also have non-block
flow first-letter siblings (i.e. floats). This patch modifies
RenderBlock::removeChild() to look for this situation and strip out
unnecessary anonymous container blocks if it occurs.

* rendering/RenderBlock.cpp:
(WebCore::RenderBlock::removeChild):
(WebCore::RenderBlock::collapseAnonymousBoxChild): Added
* rendering/RenderBlock.h:
(WebCore::RenderBlock::collapseAnonymousBoxChild): Added

LayoutTests:

Adding tests that cause a div to be removed from between a floating
first-letter block and its remaining text. If the anonymous block is
removed as an immediate consequence of the div removal, this shouldn't
crash.

* fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt: Added
* fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html: Added
* fast/css-generated-content/positioned-div-with-floating-after-content-crash-expected.txt: Added
* fast/css-generated-content/positioned-div-with-floating-after-content-crash.html: Added
* fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame1.html: Added
* fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame2.html: Added

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

diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index 616d07c..cc9cfc9 100644 (file)
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -1,3 +1,22 @@
+2012-01-27  Ken Buchanan  <kenrb@chromium.org>
+
+        Crash in updateFirstLetter() from unnecessary anonymous block
+        https://bugs.webkit.org/show_bug.cgi?id=72675
+
+        Reviewed by David Hyatt.
+
+        Adding tests that cause a div to be removed from between a floating
+        first-letter block and its remaining text. If the anonymous block is
+        removed as an immediate consequence of the div removal, this shouldn't
+        crash.
+
+        * fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt: Added
+        * fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html: Added
+        * fast/css-generated-content/positioned-div-with-floating-after-content-crash-expected.txt: Added
+        * fast/css-generated-content/positioned-div-with-floating-after-content-crash.html: Added
+        * fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame1.html: Added
+        * fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame2.html: Added
+
 2012-01-27  Levi Weintraub  <leviw@chromium.org>
 
         Unreviewed gardening. Adding new windows image expectations for css3/flexbox/cross-axis-scrollbar.html

diff --git a/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt b/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt
new file mode 100644 (file)
index 0000000..91145d1
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline-expected.txt
@@ -0,0 +1 @@
+PASS if no crash or assert

diff --git a/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html b/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html
new file mode 100644 (file)
index 0000000..9b05ac2
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/float-first-letter-siblings-convert-to-inline.html
@@ -0,0 +1,34 @@
+<style>
+.inlineFL::first-letter { overflow: visible; }
+.absolutePosition { position: absolute; }
+.floatFL:first-letter { float: right; }
+</style>
+<script>
+function recreateFirstLetterBlock() {
+  document.getElementById("parent").setAttribute('class', 'inlineFL');
+  if (window.layoutTestController)
+    layoutTestController.notifyDone();
+}
+function removeDiv() {
+  // This causes the parent to only have inline (and floating) children
+  document.getElementById("parent").removeChild(document.getElementById("child"));
+  setTimeout("recreateFirstLetterBlock();", 10);
+}
+
+function changeDivStyle() {
+  document.getElementById("child").setAttribute('class', 'inlineFL');
+  setTimeout("removeDiv();", 10);
+}
+function startTest() {
+  setTimeout("changeDivStyle();", 10);
+  if (window.layoutTestController) {
+    layoutTestController.waitUntilDone();
+    layoutTestController.dumpAsText();
+  }
+}
+window.onload = startTest;
+</script>
+<div id="parent" class="floatFL">
+  <div id="child" class="absolutePosition"></div>
+  PASS if no crash or assert
+</div>

diff --git a/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash-expected.txt b/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash-expected.txt
new file mode 100644 (file)
index 0000000..348dc46
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash-expected.txt
@@ -0,0 +1,6 @@
+
+
+--------
+Frame: 'frame'
+--------
+PASS, if no exception or crash

diff --git a/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash.html b/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash.html
new file mode 100644 (file)
index 0000000..8301dd4
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/positioned-div-with-floating-after-content-crash.html
@@ -0,0 +1,11 @@
+<script>
+function runTest() {
+    if (window.layoutTestController) {
+        layoutTestController.waitUntilDone();
+        layoutTestController.dumpAsText();
+        layoutTestController.dumpChildFramesAsText();
+    }
+}
+window.onload = runTest;
+</script>
+<iframe id="frame" src="resources/positioned-div-with-floating-after-content-crash-frame1.html"></iframe>

diff --git a/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame1.html b/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame1.html
new file mode 100644 (file)
index 0000000..5d5524a
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame1.html
@@ -0,0 +1,31 @@
+<style>
+.c1 { display: table; }
+.c1::after { position: fixed; content: counter(section); }
+.c2 { display: table-caption; float: left; }
+</style>
+<script>
+var node = document.createElement('q');
+
+function changeQClass() {
+    node.setAttribute('class', 'c1');
+    setTimeout("appendQ();", 10);
+}
+
+function appendQ() {
+    document.getElementById('positionedDiv').appendChild(node);
+    setTimeout("navigateAway();");
+}
+
+function navigateAway() {
+    // Bug only manifests on document destruction
+    window.location="positioned-div-with-floating-after-content-crash-frame2.html";
+}
+
+function runTest() {
+    setTimeout("changeQClass();", 10);
+}
+window.onload = runTest;
+</script>
+<div class="c2"><textarea></textarea></div>
+<div id="positionedDiv" class="c1">FAIL</div>
+<div class="c2"></div>

diff --git a/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame2.html b/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame2.html
new file mode 100644 (file)
index 0000000..0e6de68
--- /dev/null
+++ b/LayoutTests/fast/css-generated-content/resources/positioned-div-with-floating-after-content-crash-frame2.html
@@ -0,0 +1,8 @@
+<script>
+function finishTest() {
+    if (window.layoutTestController)
+        layoutTestController.notifyDone();
+}
+window.onload = finishTest;
+</script>
+PASS, if no exception or crash

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index c16a464..b432dfb 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,22 @@
+2012-01-27  Ken Buchanan  <kenrb@chromium.org>
+
+        Crash in updateFirstLetter() from unnecessary anonymous block
+        https://bugs.webkit.org/show_bug.cgi?id=72675
+
+        Reviewed by David Hyatt.
+
+        There was a problem with anonymous blocks not getting removed when
+        their only block flow siblings are removed if they also have non-block
+        flow first-letter siblings (i.e. floats). This patch modifies
+        RenderBlock::removeChild() to look for this situation and strip out
+        unnecessary anonymous container blocks if it occurs.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::removeChild):
+        (WebCore::RenderBlock::collapseAnonymousBoxChild): Added
+        * rendering/RenderBlock.h:
+        (WebCore::RenderBlock::collapseAnonymousBoxChild): Added
+
 2012-01-27  Fady Samuel  <fsamuel@chromium.org>
 
         Rename shouldLayoutFixedElementsRelativeToFrame and make it a setting

diff --git a/Source/WebCore/rendering/RenderBlock.cpp b/Source/WebCore/rendering/RenderBlock.cpp
index ba5b013..71c5d4b 100755 (executable)
--- a/Source/WebCore/rendering/RenderBlock.cpp
+++ b/Source/WebCore/rendering/RenderBlock.cpp
@@ -1025,6 +1025,18 @@ static bool canMergeContiguousAnonymousBlocks(RenderObject* oldChild, RenderObje
            && prev->isAnonymousColumnSpanBlock() == next->isAnonymousColumnSpanBlock();
 }
 
+void RenderBlock::collapseAnonymousBoxChild(RenderBlock* parent, RenderObject* child)
+{
+    parent->setNeedsLayoutAndPrefWidthsRecalc();
+    parent->setChildrenInline(child->childrenInline());
+    RenderBlock* anonBlock = toRenderBlock(parent->children()->removeChildNode(parent, child, child->hasLayer()));
+    anonBlock->moveAllChildrenTo(parent, child->hasLayer());
+    // Delete the now-empty block's lines and nuke it.
+    if (!parent->documentBeingDestroyed())
+        anonBlock->deleteLineBoxTree();
+    anonBlock->destroy();
+}
+
 void RenderBlock::removeChild(RenderObject* oldChild)
 {
     // If this child is a block, and if our previous and next siblings are
@@ -1081,13 +1093,17 @@ void RenderBlock::removeChild(RenderObject* oldChild)
         // The removal has knocked us down to containing only a single anonymous
         // box.  We can go ahead and pull the content right back up into our
         // box.
-        setNeedsLayoutAndPrefWidthsRecalc();
-        setChildrenInline(child->childrenInline());
-        RenderBlock* anonBlock = toRenderBlock(children()->removeChildNode(this, child, child->hasLayer()));
-        anonBlock->moveAllChildrenTo(this, child->hasLayer());
-        // Delete the now-empty block's lines and nuke it.
-        anonBlock->deleteLineBoxTree();
-        anonBlock->destroy();
+        collapseAnonymousBoxChild(this, child);
+    } else if ((prev && prev->isAnonymousBlock()) || (next && next->isAnonymousBlock())) {
+        // It's possible that the removal has knocked us down to a single anonymous
+        // block with pseudo-style element siblings (e.g. first-letter). If these
+        // are floating, then we need to pull the content up also.
+        RenderBlock* anonBlock = toRenderBlock((prev && prev->isAnonymousBlock()) ? prev : next);
+        if ((anonBlock->previousSibling() || anonBlock->nextSibling())
+            && (!anonBlock->previousSibling() || (anonBlock->previousSibling()->style()->styleType() != NOPSEUDO && anonBlock->previousSibling()->isFloating()))
+            && (!anonBlock->nextSibling() || (anonBlock->nextSibling()->style()->styleType() != NOPSEUDO && anonBlock->nextSibling()->isFloating()))) {
+            collapseAnonymousBoxChild(this, anonBlock);
+        }
     }
 
     if (!firstChild() && !documentBeingDestroyed()) {

diff --git a/Source/WebCore/rendering/RenderBlock.h b/Source/WebCore/rendering/RenderBlock.h
index 6506dc8..1db5dee 100644 (file)
--- a/Source/WebCore/rendering/RenderBlock.h
+++ b/Source/WebCore/rendering/RenderBlock.h
@@ -461,6 +461,8 @@ private:
     void makeChildrenNonInline(RenderObject* insertionPoint = 0);
     virtual void removeLeftoverAnonymousBlock(RenderBlock* child);
 
+    static void collapseAnonymousBoxChild(RenderBlock* parent, RenderObject* child);
+
     virtual void dirtyLinesFromChangedChild(RenderObject* child) { m_lineBoxes.dirtyLinesFromChangedChild(this, child); }
 
     void addChildToContinuation(RenderObject* newChild, RenderObject* beforeChild);