Bug 188008 – Crash under PlatformPopupMenuData::encode when interacting with a select menu that has variable fonts

• Component: New Bugs
• Status: REOPENED

[Tim Basiliere]

Safari build archive r234192, and Safari current release 11.0.3 on macOS 10.13.3

When clicking on a custom select menu that has a WOFF or WOFF2 variable font applied to it, the page reloads with the message "This webpage was reloaded because a problem occurred."
To replicate, apply CSS to select menu to force custom select ( select {border:0;} ) and apply WOFF or WOFF2 variable font.

[Alexey Proskuryakov]

Thank you for the report! Would you be willing to share a complete web page that reproduces the problem?

[Tim Basiliere]

I created a codepen that can replicate this on Safari. https://codepen.io/SugarSnacks/pen/mjwoWL 

I also tested this on Maxthon For Mac Version 5.1.130 to make sure it wasn't Safari specific, and it acts the same, so I think this may be webkit based.

Thank you for looking into this!

[Alexey Proskuryakov]

Created attachment 346004 [details]
crash log

[Alexey Proskuryakov]

Thank you!

Application Specific Information:
Crashing on exception: -[__NSCFNumber length]: unrecognized selector sent to instance 0x7767687437
Bundle controller class:
BrowserBundleController
 

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff3929466b __exceptionPreprocess + 171
1   libobjc.A.dylib                     0x00007fff6060e942 objc_exception_throw + 48
2   CoreFoundation                      0x00007fff3932b994 -[NSObject(NSObject) doesNotRecognizeSelector:] + 132
3   CoreFoundation                      0x00007fff3920c133 ___forwarding___ + 1443
4   CoreFoundation                      0x00007fff3920bb08 _CF_forwarding_prep_0 + 120
5   WebKit                              0x00007fff47e81fed _ZN3IPC6encodeERNS_7EncoderEPK10__CFString + 34
6   WebKit                              0x00007fff47e81dab _ZN3IPC6encodeERNS_7EncoderEPK14__CFDictionary + 325
7   WebKit                              0x00007fff47e81dd6 _ZN3IPC6encodeERNS_7EncoderEPK14__CFDictionary + 368
8   WebKit                              0x00007fff47f6a798 _ZNK6WebKit21PlatformPopupMenuData6encodeERN3IPC7EncoderE + 18
9   WebKit                              0x00007fff480e9232 _ZN3IPC10Connection4sendIN8Messages12WebPageProxy13ShowPopupMenuEEEbOT_yN3WTF9OptionSetINS_10SendOptionEEE + 98
...

[Radar WebKit Bug Importer]

<rdar://problem/42697370>

[Myles C. Maxfield]

<rdar://problem/42588577>

[Myles C. Maxfield]

WK2-only

[Myles C. Maxfield]

A debug build of webkit shows:

  * frame #0: 0x0000000510e05950 JavaScriptCore`::WTFCrash() at Assertions.cpp:267
    frame #1: 0x0000000100ec0f5b WebKit`WTFCrashWithInfo((null)=430, (null)="/Users/mmaxfield/src/WebKit/OpenSource/Source/WebKit/Shared/cf/ArgumentCodersCF.cpp", (null)="void IPC::encode(IPC::Encoder &, CFDictionaryRef)", (null)=182) at Assertions.h:551
    frame #2: 0x0000000100ec4c6a WebKit`IPC::encode(encoder=0x0000000517441500, dictionary=0x00007f88d43020a0) at ArgumentCodersCF.cpp:430
    frame #3: 0x0000000100ec419a WebKit`IPC::encode(encoder=0x0000000517441500, typeRef=0x00007f88d43020a0) at ArgumentCodersCF.cpp:167
    frame #4: 0x0000000100ec4df3 WebKit`IPC::encode(encoder=0x0000000517441500, dictionary=0x00007f88d41087c0) at ArgumentCodersCF.cpp:438
    frame #5: 0x00000001013c2914 WebKit`WebKit::FontInfo::encode(this=0x00007ffeeed4d9c0, encoder=0x0000000517441500) const at FontInfo.cpp:42
    frame #6: 0x00000001013d2ebd WebKit`IPC::ArgumentCoder<WebKit::FontInfo>::encode(encoder=0x0000000517441500, t=0x00007ffeeed4d9c0) at ArgumentCoder.h:90
    frame #7: 0x00000001013d2e95 WebKit`void IPC::Encoder::encode<WebKit::FontInfo const&, (void*)0>(this=0x0000000517441500, t=0x00007ffeeed4d9c0) at Encoder.h:71
    frame #8: 0x00000001013c6737 WebKit`IPC::Encoder& IPC::Encoder::operator<<<WebKit::FontInfo const&, (void*)0>(WebKit::FontInfo const&&&)(this=0x0000000517441500, t=0x00007ffeeed4d9c0) at Encoder.h:84
    frame #9: 0x00000001013c66b1 WebKit`WebKit::PlatformPopupMenuData::encode(this=0x00007ffeeed4d9c0, encoder=0x0000000517441500) const at PlatformPopupMenuData.cpp:36

[Myles C. Maxfield]

Looks like https://bugs.webkit.org/show_bug.cgi?id=180307 didn't go far enough

[Myles C. Maxfield]

Created attachment 349373 [details]
Patch

[Daniel Bates]

Comment on attachment 349373 [details]
Patch

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

> LayoutTests/fast/text/variations/ipc2.html:39
> +<script>
> +if (window.testRunner) {
> +    testRunner.dumpAsText();
> +    testRunner.waitUntilDone();
> +}
> +window.addEventListener("load", test);
> +</script>

This is a weird way to split the <script>s and it is unclear what benefit is serves. I suggest we move all this code to the top of beginning of the body of the first <script>. Among other benefits this makes the flow more intuitive since the "waitUntilDone()" and dumping options ("dumpAsText()") occurs before the "notifyDone()".

[EWS Watchlist]

Comment on attachment 349373 [details]
Patch

Attachment 349373 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/9169058

New failing tests:
fast/text/variations/ipc2.html

[EWS Watchlist]

Created attachment 349378 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Myles C. Maxfield]

Created attachment 349407 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 349407 [details]
Patch

Clearing flags on attachment: 349407

Committed r235910: <https://trac.webkit.org/changeset/235910>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

New test fast/text/variations/ipc2.html

from https://trac.webkit.org/changeset/235910/webkit

is timing out from the beginning on wk1.

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Ftext%2Fvariations%2Fipc2.html

[Myles C. Maxfield]

(In reply to Truitt Savell from comment #17)
> New test fast/text/variations/ipc2.html
> 
> from https://trac.webkit.org/changeset/235910/webkit
> 
> is timing out from the beginning on wk1.
> 
> Test History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=fast%2Ftext%2Fvariations%2Fipc2.html

EWS is all green :(

I'll see if I can reproduce the hang

[Truitt Savell]

This reproduced the issue on my computer on Mojave 

command:
run-webkit-tests --root testbuild-235910 fast/text/variations/ --iterations 50 -f

you cannot interact with your computer at all during the test. in the end the selection menu should be stuck in the top left corner of the display. the test will timeout.

[Myles C. Maxfield]

Committed r235924: <https://trac.webkit.org/changeset/235924>

[Truitt Savell]

Test is still timing out on Mac-wk2. Is our solution skipping the new test or are you still working on a fix?

[Ryan Haddad]

If we need more time to investigate, I suggest we roll out this change rather than skip its test.

[Myles C. Maxfield]

(In reply to Ryan Haddad from comment #22)
> If we need more time to investigate, I suggest we roll out this change
> rather than skip its test.

Let's disable the test. Rolling the change out would be a mistake because it's a 100% reproducible crash.

[Alexey Proskuryakov]

I don't follow the argument. Why is it unimportant to have tests for reproducible crashes?

[Myles C. Maxfield]

(In reply to Alexey Proskuryakov from comment #24)
> I don't follow the argument. Why is it unimportant to have tests for
> reproducible crashes?

It is important to have tests for reproducible crashes. It's just less important than having fixes for reproducible crashes.

[Alexey Proskuryakov]

The WebKit policy is to have tests with fixes, and a disabled test obviously doesn't satisfy the requirement.

While we do get to apply judgement in extreme cases, there is no general provision like "a fix is more important than a test".

I'm rolling back these patches now.

[Alexey Proskuryakov]

Rolled back in r236068.