| Summary: | Native text substitutions interfere with HTML <datalist> options resulting in crash | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Javan Makhmali <javan> |
| Component: | Forms | Assignee: | 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: | |||
Created attachment 381205 [details]
Screen recording of the crash in Safari 13
Created attachment 381206 [details]
Safari 13 crash log
Created attachment 381207 [details]
Screen recording of the crash in Safari TP
Created attachment 381208 [details]
Safari TP crash log
I’m able to reproduce this on macOS 10.15 as well. > I believe this issue occurs in Safari 12.1 as well.
Confirmed that too.
- (void)selectedRow:(NSTableView *)sender
{
_dropdown->didSelectOption(_suggestions.at([sender selectedRow]));
}
-[NSTableView selectedRow] returns -1 here for some reason, causing us to crash.
(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 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. Created attachment 382641 [details]
Repro w/o requiring replacement UI
To repro, load the page and click "Foo" in the suggestions dropdown.
Created attachment 382686 [details]
Fixes the crash
Comment on attachment 382686 [details] Fixes the crash Clearing flags on attachment: 382686 Committed r252062: <https://trac.webkit.org/changeset/252062> All reviewed patches have been landed. Closing bug. ❤️ 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++? (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... Reopening to attach new patch. Created attachment 383422 [details]
long -> unsigned
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 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
Comment on attachment 383422 [details] long -> unsigned Clearing flags on attachment: 383422 Committed r252719: <https://trac.webkit.org/changeset/252719> All reviewed patches have been landed. Closing bug. |
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.