Bug 221635 – Crash in WebCore::RenderThemeMac::paintSearchFieldResultsDecorationPart() from large scale

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julian Gonzalez]

On macOS, we are hitting an assertion inside CoreUI:

CoreUI: Value passed for rendition key attribute out of bounds for u_int16_t identifier:'kCRThemeScaleName:12' value:'1000000'

0   libsystem_kernel.dylib        	0x00007fff2045a936 __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff20489615 pthread_kill + 263
2   libsystem_c.dylib             	0x00007fff203de411 abort + 120
3   libsystem_c.dylib             	0x00007fff203dd7e8 __assert_rtn + 314
4   com.apple.coreui              	0x00007fff270c3f46 _CUIRenditionKeySetIntegerValueForAttribute.cold.1 + 74
5   com.apple.coreui              	0x00007fff26fe796a _CUIRenditionKeySetIntegerValueForAttribute + 39
...
25  com.apple.WebCore             	0x0000000361d3b1a1 WebCore::RenderThemeMac::paintSearchFieldResultsDecorationPart(WebCore::RenderBox const&, WebCore::PaintInfo const&, WebCore::IntRect const&) + 1233 (RenderThemeMac.mm:2065)
26  com.apple.WebCore             	0x0000000364d85888 WebCore::RenderTheme::paint(WebCore::RenderBox const&, WebCore::ControlStates&, WebCore::PaintInfo const&, WebCore::LayoutRect const&) + 3144
27  com.apple.WebCore             	0x0000000364a8d7a0 WebCore::RenderBox::paintBoxDecorations(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 928 (RenderBox.cpp:1450)
28  com.apple.WebCore             	0x00000003649fe448 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 424 (RenderBlock.cpp:1231)
29  com.apple.WebCore             	0x00000003649fce50 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&) + 608 (RenderBlock.cpp:1108)
30  com.apple.WebCore             	0x0000000364bda723 WebCore::RenderLayer::paintBackgroundForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::GraphicsContext&, WebCore::GraphicsContext&, WebCore::LayoutRect const&, bool, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::RenderObject*) + 1043 (RenderLayer.cpp:3706)

This is reproducible with large values for transform: scale()

<rdar://problem/73044285>

[Julian Gonzalez]

Created attachment 419774 [details]
Patch

[Tim Horton]

Comment on attachment 419774 [details]
Patch

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

> LayoutTests/platform/mac/editing/style/large-scale-crash-expected.txt:1
> +* { -webkit-appearance: searchfield-results-decoration; transform: scale(100); writing-mode: vertical-rl; } if (window.testRunner) testRunner.dumpAsText(); onload = () => { document.styleSheets[0].insertRule(`* { all: initial; }`); }; This tests that we do not hit an assertion while rendering the page. (which prints out its style). PASS

Is this all scrunched on one line for a reason? Usually we format -expected HTML nicely like we do the real test.

[Ryosuke Niwa]

Comment on attachment 419774 [details]
Patch

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

>> LayoutTests/platform/mac/editing/style/large-scale-crash-expected.txt:1
>> +* { -webkit-appearance: searchfield-results-decoration; transform: scale(100); writing-mode: vertical-rl; } if (window.testRunner) testRunner.dumpAsText(); onload = () => { document.styleSheets[0].insertRule(`* { all: initial; }`); }; This tests that we do not hit an assertion while rendering the page. (which prints out its style). PASS
> 
> Is this all scrunched on one line for a reason? Usually we format -expected HTML nicely like we do the real test.

It might be the side effect of CSS rule here...

[Tim Horton]

Comment on attachment 419774 [details]
Patch

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

>>> LayoutTests/platform/mac/editing/style/large-scale-crash-expected.txt:1
>>> +* { -webkit-appearance: searchfield-results-decoration; transform: scale(100); writing-mode: vertical-rl; } if (window.testRunner) testRunner.dumpAsText(); onload = () => { document.styleSheets[0].insertRule(`* { all: initial; }`); }; This tests that we do not hit an assertion while rendering the page. (which prints out its style). PASS
>> 
>> Is this all scrunched on one line for a reason? Usually we format -expected HTML nicely like we do the real test.
> 
> It might be the side effect of CSS rule here...

OH, I misread, I thought it was a expected.html, not .txt. Sorry :)

[Tim Horton]

Comment on attachment 419774 [details]
Patch

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

>>>> LayoutTests/platform/mac/editing/style/large-scale-crash-expected.txt:1
>>>> +* { -webkit-appearance: searchfield-results-decoration; transform: scale(100); writing-mode: vertical-rl; } if (window.testRunner) testRunner.dumpAsText(); onload = () => { document.styleSheets[0].insertRule(`* { all: initial; }`); }; This tests that we do not hit an assertion while rendering the page. (which prints out its style). PASS
>>> 
>>> Is this all scrunched on one line for a reason? Usually we format -expected HTML nicely like we do the real test.
>> 
>> It might be the side effect of CSS rule here...
> 
> OH, I misread, I thought it was a expected.html, not .txt. Sorry :)

(You could probably make the expected.txt look a little less ridiculous by only applying the style to a single element, instead of `*`, but obviously would want to make sure it still triggered the crash if you did that)

[Julian Gonzalez]

(In reply to Tim Horton from comment #5)
> Comment on attachment 419774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419774&action=review
> 
> >>>> LayoutTests/platform/mac/editing/style/large-scale-crash-expected.txt:1
> >>>> +* { -webkit-appearance: searchfield-results-decoration; transform: scale(100); writing-mode: vertical-rl; } if (window.testRunner) testRunner.dumpAsText(); onload = () => { document.styleSheets[0].insertRule(`* { all: initial; }`); }; This tests that we do not hit an assertion while rendering the page. (which prints out its style). PASS
> >>> 
> >>> Is this all scrunched on one line for a reason? Usually we format -expected HTML nicely like we do the real test.
> >> 
> >> It might be the side effect of CSS rule here...
> > 
> > OH, I misread, I thought it was a expected.html, not .txt. Sorry :)
> 
> (You could probably make the expected.txt look a little less ridiculous by
> only applying the style to a single element, instead of `*`, but obviously
> would want to make sure it still triggered the crash if you did that)

I tried this a few times (isolating the rule to one element) but I wasn't able to keep the crashing property of the test that way :(

[Ryosuke Niwa]

Comment on attachment 419774 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        * platform/mac/editing/style/large-scale-crash-expected.txt: Added.
> +        * platform/mac/editing/style/large-scale-crash.html: Added.

Oh, please place this under fast/css.
This is nothing to do with editing, and we don't want to be placing tests under platform directory.

[Ryosuke Niwa]

Comment on attachment 419774 [details]
Patch

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

>> LayoutTests/ChangeLog:12
>> +        * platform/mac/editing/style/large-scale-crash.html: Added.
> 
> Oh, please place this under fast/css.
> This is nothing to do with editing, and we don't want to be placing tests under platform directory.

Or maybe fast/rendering. We should probably rename the file to include searchfield in the name since that's where we crash.

[Ryosuke Niwa]

Created attachment 419901 [details]
Improved test

[Julian Gonzalez]

Created attachment 419924 [details]
Patch

[Julian Gonzalez]

(In reply to Ryosuke Niwa from comment #9)
> Created attachment 419901 [details]
> Improved test

Thanks, I incorporated this test into the new patch!

[EWS]

Committed r272794: <https://commits.webkit.org/r272794>

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