Bug 188008 - Crash under PlatformPopupMenuData::encode when interacting with a select menu that has variable fonts
Summary: Crash under PlatformPopupMenuData::encode when interacting with a select menu...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Local Build
Hardware: Mac macOS 10.13
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-25 10:26 PDT by Tim Basiliere
Modified: 2018-09-17 10:11 PDT (History)
14 users (show)

See Also:


Attachments
crash log (110.92 KB, text/plain)
2018-07-28 15:48 PDT, Alexey Proskuryakov
no flags Details
Patch (4.58 KB, patch)
2018-09-10 20:22 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews205 for win-future (12.87 MB, application/zip)
2018-09-10 22:11 PDT, EWS Watchlist
no flags Details
Patch (4.50 KB, patch)
2018-09-11 10:26 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Basiliere 2018-07-25 10:26:18 PDT
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.
Comment 1 Alexey Proskuryakov 2018-07-27 12:24:28 PDT
Thank you for the report! Would you be willing to share a complete web page that reproduces the problem?
Comment 2 Tim Basiliere 2018-07-27 13:17:27 PDT
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!
Comment 3 Alexey Proskuryakov 2018-07-28 15:48:35 PDT
Created attachment 346004 [details]
crash log
Comment 4 Alexey Proskuryakov 2018-07-28 15:49:19 PDT
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
...
Comment 5 Radar WebKit Bug Importer 2018-07-28 15:50:11 PDT
<rdar://problem/42697370>
Comment 6 Myles C. Maxfield 2018-09-10 19:42:14 PDT
<rdar://problem/42588577>
Comment 7 Myles C. Maxfield 2018-09-10 19:45:32 PDT
WK2-only
Comment 8 Myles C. Maxfield 2018-09-10 19:46:08 PDT
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
Comment 9 Myles C. Maxfield 2018-09-10 19:56:17 PDT
Looks like https://bugs.webkit.org/show_bug.cgi?id=180307 didn't go far enough
Comment 10 Myles C. Maxfield 2018-09-10 20:22:45 PDT
Created attachment 349373 [details]
Patch
Comment 11 Daniel Bates 2018-09-10 21:16:22 PDT
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()".
Comment 12 EWS Watchlist 2018-09-10 22:11:38 PDT
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
Comment 13 EWS Watchlist 2018-09-10 22:11:50 PDT
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
Comment 14 Myles C. Maxfield 2018-09-11 10:26:27 PDT
Created attachment 349407 [details]
Patch
Comment 15 WebKit Commit Bot 2018-09-11 12:30:53 PDT
Comment on attachment 349407 [details]
Patch

Clearing flags on attachment: 349407

Committed r235910: <https://trac.webkit.org/changeset/235910>
Comment 16 WebKit Commit Bot 2018-09-11 12:30:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Truitt Savell 2018-09-11 13:53:24 PDT
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
Comment 18 Myles C. Maxfield 2018-09-11 14:11:01 PDT
(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
Comment 19 Truitt Savell 2018-09-11 16:49:28 PDT
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.
Comment 20 Myles C. Maxfield 2018-09-11 17:51:32 PDT
Committed r235924: <https://trac.webkit.org/changeset/235924>
Comment 21 Truitt Savell 2018-09-12 14:57:56 PDT
Test is still timing out on Mac-wk2. Is our solution skipping the new test or are you still working on a fix?
Comment 22 Ryan Haddad 2018-09-13 12:11:17 PDT
If we need more time to investigate, I suggest we roll out this change rather than skip its test.
Comment 23 Myles C. Maxfield 2018-09-14 16:18:23 PDT
(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.
Comment 24 Alexey Proskuryakov 2018-09-14 17:13:20 PDT
I don't follow the argument. Why is it unimportant to have tests for reproducible crashes?
Comment 25 Myles C. Maxfield 2018-09-14 17:27:48 PDT
(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.
Comment 26 Alexey Proskuryakov 2018-09-17 09:45:10 PDT
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.
Comment 27 Alexey Proskuryakov 2018-09-17 10:11:32 PDT
Rolled back in r236068.