Bug 225949 – CrashOnOverflow in WebCore::RenderTable::cellBefore(WebCore::RenderTableCell const*)

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

[Ryosuke Niwa]

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000005b8f5a8e9 WTF::CrashOnOverflow::crash() + 9 (CheckedArithmetic.h:134)
1   com.apple.WebCore             	0x00000005b8f5a8ae WTF::CrashOnOverflow::overflowed() + 14 (CheckedArithmetic.h:127)
2   com.apple.WebCore             	0x00000005bbef91c5 WTF::Vector<WebCore::RenderTableSection::CellStruct, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::at(unsigned long) + 53 (Vector.h:701)
3   com.apple.WebCore             	0x00000005bbef90b9 WTF::Vector<WebCore::RenderTableSection::CellStruct, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>::operator[](unsigned long) + 9 (Vector.h:721)
4   com.apple.WebCore             	0x00000005be4ec595 WebCore::RenderTableSection::cellAt(unsigned int, unsigned int) + 53 (RenderTableSection.h:263)
5   com.apple.WebCore             	0x00000005be8f584e WebCore::RenderTable::cellBefore(WebCore::RenderTableCell const*) const + 78 (RenderTable.cpp:1472)
6   com.apple.WebCore             	0x00000005be905403 WebCore::RenderTableCell::computeCollapsedStartBorder(WebCore::IncludeBorderColorOrNot) const + 723 (RenderTableCell.cpp:571)
7   com.apple.WebCore             	0x00000005be904e36 WebCore::RenderTableCell::collapsedStartBorder(WebCore::IncludeBorderColorOrNot) const + 518 (RenderTableCell.cpp:552)
8   com.apple.WebCore             	0x00000005be90a068 WebCore::RenderTableCell::borderHalfStart(bool) const + 216 (RenderTableCell.cpp:1078)
9   com.apple.WebCore             	0x00000005be90451c WebCore::RenderTableCell::borderHalfLeft(bool) const + 60 (RenderTableCell.cpp:1048)
10  com.apple.WebCore             	0x00000005be903137 WebCore::RenderTableCell::clippedOverflowRectForRepaint(WebCore::RenderLayerModelObject const*) const + 439 (RenderTableCell.cpp:361)
11  com.apple.WebCore             	0x00000005be89d893 WebCore::RenderObject::repaint() const + 291 (RenderObject.cpp:922)
12  com.apple.WebCore             	0x00000005be91e698 WebCore::RenderTableRow::layout() + 968 (RenderTableRow.cpp:158)
13  com.apple.WebCore             	0x00000005be50f177 WebCore::RenderElement::layoutIfNeeded() + 71 (RenderElement.h:127)
14  com.apple.WebCore             	0x00000005be921ec8 WebCore::RenderTableSection::layout() + 488 (RenderTableSection.cpp:371)
15  com.apple.WebCore             	0x00000005be50f177 WebCore::RenderElement::layoutIfNeeded() + 71 (RenderElement.h:127)
16  com.apple.WebCore             	0x00000005be8f2a97 WebCore::RenderTable::layout() + 2199 (RenderTable.cpp:472)
17  com.apple.WebCore             	0x00000005be50f177 WebCore::RenderElement::layoutIfNeeded() + 71 (RenderElement.h:127)
18  com.apple.WebCore             	0x00000005be50db76 WebCore::ComplexLineLayout::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1030 (ComplexLineLayout.cpp:1780)
19  com.apple.WebCore             	0x00000005be60cec5 WebCore::RenderBlockFlow::layoutInlineChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 373 (RenderBlockFlow.cpp:706)
20  com.apple.WebCore             	0x00000005be60b37d WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 1229 (RenderBlockFlow.cpp:525)
21  com.apple.WebCore             	0x00000005be5da5ba WebCore::RenderBlock::layout() + 282 (RenderBlock.cpp:598)
22  com.apple.WebCore             	0x00000005be6107e5 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1461 (RenderBlockFlow.cpp:764)
23  com.apple.WebCore             	0x00000005be60d20e WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 718 (RenderBlockFlow.cpp:675)
24  com.apple.WebCore             	0x00000005be60b388 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 1240 (RenderBlockFlow.cpp:527)
25  com.apple.WebCore             	0x00000005be5da5ba WebCore::RenderBlock::layout() + 282 (RenderBlock.cpp:598)
26  com.apple.WebCore             	0x00000005be6107e5 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1461 (RenderBlockFlow.cpp:764)
27  com.apple.WebCore             	0x00000005be60d20e WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 718 (RenderBlockFlow.cpp:675)
28  com.apple.WebCore             	0x00000005be60b388 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 1240 (RenderBlockFlow.cpp:527)
29  com.apple.WebCore             	0x00000005be5da5ba WebCore::RenderBlock::layout() + 282 (RenderBlock.cpp:598)
30  com.apple.WebCore             	0x00000005be6107e5 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 1461 (RenderBlockFlow.cpp:764)
31  com.apple.WebCore             	0x00000005be60d20e WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 718 (RenderBlockFlow.cpp:675)
32  com.apple.WebCore             	0x00000005be60b388 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 1240 (RenderBlockFlow.cpp:527)
33  com.apple.WebCore             	0x00000005be5da5ba WebCore::RenderBlock::layout() + 282 (RenderBlock.cpp:598)
34  com.apple.WebCore             	0x00000005be986547 WebCore::RenderView::layout() + 1479 (RenderView.cpp:185)
35  com.apple.WebCore             	0x00000005bda9c54f WebCore::FrameViewLayoutContext::layout() + 1359 (FrameViewLayoutContext.cpp:232)
36  com.apple.WebCore             	0x00000005bc8a4658 WebCore::Document::implicitClose() + 1064 (Document.cpp:3187)
37  com.apple.WebCore             	0x00000005bd803f39 WebCore::FrameLoader::checkCallImplicitClose() + 217 (FrameLoader.cpp:940)
38  com.apple.WebCore             	0x00000005bd8033c3 WebCore::FrameLoader::checkCompleted() + 691 (FrameLoader.cpp:881)
39  com.apple.WebCore             	0x00000005bd7ff8b5 WebCore::FrameLoader::finishedParsing() + 453 (FrameLoader.cpp:786)

<rdar://77846018>

[zalan]

Created attachment 429614 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 429614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=429614&action=review

> Source/WebCore/ChangeLog:18
> +        (WebCore::RenderTableSection::willInsertTableRow): fun fact: this code dates back to r3351 "The new table code from Lars.".

:O

> Source/WebCore/rendering/RenderTableSection.cpp:126
> +    setNeedsCellRecalc();

I guess this won't regress the performance that much as beforeChild is not null as long as we are not inserting the first row. Just not sure how often this is done in the wild...

> LayoutTests/fast/table/table-split-on-insert-with-section-crash.html:1
> +<style>

Can we have a <!DOCTYPE html> or do we depend on quirks mode?

> LayoutTests/fast/table/table-split-on-insert-with-section-crash.html:16
> +  onload = () => {

Check indentation of the whole onload block

> LayoutTests/fast/table/table-split-on-insert-with-section-crash.html:32
> +<!-- Pass if no crash or assert. -->

The designMode="on" normally turns down any attempt to show any message in the expected file via html elements. What about using console.log() to show the text from this comment?

[zalan]

Created attachment 429655 [details]
Patch

[zalan]

> > Source/WebCore/rendering/RenderTableSection.cpp:126
> > +    setNeedsCellRecalc();
> 
> I guess this won't regress the performance that much as beforeChild is not
> null as long as we are not inserting the first row. Just not sure how often
> this is done in the wild...
Yeah, I am not too worried about the perf impact. The grid needs to be invalidated in any way.

> The designMode="on" normally turns down any attempt to show any message in
> the expected file via html elements. What about using console.log() to show
> the text from this comment?
That's a good point in general, but in cases like this I don't think it really matters whether the text shows up anywhere. The text content here is merely a guidance.

[zalan]

and thanks for the review!

[EWS]

Committed r278028 (238125@main): <https://commits.webkit.org/238125@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429655 [details].

[Truitt Savell]

it looks like the changes in https://trac.webkit.org/changeset/278028/webkit has caused 

fast/selectors/slow-style-sharing-with-long-cousin-list.html 

to timeout on iOS and windows. 

History: https://results.webkit.org/?suite=layout-tests&test=fast%2Fselectors%2Fslow-style-sharing-with-long-cousin-list.html

[zalan]

>Yeah, I am not too worried about the perf impact.
lol, 30K rows.

[Truitt Savell]

Reverted r278028 for reason:

Caused fast/selectors/slow-style-sharing-with-long-cousin-list.html to timeout on iOS and Windows.

Committed r278125 (238175@main): <https://commits.webkit.org/238175@main>

[zalan]

While I still don't think it hurts normal use cases, some micro-benchmarks like this test case may suffer. I am going to see if we can make it less expensive (I don't really want to limit the invalidation to this particular "let's split the inline and move over the table rows" case as that may potentially leave the grid invalid for other the mutating codepaths.)

[zalan]

We seem to have an insertion position bug here too.

we've got the following structure:
RenderInline
 RenderTable (generated)
  RenderTableSection (generated)
    RenderTableRow
    RenderTableRow

and the script inserts a <blockquote> where the insertion position is as follows:
parent: RenderInline
nextSibling: second RenderTableRow

A 0x1132fc260 (renderer 0x1132fc6f0) 
  TR 0x1132fe790 (renderer 0x1132fe820) 
    BR 0x1132fe1e0 (renderer 0x1132fefd0) 
  BLOCKQUOTE 0x1132fe540 (renderer 0x1132ff1e0)
    INPUT 0x1132fe270 (renderer 0x0)
  TR 0x1132fc300 (renderer 0x1132fc7d0) 

so we start splitting the <table> right in the middle (between the 2 sibling rows) which triggers some unexpected state. We should fix this incorrect insertion position as well the invalidation bug as it may get triggered by some other (invalid)state.

[zalan]

> I don't really want to limit the invalidation to this particular
> "let's split the inline and move over the table rows" case as that may
> potentially leave the grid invalid for other the mutating codepaths.
Now that I figured it is an invalid insertion, I may just do exactly this to cover this unexpected case.

[zalan]

Created attachment 430027 [details]
Patch

[EWS]

Committed r278219 (238258@main): <https://commits.webkit.org/238258@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430027 [details].