Bug 184462

Summary: Web Automation: simulated mouse interactions should not be done until associated DOM events have been dispatched
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebDriverAssignee: BJ Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, cgarcia, commit-queue, joepeck, lforschler, simon.fraser, thorton, timothy, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 184747    
Bug Blocks: 184603    
Attachments:
Description Flags
Patch
none
Proposed Fix
none
Revised Patch none

Description BJ Burg 2018-04-10 12:28:09 PDT
We do this properly for keyboard events but not mouse events.
Comment 1 Radar WebKit Bug Importer 2018-04-10 12:39:47 PDT
<rdar://problem/39323336>
Comment 2 BJ Burg 2018-04-11 17:26:23 PDT
Created attachment 337755 [details]
Patch
Comment 3 Carlos Garcia Campos 2018-04-11 23:12:40 PDT
Comment on attachment 337755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337755&action=review

> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:82
> +    std::optional<String> message { std::nullopt };

I don't think we need to initialize this, it's already nullopt. It's a bit weird to have here some members, then the constructors and then another member, could we move this below the constructors?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:84
> +    AutomationCommandError(Inspector::Protocol::Automation::ErrorMessage type)

explicit

> Source/WebKit/UIProcess/WebPageProxy.cpp:1882
> +    m_mouseEventQueue.append(event);

I don't think we want to append events if the page was closed, I would early return if !isValid()

> Source/WebKit/UIProcess/WebPageProxy.cpp:1894
> +    NativeWebMouseEvent event = m_mouseEventQueue.first();

Do we want to copy the event here? I guess it could be const NativeWebMouseEvent&

> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
> +    auto& event = m_mouseEventQueue.first();
> +    if (event.type() != WebEvent::Type::MouseDown)
> +        return nullptr;

What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5209
> +        } else {
> +            if (auto* automationSession = process().processPool().automationSession())

else if
Comment 4 BJ Burg 2018-04-16 12:44:58 PDT
Comment on attachment 337755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337755&action=review

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:82
>> +    std::optional<String> message { std::nullopt };
> 
> I don't think we need to initialize this, it's already nullopt. It's a bit weird to have here some members, then the constructors and then another member, could we move this below the constructors?

I'll clean this up. I just kept appending stuff as I went.

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.h:84
>> +    AutomationCommandError(Inspector::Protocol::Automation::ErrorMessage type)
> 
> explicit

Good catch.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1882
>> +    m_mouseEventQueue.append(event);
> 
> I don't think we want to append events if the page was closed, I would early return if !isValid()

OK.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:1894
>> +    NativeWebMouseEvent event = m_mouseEventQueue.first();
> 
> Do we want to copy the event here? I guess it could be const NativeWebMouseEvent&

I'll make it take a reference.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
>> +        return nullptr;
> 
> What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?

I tested this and nothing seemed broken. I looked into this more after reading your question. The chain is as follows:

- Enqueue mousedown event
- UIProcess sends mousedown to WebProcess
- EventDispatcher tries to dispatch
-> HTMLSelectElement::menuListDefaultEventHandler
-> RenderMenuList::showPopup
-> WebPopupMenu::show
-> IPC to UIProcess
-> WebPopupMenuProxy(Mac)::showPopupMenu

At this point WebProcess is still processing the mousedown, but now execution is back in UIProcess. It will spin a nested run loop to handle the NSEvents being sent to the dropdown. When one is selected, the native control eats the subsequent mouseup that fires over the opened dialog, so no mouseup is enqueued into m_mouseEventQueue once it is dismissed. When this returns to WebProcess it makes a fake event and dispatches it within the DOM only (i.e., not a native event).

So I don't think there is anything to change here.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5209
>> +            if (auto* automationSession = process().processPool().automationSession())
> 
> else if

OK
Comment 5 BJ Burg 2018-04-16 13:03:46 PDT
Created attachment 338027 [details]
Proposed Fix
Comment 6 Carlos Garcia Campos 2018-04-17 02:45:44 PDT
(In reply to Brian Burg from comment #4)
> 
> >> Source/WebKit/UIProcess/WebPageProxy.cpp:4785
> >> +        return nullptr;
> > 
> > What happens if there's a mouse move after the mouse down? This will return nullptr, but current code only resets the currently processed mouse down on mouse Up. Or is it guaranteed that mouse down is the only event in the queue?
> 
> I tested this and nothing seemed broken. I looked into this more after
> reading your question. The chain is as follows:
> 
> - Enqueue mousedown event
> - UIProcess sends mousedown to WebProcess
> - EventDispatcher tries to dispatch
> -> HTMLSelectElement::menuListDefaultEventHandler
> -> RenderMenuList::showPopup
> -> WebPopupMenu::show
> -> IPC to UIProcess
> -> WebPopupMenuProxy(Mac)::showPopupMenu
> 
> At this point WebProcess is still processing the mousedown, but now
> execution is back in UIProcess. It will spin a nested run loop to handle the
> NSEvents being sent to the dropdown. When one is selected, the native
> control eats the subsequent mouseup that fires over the opened dialog, so no
> mouseup is enqueued into m_mouseEventQueue once it is dismissed. When this
> returns to WebProcess it makes a fake event and dispatches it within the DOM
> only (i.e., not a native event).
> 
> So I don't think there is anything to change here.

You are right, I missed the point that events are appended but here we always get the first one, not the last one, so adding more vents doesn't affect it.
Comment 7 Carlos Garcia Campos 2018-04-17 02:50:19 PDT
Comment on attachment 338027 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=338027&action=review

This looks good to me, but needs to be approved by a WebKit2 owner.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:56
> +String AutomationCommandError::toProtocolString()

This could be const, I guess.
Comment 8 Tim Horton 2018-04-17 13:51:33 PDT
Comment on attachment 338027 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=338027&action=review

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1440
> +                    .setY(y - page->topContentInset())

Somehow this topContentInset() is surprising -- do we not have functions to map between whatever relevant coordinate spaces are involved?
Comment 9 BJ Burg 2018-04-17 14:21:19 PDT
Comment on attachment 338027 [details]
Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=338027&action=review

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1440
>> +                    .setY(y - page->topContentInset())
> 
> Somehow this topContentInset() is surprising -- do we not have functions to map between whatever relevant coordinate spaces are involved?

We do, in WebProcess. I don't know the equivalent from UIProcess. We do this in WebAutomationSessionProxy::computeElementLayout to go from client iframe to client main frame coordinates.
Comment 10 BJ Burg 2018-04-17 17:46:10 PDT
Committed r230743: <https://trac.webkit.org/changeset/230743>
Comment 11 WebKit Commit Bot 2018-04-18 12:05:26 PDT
Re-opened since this is blocked by bug 184747
Comment 12 BJ Burg 2018-04-19 12:35:24 PDT
(In reply to WebKit Commit Bot from comment #11)
> Re-opened since this is blocked by bug 184747

This was rolled out because clicking apparently hangs on some systems. Upon further reflection, I wrote and tested the patch with a physical USB mouse but not a trackpad. Tapping on a trackpad generates MouseForce* events and enqueues them, but we don't dequeue them in WebPageProxy::didReceiveEvent. So the fix is to move MouseForce* cases to also be handled by m_mouseEventQueue machinery.
Comment 13 BJ Burg 2018-04-19 12:51:05 PDT
Created attachment 338352 [details]
Revised Patch
Comment 14 WebKit Commit Bot 2018-04-19 13:37:37 PDT
Comment on attachment 338352 [details]
Revised Patch

Clearing flags on attachment: 338352

Committed r230817: <https://trac.webkit.org/changeset/230817>
Comment 15 WebKit Commit Bot 2018-04-19 13:37:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Simon Fraser (smfr) 2018-05-04 14:15:58 PDT
Comment on attachment 338352 [details]
Revised Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338352&action=review

> Source/WebKit/UIProcess/WebPageProxy.cpp:296
> +static const char* webMouseEventTypeString(WebEvent::Type type)

This could have been:

TextStream& operator<<(TextStream& ts, WebEvent::Type type)

and then you'd use LOG_WITH_STREAM below.