Bug 203116 – Native text substitutions interfere with HTML <datalist> options resulting in crash

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Javan Makhmali]

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.

[Javan Makhmali]

Created attachment 381205 [details]
Screen recording of the crash in Safari 13

[Javan Makhmali]

Created attachment 381206 [details]
Safari 13 crash log

[Javan Makhmali]

Created attachment 381207 [details]
Screen recording of the crash in Safari TP

[Javan Makhmali]

Created attachment 381208 [details]
Safari TP crash log

[Wenson Hsieh]

I’m able to reproduce this on macOS 10.15 as well.

[David Kilzer (:ddkilzer)]

<rdar://problem/49875932>

[Alexey Proskuryakov]

> I believe this issue occurs in Safari 12.1 as well.

Confirmed that too.

[Wenson Hsieh]

- (void)selectedRow:(NSTableView *)sender
{
    _dropdown->didSelectOption(_suggestions.at([sender selectedRow]));
}

-[NSTableView selectedRow] returns -1 here for some reason, causing us to crash.

[Wenson Hsieh]

(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

[Wenson Hsieh]

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.

[Wenson Hsieh]

Created attachment 382641 [details]
Repro w/o requiring replacement UI

To repro, load the page and click "Foo" in the suggestions dropdown.

[Wenson Hsieh]

Created attachment 382686 [details]
Fixes the crash

[WebKit Commit Bot]

Comment on attachment 382686 [details]
Fixes the crash

Clearing flags on attachment: 382686

Committed r252062: <https://trac.webkit.org/changeset/252062>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Javan Makhmali]

❤️

[Darin Adler]

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++?

[Wenson Hsieh]

(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...

[Wenson Hsieh]

Reopening to attach new patch.

[Wenson Hsieh]

Created attachment 383422 [details]
long -> unsigned

[EWS Watchlist]

Comment on attachment 383422 [details]
long -> unsigned

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

New failing tests:
fast/repaint/backgroundSizeRepaint.html

[EWS Watchlist]

Created attachment 383435 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 383422 [details]
long -> unsigned

Clearing flags on attachment: 383422

Committed r252719: <https://trac.webkit.org/changeset/252719>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.