Bug 225949 - CrashOnOverflow in WebCore::RenderTable::cellBefore(WebCore::RenderTableCell const*)
Summary: CrashOnOverflow in WebCore::RenderTable::cellBefore(WebCore::RenderTableCell ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: zalan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-18 18:00 PDT by Ryosuke Niwa
Modified: 2021-05-28 12:51 PDT (History)
18 users (show)

See Also:


Attachments
Patch (4.73 KB, patch)
2021-05-24 19:50 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (4.66 KB, patch)
2021-05-25 08:58 PDT, zalan
no flags Details | Formatted Diff | Diff
Patch (7.91 KB, patch)
2021-05-28 10:44 PDT, zalan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-05-18 18:00:12 PDT
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>
Comment 1 zalan 2021-05-24 19:50:45 PDT
Created attachment 429614 [details]
Patch
Comment 2 Sergio Villar Senin 2021-05-25 01:46:37 PDT
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?
Comment 3 zalan 2021-05-25 08:58:31 PDT
Created attachment 429655 [details]
Patch
Comment 4 zalan 2021-05-25 09:09:29 PDT
> > 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.
Comment 5 zalan 2021-05-25 09:09:42 PDT
and thanks for the review!
Comment 6 EWS 2021-05-25 10:34:18 PDT
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].
Comment 7 Truitt Savell 2021-05-26 14:09:04 PDT
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
Comment 8 zalan 2021-05-26 14:14:48 PDT
>Yeah, I am not too worried about the perf impact.
lol, 30K rows.
Comment 9 Truitt Savell 2021-05-26 14:19:25 PDT
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>
Comment 10 zalan 2021-05-26 18:29:53 PDT
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.)
Comment 11 zalan 2021-05-27 13:52:26 PDT
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.
Comment 12 zalan 2021-05-28 10:40:48 PDT
> 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.
Comment 13 zalan 2021-05-28 10:44:19 PDT
Created attachment 430027 [details]
Patch
Comment 14 EWS 2021-05-28 12:51:40 PDT
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].