Bug 203116

Summary: Native text substitutions interfere with HTML <datalist> options resulting in crash
Product: WebKit Reporter: Javan Makhmali <javan>
Component: FormsAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, cdumez, commit-queue, darin, ddkilzer, ews-watchlist, megan_gardner, thorton, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: Mac   
OS: macOS 10.14   
Bug Depends on: 203777    
Bug Blocks:    
Attachments:
Description Flags
Minimal test case to reproduce <datalist> crash
none
Screen recording of the crash in Safari 13
none
Safari 13 crash log
none
Screen recording of the crash in Safari TP
none
Safari TP crash log
none
Repro w/o requiring replacement UI
none
Fixes the crash
none
long -> unsigned
none
Archive of layout-test-results from ews213 for win-future none

Javan Makhmali
Reported 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.
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
DetailsFormatted DiffDiff
long -> unsigned (6.54 KB, patch)
2019-11-12 19:25 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews213 for win-future (14.36 MB, application/zip)
2019-11-12 23:32 PST, EWS Watchlist 		no flags
Details
View All Add attachment proposed patch, testcase, etc.
Javan Makhmali
Comment 1 2019-10-17 11:36:52 PDT
Created attachment 381205 [details] Screen recording of the crash in Safari 13
Javan Makhmali
Comment 2 2019-10-17 11:37:24 PDT
Created attachment 381206 [details] Safari 13 crash log
Javan Makhmali
Comment 3 2019-10-17 11:37:54 PDT
Created attachment 381207 [details] Screen recording of the crash in Safari TP
Javan Makhmali
Comment 4 2019-10-17 11:38:11 PDT
Created attachment 381208 [details] Safari TP crash log
Wenson Hsieh
Comment 5 2019-10-18 09:55:21 PDT
I’m able to reproduce this on macOS 10.15 as well.
David Kilzer (:ddkilzer)
Comment 6 2019-10-18 14:52:14 PDT
<rdar://problem/49875932>
Alexey Proskuryakov
Comment 7 2019-10-19 11:55:20 PDT
> I believe this issue occurs in Safari 12.1 as well. Confirmed that too.
Wenson Hsieh
Comment 8 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.
Wenson Hsieh
Comment 9 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
Wenson Hsieh
Comment 10 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.
Wenson Hsieh
Comment 11 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.
Wenson Hsieh
Comment 12 2019-11-02 17:22:44 PDT
Created attachment 382686 [details] Fixes the crash
WebKit Commit Bot
Comment 13 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>
WebKit Commit Bot
Comment 14 2019-11-05 10:52:21 PST
All reviewed patches have been landed. Closing bug.
Javan Makhmali
Comment 15 2019-11-05 11:15:53 PST
❤️
Darin Adler
Comment 16 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++?
Wenson Hsieh
Comment 17 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...
Wenson Hsieh
Comment 18 2019-11-12 19:25:27 PST
Reopening to attach new patch.
Wenson Hsieh
Comment 19 2019-11-12 19:25:28 PST
Created attachment 383422 [details] long -> unsigned
EWS Watchlist
Comment 20 2019-11-12 23:32:07 PST Comment hidden (obsolete)
EWS Watchlist
Comment 21 2019-11-12 23:32:10 PST Comment hidden (obsolete)
WebKit Commit Bot
Comment 22 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>
WebKit Commit Bot
Comment 23 2019-11-20 17:08:39 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.