| Summary: | iExploder(#25865): Failed assertion in RenderText::layout() | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Lars Knoll <lars.knoll> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bdakin, ian, mitz, mrowe | ||||||||||
| Priority: | P1 | Keywords: | HasReduction, InRadar | ||||||||||
| Version: | 420+ | ||||||||||||
| Hardware: | Mac | ||||||||||||
| OS: | OS X 10.4 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Alexey Proskuryakov
2006-04-19 11:09:02 PDT
Created attachment 7842 [details]
test case
Reduction: <div style="display: table-column-group;">a</div> Created attachment 9006 [details]
test case
Same reduction, as an attachment.
*** Bug 12040 has been marked as a duplicate of this bug. *** 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>
We'd really like iExploder and Hamachi to pass. 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?
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. 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;
Committed in r19582. |