Bug 203116 - Native text substitutions interfere with HTML <datalist> options resulting in crash
Summary: Native text substitutions interfere with HTML <datalist> options resulting in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: Safari 13
Hardware: Macintosh macOS 10.14
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on: 203777
Blocks:
  Show dependency treegraph
 
Reported: 2019-10-17 11:35 PDT by Javan Makhmali
Modified: 2019-11-20 17:08 PST (History)
9 users (show)

See Also:


Attachments
Minimal test case to reproduce <datalist> crash (225 bytes, text/html)
2019-10-17 11:35 PDT, Javan Makhmali
no flags Details
Screen recording of the crash in Safari 13 (1.01 MB, video/mp4)
2019-10-17 11:36 PDT, Javan Makhmali
no flags Details
Safari 13 crash log (114.65 KB, text/plain)
2019-10-17 11:37 PDT, Javan Makhmali
no flags Details
Screen recording of the crash in Safari TP (990.23 KB, video/mp4)
2019-10-17 11:37 PDT, Javan Makhmali
no flags Details
Safari TP crash log (118.31 KB, text/plain)
2019-10-17 11:38 PDT, Javan Makhmali
no flags Details
Repro w/o requiring replacement UI (328 bytes, text/html)
2019-11-01 15:32 PDT, Wenson Hsieh
no flags Details
Fixes the crash (16.23 KB, patch)
2019-11-02 17:22 PDT, Wenson Hsieh
no flags Details | Formatted Diff | Diff
long -> unsigned (6.54 KB, patch)
2019-11-12 19:25 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews213 for win-future (14.36 MB, application/zip)
2019-11-12 23:32 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Javan Makhmali 2019-10-17 11:35:39 PDT
Created attachment 381204 [details]
Minimal test case to reproduce <datalist> crash

To reproduce:

1. Open System Preferences → Keyboard → Text and add entry to replace "cat" with "Cat"
2. Open the attached datalist.html file in Safari
3. Ensure that the menu option for Edit → Substitutions → Text Replacement is selected
3. Type "cat" in the text field and wait for the "Cat" replacement overlay to appear
4. Click any option from the expanded <datalist> menu and Safari will crash


Tested on macOS 10.14.6 (18G103) with Safari 13.0.2 (14608.2.40.1.3) and Safari TP Release 94 (Safari 13.1, WebKit 14609.1.6.1). I believe this issue occurs in Safari 12.1 as well.
Comment 1 Javan Makhmali 2019-10-17 11:36:52 PDT
Created attachment 381205 [details]
Screen recording of the crash in Safari 13
Comment 2 Javan Makhmali 2019-10-17 11:37:24 PDT
Created attachment 381206 [details]
Safari 13 crash log
Comment 3 Javan Makhmali 2019-10-17 11:37:54 PDT
Created attachment 381207 [details]
Screen recording of the crash in Safari TP
Comment 4 Javan Makhmali 2019-10-17 11:38:11 PDT
Created attachment 381208 [details]
Safari TP crash log
Comment 5 Wenson Hsieh 2019-10-18 09:55:21 PDT
I’m able to reproduce this on macOS 10.15 as well.
Comment 6 David Kilzer (:ddkilzer) 2019-10-18 14:52:14 PDT
<rdar://problem/49875932>
Comment 7 Alexey Proskuryakov 2019-10-19 11:55:20 PDT
> I believe this issue occurs in Safari 12.1 as well.

Confirmed that too.
Comment 8 Wenson Hsieh 2019-11-01 11:00:58 PDT
- (void)selectedRow:(NSTableView *)sender
{
    _dropdown->didSelectOption(_suggestions.at([sender selectedRow]));
}

-[NSTableView selectedRow] returns -1 here for some reason, causing us to crash.
Comment 9 Wenson Hsieh 2019-11-01 11:50:45 PDT
(In reply to Wenson Hsieh from comment #8)
> - (void)selectedRow:(NSTableView *)sender
> {
>     _dropdown->didSelectOption(_suggestions.at([sender selectedRow]));
> }
> 
> -[NSTableView selectedRow] returns -1 here for some reason, causing us to
> crash.

Looks like we’re also responsible for nuking the index :/

<WKDataListSuggestionTableView 0x7f8913cd7ac0>: (17) _lastSelectedRow := -1

-[NSTableView setLastSelectedRow:] + 266
-[NSTableRowData reloadData] + 507
-[NSTableView reloadData] + 252
-[WKDataListSuggestionTableView reload] + 36
-[WKDataListSuggestionsController updateWithInformation:] + 102
_ZN6WebKit33WebDataListSuggestionsDropdownMac4showEON7WebCore29DataListSuggestionInformationE + 109
_ZN6WebKit12WebPageProxy23showDataListSuggestionsEON7WebCore29DataListSuggestionInformationE + 166
_ZN3IPC22callMemberFunctionImplIN6WebKit12WebPageProxyEMS2_FvON7WebCore29DataListSuggestionInformationEENSt3__15tupleIJS4_EEEJLm0EEEEvPT_T0_OT1_NS8_16integer_sequenceImJXspT2_EEEE + 160
_ZN3IPC18callMemberFunctionIN6WebKit12WebPageProxyEMS2_FvON7WebCore29DataListSuggestionInformationEENSt3__15tupleIJS4_EEENS8_16integer_sequenceImJLm0EEEEEEvOT1_PT_T0_ + 112
_ZN3IPC13handleMessageIN8Messages12WebPageProxy23ShowDataListSuggestionsEN6WebKit12WebPageProxyEMS5_FvON7WebCore29DataListSuggestionInformationEEEEvRNS_7DecoderEPT0_T1_ + 253
Comment 10 Wenson Hsieh 2019-11-01 12:10:36 PDT
This call to -reload happens because the value of the input field changes:

1   0x1103e34f7 WebKit::WebDataListSuggestionPicker::displayWithActivationType(WebCore::DataListSuggestionActivationType)
2   0x26ac32d53 WebCore::TextFieldInputType::displaySuggestions(WebCore::DataListSuggestionActivationType)
3   0x26ac3613b WebCore::TextFieldInputType::didSetValueByUserEdit()
4   0x26ac2a02c WebCore::SearchInputType::didSetValueByUserEdit()
5   0x26ac35fc5 WebCore::TextFieldInputType::subtreeHasChanged()
6   0x26aaedda2 WebCore::HTMLInputElement::subtreeHasChanged()
7   0x26abc55a3 WebCore::HTMLTextFormControlElement::didEditInnerTextValue()
8   0x26a94a3d0 WebCore::notifyTextFromControls(WebCore::Element*, WebCore::Element*)
9   0x26a94a008 WebCore::Editor::appliedEditing(WebCore::CompositeEditCommand&)
10  0x26a90d0e3 WebCore::CompositeEditCommand::didApplyCommand()
11  0x26a8f87ee WebCore::CompositeEditCommand::apply()
12  0x26a8f8253 WebCore::AlternativeTextController::applyAlternativeTextToRange(WebCore::Range const&, WTF::String const&, WebCore::AlternativeTextType, WTF::OptionSet<WebCore::DocumentMarker::MarkerType>)
13  0x26a8f77a3 WebCore::AlternativeTextController::handleAlternativeTextUIResult(WTF::String const&)
14  0x26a957a3e WebCore::Editor::handleAlternativeTextUIResult(WTF::String const&)
15  0x1105e768b WebKit::WebPage::handleAlternativeTextUIResult(WTF::String const&)

…in turn, due to the autocorrection bubble going away. I think I should be able to come up with a test case for this crash that instead triggers the reload via script, and doesn’t depend on the autocorrect bubble.
Comment 11 Wenson Hsieh 2019-11-01 15:32:18 PDT
Created attachment 382641 [details]
Repro w/o requiring replacement UI

To repro, load the page and click "Foo" in the suggestions dropdown.
Comment 12 Wenson Hsieh 2019-11-02 17:22:44 PDT
Created attachment 382686 [details]
Fixes the crash
Comment 13 WebKit Commit Bot 2019-11-05 10:52:19 PST
Comment on attachment 382686 [details]
Fixes the crash

Clearing flags on attachment: 382686

Committed r252062: <https://trac.webkit.org/changeset/252062>
Comment 14 WebKit Commit Bot 2019-11-05 10:52:21 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Javan Makhmali 2019-11-05 11:15:53 PST
❤️
Comment 16 Darin Adler 2019-11-12 18:28:13 PST
Comment on attachment 382686 [details]
Fixes the crash

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

> Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:222
> +    void activateDataListSuggestion(unsigned long index, object callback);

"unsigned long" in IDL corresponds to "unsigned" in C++

> Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:204
> +    virtual void activateDataListSuggestion(long, JSValueRef) { notImplemented(); }

But here we are using "long" instead of "unsigned". There’s almost no reason to use "long" or "unsigned long" in any WebKit C++ code, and definitely no reason to use it here. And why say unsigned in the IDL and use a signed type in the C++?
Comment 17 Wenson Hsieh 2019-11-12 18:50:54 PST
(In reply to Darin Adler from comment #16)
> Comment on attachment 382686 [details]
> Fixes the crash
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382686&action=review
> 
> > Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:222
> > +    void activateDataListSuggestion(unsigned long index, object callback);
> 
> "unsigned long" in IDL corresponds to "unsigned" in C++
> 
> > Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:204
> > +    virtual void activateDataListSuggestion(long, JSValueRef) { notImplemented(); }
> 
> But here we are using "long" instead of "unsigned". There’s almost no reason
> to use "long" or "unsigned long" in any WebKit C++ code, and definitely no
> reason to use it here. And why say unsigned in the IDL and use a signed type
> in the C++?

Oops, yeah, index should just be an unsigned. Fixing in a followup...
Comment 18 Wenson Hsieh 2019-11-12 19:25:27 PST
Reopening to attach new patch.
Comment 19 Wenson Hsieh 2019-11-12 19:25:28 PST
Created attachment 383422 [details]
long -> unsigned
Comment 20 Build Bot 2019-11-12 23:32:07 PST Comment hidden (obsolete)
Comment 21 Build Bot 2019-11-12 23:32:10 PST Comment hidden (obsolete)
Comment 22 WebKit Commit Bot 2019-11-20 17:08:37 PST
Comment on attachment 383422 [details]
long -> unsigned

Clearing flags on attachment: 383422

Committed r252719: <https://trac.webkit.org/changeset/252719>
Comment 23 WebKit Commit Bot 2019-11-20 17:08:39 PST
All reviewed patches have been landed.  Closing bug.