Attachment 357844 Details for Bug 192912 – Patch

[patch] Patch

bug-192912-20181220114629.patch (text/plain), 3.40 KB, created by BJ Burg on 2018-12-20 11:46:29 PST

(hide)

Description:

Filename:

MIME Type:

Creator: BJ Burg

Created: 2018-12-20 11:46:29 PST

Size: 3.40 KB

patch

obsolete

>Subversion Revision: 239429
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 3f27a965cc6c3e2d2729bdf506e629e5267a8825..feee044a27d0a38f2e7991792a04511eb2a5c316 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,33 @@
>+2018-12-19  Brian Burg  <bburg@apple.com>
>+
>+        Unwanted page navigation after showing & dismissing contextual menu with control-click
>+        https://bugs.webkit.org/show_bug.cgi?id=192912
>+        <rdar://problem/46318508>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        After the conversion to use a mouse event queue, this behavior was observed rarely, especially
>+        when CPU is under load and lots of things are going on in the page. In other words, it's racy.
>+
>+        Based on NSEvent logging, it seems that when the system is under load, we simply take too long
>+        to enter the nested runloop that AppKit uses to handle events when the context menu is present.
>+        AppKit doesn't care whether or not the MouseDown triggered a nested runloop; on my machine it delivers
>+        the MouseUp event about 130ms after the MouseDown event. If we haven't show the context menu in
>+        that time, then the MouseUp is enqueued as a normal mouse event. If the Ctrl-click is on a link,
>+        then the MouseUp will be interpreted by EventHandler as a click event, which in the simplest case
>+        will initiate a main frame navigation. When the context menu is dismissed and the nested runloop
>+        is torn down, the navigation IPC message is handled in UIProcess and the page navigates away.
>+
>+        We can't do much to change AppKit's inherently racy behavior, but we can avoid processing
>+        mouse events that are delivered whilst we are processing the context menu-initiating event.
>+        From the WebProcess point of view, there is no race anymore because it does not receive the
>+        MouseUp event.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::showContextMenu):
>+        If new events come in, drop them on the floor. Based on testing, the nested runloop will discard
>+        the unpaired MouseUp event anyway, so this does not cause a change in behavior.
>+
> 2018-12-19  Chris Dumez  <cdumez@apple.com>
> 
>         wtf/Optional.h: move-constructor and move-assignment operator should disengage the value being moved from
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 4b01252ebb8fe927c5aaf4c570da8e455f8ecb6d..63a1f160b3658390ec28c6d8cb670f0e39110b0b 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -5450,6 +5450,12 @@ void WebPageProxy::showContextMenu(ContextMenuContextData&& contextMenuContextDa
>     // Showing a context menu runs a nested runloop, which can handle messages that cause |this| to get closed.
>     Ref<WebPageProxy> protect(*this);
> 
>+    // Discard any enqueued mouse events that have been delivered to the UIProcess whilst the WebProcess is still processing the
>+    // MouseDown event that triggered this ShowContextMenu message. This can happen if we take too long to enter the nested runloop.
>+    ASSERT(isProcessingMouseEvents());
>+    while (m_mouseEventQueue.size() > 1)
>+        m_mouseEventQueue.takeLast();
>+
>     m_activeContextMenuContextData = contextMenuContextData;
> 
>     m_activeContextMenu = pageClient().createContextMenuProxy(*this, WTFMove(contextMenuContextData), userData);

Flags:

timothy: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 192912: 357844