Bug 8486 - iExploder(#25865): Failed assertion in RenderText::layout()
Summary: iExploder(#25865): Failed assertion in RenderText::layout()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Lars Knoll
URL:
Keywords: HasReduction, InRadar
: 12040 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-04-19 11:09 PDT by Alexey Proskuryakov
Modified: 2007-02-12 13:59 PST (History)
4 users (show)

See Also:


Attachments
test case (44.41 KB, text/html)
2006-04-19 11:09 PDT, Alexey Proskuryakov
no flags Details
test case (49 bytes, text/html)
2006-06-24 11:03 PDT, Alexey Proskuryakov
no flags Details
patch to fix the crash (8.06 KB, patch)
2007-02-12 05:42 PST, Lars Knoll
no flags Details | Formatted Diff | Diff
new version of the patch (7.89 KB, patch)
2007-02-12 12:34 PST, Lars Knoll
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2006-04-19 11:09:02 PDT
/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
Comment 1 Alexey Proskuryakov 2006-04-19 11:09:25 PDT
Created attachment 7842 [details]
test case
Comment 2 Alexey Proskuryakov 2006-06-24 11:02:18 PDT
Reduction: <div style="display: table-column-group;">a</div>
Comment 3 Alexey Proskuryakov 2006-06-24 11:03:40 PDT
Created attachment 9006 [details]
test case

Same reduction, as an attachment.
Comment 4 mitz 2006-12-31 00:34:01 PST
*** Bug 12040 has been marked as a duplicate of this bug. ***
Comment 5 Mark Rowe (bdash) 2007-02-01 18:10:20 PST
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>

Comment 6 Mark Rowe (bdash) 2007-02-01 18:40:21 PST
<rdar://problem/4971212>
Comment 7 Maciej Stachowiak 2007-02-04 11:20:14 PST
We'd really like iExploder and Hamachi to pass.
Comment 8 Lars Knoll 2007-02-12 05:42:15 PST
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 9 mitz 2007-02-12 06:12:23 PST
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?
Comment 10 Lars Knoll 2007-02-12 12:10:07 PST
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.
Comment 11 Lars Knoll 2007-02-12 12:34:59 PST
Created attachment 13139 [details]
new version of the patch

Fixed the style issues.
Comment 12 mitz 2007-02-12 12:38:49 PST
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;
Comment 13 Beth Dakin 2007-02-12 13:59:39 PST
Committed in r19582.