| Summary: | CrashOnOverflow in WebCore::RenderTable::cellBefore(WebCore::RenderTableCell const*) | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||
| Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | bfulgham, cgarcia, changseok, esprehn+autocc, ews-watchlist, fred.wang, glenn, gpoo, koivisto, kondapallykalyan, pdr, product-security, rbuis, simon.fraser, svillar, tsavell, webkit-bug-importer, zalan | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Ryosuke Niwa
2021-05-18 18:00:12 PDT
Created attachment 429614 [details]
Patch
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? Created attachment 429655 [details]
Patch
> > 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. and thanks for the review! 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]. 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 >Yeah, I am not too worried about the perf impact.
lol, 30K rows.
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> 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.) 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.
> 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.
Created attachment 430027 [details]
Patch
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]. |