Native text substitutions interfere with HTML <datalist> options resulting in crash
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 18:52:12 +0000 (18:52 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2019 18:52:12 +0000 (18:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=203116
<rdar://problem/49875932>

Reviewed by Tim Horton.

Source/WebKit:

On macOS, an NSTableView inside a separate window is used to render suggestions when showing a datalist. The
crash happens when this table view is reloaded while clicking a datalist suggestion; that is, if -[NSTableView
reloadData] is invoked after the user sends a platform MouseDown event on the table view cell but before the
MouseUp is received, the selected row of the table view will be -1 when the action, `-selectedRow:`, is invoked.

In this particular case, the table view reload is triggered as a result of hiding the autocorrection bubble on
macOS, thereby committing the alternative text suggestion and changing the value of the text field via an
editing command.

To avoid crashing, we simply make `-selectedRow:` robust in the case where the index is invalid.

Test: fast/forms/datalist/datalist-click-crash.html

* UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:
(-[WKDataListSuggestionsController selectedRow:]):

Tools:

Add a new testing hook to wait for datalist suggestions to show up and choose the suggestion at the given index.

* DumpRenderTree/mac/UIScriptControllerMac.h:
* DumpRenderTree/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::activateDataListSuggestion):
* TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::activateDataListSuggestion):
* WebKitTestRunner/ios/UIScriptControllerIOS.h:
* WebKitTestRunner/ios/UIScriptControllerIOS.mm:
(WTR::UIScriptControllerIOS::activateDataListSuggestion):
* WebKitTestRunner/mac/UIScriptControllerMac.h:
* WebKitTestRunner/mac/UIScriptControllerMac.mm:
(WTR::UIScriptControllerMac::isShowingDataListSuggestions const):
(WTR::UIScriptControllerMac::activateDataListSuggestion):

Dig through the view hierarchy of the NSWindow subclass used to show datalist suggestions for the table view
containing the suggestions; then, select the given row, and invoke the action on the target.

(WTR::UIScriptControllerMac::dataListSuggestionsTableView const):

LayoutTests:

Add a new layout test to exercise the crash.

* fast/forms/datalist/datalist-click-crash-expected.txt: Added.
* fast/forms/datalist/datalist-click-crash.html: Added.
* resources/ui-helper.js:
(window.UIHelper.activateDataListSuggestion):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@252062 268f45cc-cd09-0410-ab3c-d52691b4dbfc

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/forms/datalist/datalist-click-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/datalist/datalist-click-crash.html [new file with mode: 0644]
LayoutTests/resources/ui-helper.js
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/mac/WebDataListSuggestionsDropdownMac.mm
Tools/ChangeLog
Tools/DumpRenderTree/mac/UIScriptControllerMac.h
Tools/DumpRenderTree/mac/UIScriptControllerMac.mm
Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl
Tools/TestRunnerShared/UIScriptContext/UIScriptController.h
Tools/WebKitTestRunner/ios/UIScriptControllerIOS.h
Tools/WebKitTestRunner/ios/UIScriptControllerIOS.mm
Tools/WebKitTestRunner/mac/UIScriptControllerMac.h
Tools/WebKitTestRunner/mac/UIScriptControllerMac.mm

index 691505d..01446ec 100644 (file)
@@ -1,3 +1,18 @@
+2019-11-05  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Native text substitutions interfere with HTML <datalist> options resulting in crash
+        https://bugs.webkit.org/show_bug.cgi?id=203116
+        <rdar://problem/49875932>
+
+        Reviewed by Tim Horton.
+
+        Add a new layout test to exercise the crash.
+
+        * fast/forms/datalist/datalist-click-crash-expected.txt: Added.
+        * fast/forms/datalist/datalist-click-crash.html: Added.
+        * resources/ui-helper.js:
+        (window.UIHelper.activateDataListSuggestion):
+
 2019-11-05  Andy Estes  <aestes@apple.com>
 
         ApplePaySession should never prevent entering the back/forward cache
diff --git a/LayoutTests/fast/forms/datalist/datalist-click-crash-expected.txt b/LayoutTests/fast/forms/datalist/datalist-click-crash-expected.txt
new file mode 100644 (file)
index 0000000..aa09541
--- /dev/null
@@ -0,0 +1,11 @@
+This test verifies that we don't crash when selecting a datalist suggestion, if the suggestions list is reloaded during the click (after mousedown and before mouseup). To manually run the test, click 'Foo' in the datalist dropdown. The application should not crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS Did not crash.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+
diff --git a/LayoutTests/fast/forms/datalist/datalist-click-crash.html b/LayoutTests/fast/forms/datalist/datalist-click-crash.html
new file mode 100644 (file)
index 0000000..eafa975
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script src="../../../resources/ui-helper.js"></script>
+    <script src="../../../resources/js-test.js"></script>
+</head>
+<body>
+<input list="foobar" />
+<datalist id="foobar">
+    <select>
+        <option>Foo</option>
+    </select>
+</datalist>
+<br>
+</body>
+<script>
+jsTestIsAsync = true;
+
+addEventListener("load", async function() {
+    description("This test verifies that we don't crash when selecting a datalist suggestion, if the suggestions list is reloaded during the click (after mousedown and before mouseup). To manually run the test, click 'Foo' in the datalist dropdown. The application should not crash.");
+    if (!window.testRunner)
+        return;
+
+    await UIHelper.activateDataListSuggestion(0);
+    document.querySelectorAll("input, select").forEach(element => element.remove());
+    testPassed("Did not crash.");
+    finishJSTest();
+});
+
+(async function() {
+    while (true) {
+        const input = document.querySelector("input");
+        if (!input)
+            return;
+
+        input.focus();
+        document.execCommand("InsertText", true, "");
+        await new Promise(requestAnimationFrame);
+    }
+})();
+</script>
+</html>
index 3aaba29..6e95a87 100644 (file)
@@ -666,6 +666,13 @@ window.UIHelper = class UIHelper {
         });
     }
 
+    static activateDataListSuggestion(index) {
+        const script = `uiController.activateDataListSuggestion(${index}, () => {
+            uiController.uiScriptComplete("");
+        });`;
+        return new Promise(resolve => testRunner.runUIScript(script, resolve));
+    }
+
     static isShowingDataListSuggestions()
     {
         return new Promise(resolve => {
index b23f436..17a4325 100644 (file)
@@ -1,3 +1,27 @@
+2019-11-05  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Native text substitutions interfere with HTML <datalist> options resulting in crash
+        https://bugs.webkit.org/show_bug.cgi?id=203116
+        <rdar://problem/49875932>
+
+        Reviewed by Tim Horton.
+
+        On macOS, an NSTableView inside a separate window is used to render suggestions when showing a datalist. The
+        crash happens when this table view is reloaded while clicking a datalist suggestion; that is, if -[NSTableView
+        reloadData] is invoked after the user sends a platform MouseDown event on the table view cell but before the
+        MouseUp is received, the selected row of the table view will be -1 when the action, `-selectedRow:`, is invoked.
+
+        In this particular case, the table view reload is triggered as a result of hiding the autocorrection bubble on
+        macOS, thereby committing the alternative text suggestion and changing the value of the text field via an
+        editing command.
+
+        To avoid crashing, we simply make `-selectedRow:` robust in the case where the index is invalid.
+
+        Test: fast/forms/datalist/datalist-click-crash.html
+
+        * UIProcess/mac/WebDataListSuggestionsDropdownMac.mm:
+        (-[WKDataListSuggestionsController selectedRow:]):
+
 2019-11-05  Sihui Liu  <sihui_liu@apple.com>
 
         REGRESSION (r250754): web page using IDBIndex doesn't load occasionally
index 5d02e4b..39a09e5 100644 (file)
@@ -402,7 +402,11 @@ void WebDataListSuggestionsDropdownMac::close()
 
 - (void)selectedRow:(NSTableView *)sender
 {
-    _dropdown->didSelectOption(_suggestions.at([sender selectedRow]));
+    auto selectedString = self.currentSelectedString;
+    if (!selectedString)
+        return;
+
+    _dropdown->didSelectOption(selectedString);
 }
 
 - (NSInteger)numberOfRowsInTableView:(NSTableView *)tableView
index 308ca55..3193960 100644 (file)
@@ -1,3 +1,32 @@
+2019-11-05  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Native text substitutions interfere with HTML <datalist> options resulting in crash
+        https://bugs.webkit.org/show_bug.cgi?id=203116
+        <rdar://problem/49875932>
+
+        Reviewed by Tim Horton.
+
+        Add a new testing hook to wait for datalist suggestions to show up and choose the suggestion at the given index.
+
+        * DumpRenderTree/mac/UIScriptControllerMac.h:
+        * DumpRenderTree/mac/UIScriptControllerMac.mm:
+        (WTR::UIScriptControllerMac::activateDataListSuggestion):
+        * TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
+        * TestRunnerShared/UIScriptContext/UIScriptController.h:
+        (WTR::UIScriptController::activateDataListSuggestion):
+        * WebKitTestRunner/ios/UIScriptControllerIOS.h:
+        * WebKitTestRunner/ios/UIScriptControllerIOS.mm:
+        (WTR::UIScriptControllerIOS::activateDataListSuggestion):
+        * WebKitTestRunner/mac/UIScriptControllerMac.h:
+        * WebKitTestRunner/mac/UIScriptControllerMac.mm:
+        (WTR::UIScriptControllerMac::isShowingDataListSuggestions const):
+        (WTR::UIScriptControllerMac::activateDataListSuggestion):
+
+        Dig through the view hierarchy of the NSWindow subclass used to show datalist suggestions for the table view
+        containing the suggestions; then, select the given row, and invoke the action on the target.
+
+        (WTR::UIScriptControllerMac::dataListSuggestionsTableView const):
+
 2019-11-05  Daniel Bates  <dabates@apple.com>
 
         TestWebKitAPI.WebKit.DocumentEditingContext is failing on iPad
index 0e7ec28..678bc21 100644 (file)
@@ -50,6 +50,7 @@ public:
     void simulateAccessibilitySettingsChangeNotification(JSValueRef) override;
     NSUndoManager *platformUndoManager() const override;
     void copyText(JSStringRef) override;
+    void activateDataListSuggestion(long, JSValueRef) override;
 };
 
 }
index 996fb92..2e23c6e 100644 (file)
@@ -100,6 +100,19 @@ JSObjectRef UIScriptControllerMac::contentsOfUserInterfaceItem(JSStringRef inter
 #endif
 }
 
+void UIScriptControllerMac::activateDataListSuggestion(long index, JSValueRef callback)
+{
+    // FIXME: Not implemented.
+    UNUSED_PARAM(index);
+
+    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
+    dispatch_async(dispatch_get_main_queue(), ^{
+        if (!m_context)
+            return;
+        m_context->asyncTaskComplete(callbackID);
+    });
+}
+
 void UIScriptControllerMac::overridePreference(JSStringRef preferenceRef, JSStringRef valueRef)
 {
     WebPreferences *preferences = mainFrame.webView.preferences;
index 5520cc5..a9c0f57 100644 (file)
@@ -219,6 +219,7 @@ interface UIScriptController {
 
     // <datalist>
     readonly attribute boolean isShowingDataListSuggestions;
+    void activateDataListSuggestion(unsigned long index, object callback);
 
     void keyboardAccessoryBarNext();
     void keyboardAccessoryBarPrevious();
index 3d032aa..4e0c75e 100644 (file)
@@ -201,6 +201,7 @@ public:
     virtual JSObjectRef calendarType() const { notImplemented(); return nullptr; }
     virtual void setDefaultCalendarType(JSStringRef calendarIdentifier) { notImplemented(); }
     virtual JSObjectRef inputViewBounds() const { notImplemented(); return nullptr; }
+    virtual void activateDataListSuggestion(long, JSValueRef) { notImplemented(); }
 
     // Share Sheet
 
index a6cadbf..5a04570 100644 (file)
@@ -129,6 +129,7 @@ public:
     void beginBackSwipe(JSValueRef) override;
     void completeBackSwipe(JSValueRef) override;
     bool isShowingDataListSuggestions() const override;
+    void activateDataListSuggestion(long, JSValueRef) override;
     void drawSquareInEditableImage() override;
     long numberOfStrokesInEditableImage() override;
     void setKeyboardInputModeIdentifier(JSStringRef) override;
index 44062a8..27f3880 100644 (file)
@@ -1032,6 +1032,19 @@ void UIScriptControllerIOS::completeBackSwipe(JSValueRef callback)
     [webView() _completeBackSwipeForTesting];
 }
 
+void UIScriptControllerIOS::activateDataListSuggestion(long index, JSValueRef callback)
+{
+    // FIXME: Not implemented.
+    UNUSED_PARAM(index);
+
+    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
+    dispatch_async(dispatch_get_main_queue(), ^{
+        if (!m_context)
+            return;
+        m_context->asyncTaskComplete(callbackID);
+    });
+}
+
 bool UIScriptControllerIOS::isShowingDataListSuggestions() const
 {
     Class remoteKeyboardWindowClass = NSClassFromString(@"UIRemoteKeyboardWindow");
index 808c05c..18bd378 100644 (file)
@@ -43,6 +43,7 @@ public:
     double zoomScale() const override;
     void simulateAccessibilitySettingsChangeNotification(JSValueRef) override;
     bool isShowingDataListSuggestions() const override;
+    void activateDataListSuggestion(long index, JSValueRef callback) override;
     void beginBackSwipe(JSValueRef) override;
     void completeBackSwipe(JSValueRef) override;
     void playBackEventStream(JSStringRef, JSValueRef) override;
@@ -57,6 +58,9 @@ public:
     void chooseMenuAction(JSStringRef, JSValueRef) override;
 
     void activateAtPoint(long x, long y, JSValueRef callback) override;
+
+private:
+    NSTableView *dataListSuggestionsTableView() const;
 };
 
 } // namespace WTR
index c20efd9..1cfefee 100644 (file)
@@ -28,6 +28,7 @@
 
 #import "EventSenderProxy.h"
 #import "EventSerializerMac.h"
+#import "PlatformViewHelpers.h"
 #import "PlatformWebView.h"
 #import "SharedEventStreamsMac.h"
 #import "TestController.h"
@@ -96,11 +97,38 @@ void UIScriptControllerMac::simulateAccessibilitySettingsChangeNotification(JSVa
 
 bool UIScriptControllerMac::isShowingDataListSuggestions() const
 {
+    return dataListSuggestionsTableView();
+}
+
+void UIScriptControllerMac::activateDataListSuggestion(long index, JSValueRef callback)
+{
+    unsigned callbackID = m_context->prepareForAsyncTask(callback, CallbackTypeNonPersistent);
+
+    RetainPtr<NSTableView> table;
+    do {
+        table = dataListSuggestionsTableView();
+    } while (index >= [table numberOfRows] && [[NSRunLoop currentRunLoop] runMode:NSDefaultRunLoopMode beforeDate:[NSDate distantPast]]);
+
+    [table selectRowIndexes:[NSIndexSet indexSetWithIndex:index] byExtendingSelection:NO];
+
+    // Send the action after a short delay to simulate normal user interaction.
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.05 * NSEC_PER_SEC)), dispatch_get_main_queue(), [this, protectedThis = makeRefPtr(*this), callbackID, table] {
+        if ([table window])
+            [table sendAction:[table action] to:[table target]];
+
+        if (!m_context)
+            return;
+        m_context->asyncTaskComplete(callbackID);
+    });
+}
+
+NSTableView *UIScriptControllerMac::dataListSuggestionsTableView() const
+{
     for (NSWindow *childWindow in webView().window.childWindows) {
         if ([childWindow isKindOfClass:NSClassFromString(@"WKDataListSuggestionWindow")])
-            return true;
+            return (NSTableView *)[findAllViewsInHierarchyOfType(childWindow.contentView, NSClassFromString(@"WKDataListSuggestionTableView")) firstObject];
     }
-    return false;
+    return nil;
 }
 
 static void playBackEvents(WKWebView *webView, UIScriptContext *context, NSString *eventStream, JSValueRef callback)