Bug 8486 – iExploder(#25865): Failed assertion in RenderText::layout()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexey Proskuryakov]

/Users/ap/WebKit/WebCore/rendering/RenderText.h:70: failed assertion `false'

#3	0x01be97fc in WebCore::RenderText::layout at RenderText.h:70
#4	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#5	0x01992480 in WebCore::RenderContainer::layout at RenderContainer.cpp:410
#6	0x019fa5d0 in WebCore::RenderTable::layout at RenderTable.cpp:272
#7	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#8	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#9	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#10	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#11	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#12	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#13	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#14	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#15	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#16	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#17	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#18	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#19	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#20	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#21	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#22	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#23	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#24	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#25	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#26	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#27	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#28	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#29	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#30	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#31	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#32	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#33	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#34	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#35	0x01be518c in WebCore::RenderObject::layoutIfNeeded at Font.h:461
#36	0x0197e494 in WebCore::RenderBlock::layoutBlockChildren at RenderBlock.cpp:1073
#37	0x0197f024 in WebCore::RenderBlock::layoutBlock at RenderBlock.cpp:495
#38	0x0196ebb4 in WebCore::RenderBlock::layout at RenderBlock.cpp:417
#39	0x0198d948 in WebCore::RenderCanvas::layout at RenderCanvas.cpp:114
#40	0x018bed3c in WebCore::FrameView::layout at FrameView.cpp:410

[Alexey Proskuryakov]

Created attachment 7842 [details]
test case

[Alexey Proskuryakov]

Reduction: <div style="display: table-column-group;">a</div>

[Alexey Proskuryakov]

Created attachment 9006 [details]
test case

Same reduction, as an attachment.

*** Bug 12040 has been marked as a duplicate of this bug. ***

[Mark Rowe (bdash)]

This assertion failure is also triggered by Hamachi, via different input:

<script type="text/javascript">
    window.onload = function() {
        var c = document.createElement('colgroup');
        c.appendChild(document.createElement('br'));
        c.appendChild(document.createElement('xmp'));
        document.documentElement.appendChild(c);
    }
</script>

[Mark Rowe (bdash)]

<rdar://problem/4971212>

[Maciej Stachowiak]

We'd really like iExploder and Hamachi to pass.

[Lars Knoll]

Created attachment 13127 [details]
patch to fix the crash

The patch fixes the crash. Since it disallows child renderers for table-col-group that aren't table-col's, it changes the result of two other test cases, but IMO this is more correct than the old output.

Comment on attachment 13127 [details]
patch to fix the crash

+            RenderObject *r = createRenderer(document()->renderArena(), style);

The * should go on the other side.

+                if (renderer()) {
is redundant since you already null-checked r.

Is it necessary to add a virtual function to fix this bug? I can think of two alternatives that don't add a virtual function: one is to implement RenderTableCol::layout() to just reset needsLayout and return, but then of course you will still have unneeded renderers in the tree (not sure if that's really a problem). Another option is to change RenderObject::addChild() to return a bool and allow it to reject the child by returning false, then override it in RenderTableCol to do the check. Have you considered these alternatives?

[Lars Knoll]

I don't think just reimplementing layout() in RenderTableCol is a good solution, as it leads to additional RenderObjects just lying around that could possibly mess up other dynamic manipulations of the render tree. In addition the table layouting routing has to make sure it ignores them. IMO it's cleaner design to catch these cases as early as possible.

I also thought about adding a boolean return value to addChild, and we could do the patch that way, but that would require changing quite a bit more code. addChild() is called from quite a few places within the render tree as well. Obviously we could ignore the return value there, but that might lead to other issues in the long term.

[Lars Knoll]

Created attachment 13139 [details]
new version of the patch

Fixed the style issues.

Comment on attachment 13139 [details]
new version of the patch

r=me

Consider replacing this with a single return statement:
+    if (child->isText() || !style || style->display() != TABLE_COLUMN)
+        return false;
+    return true;

[Beth Dakin]

Committed in r19582.