Bug 191155

Summary: SimulatedInputDispatcher::transitionInputSourceToState() can reuse a moved-from completion handler
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebDriverAssignee: Brian Burg <bburg>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, cgarcia, commit-queue, joepeck, mcatanzaro, thorton, timothy, webkit-bug-importer, zan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=192424
Attachments:
Description Flags
REST API and Automation Protocol trace (safaridriver)
none
Patch none

Description Zan Dobersek 2018-11-01 10:04:22 PDT
In the SimulatedInputDispatcher::transitionInputSourceToState() method, the `eventDispatchFinished` completion handler can be incorrectly reused when multiple WebAutomationSession::simulateKeyboardInteraction() calls are done.

This can specifically happen when keyboard input simulation ends up adjusting to differences between the current and the new state of pressed virtual keys. As soon as two or more virtual keys are different between the states, the corresponding simulateKeyboardInteraction() invocations are dispatched. Problem is that while the first invocation moves out from the `eventDispatchFinished` variable that contains the completion handler, the second dispatch (and any additional ones) does the same -- on an already moved-out variable.

This results in a crash later in WebAutomationSession, when the completion handlers stored in the `m_pendingKeyboardEventsFlushedCallbacksPerPage` HashMap are retrieved from there and dispatched.

I don't know if this is a problem in any of the currently-imported WebDriver tests, but it was observed in existing WebDriver tests in the web-platform-tests suite.
Comment 1 Zan Dobersek 2018-11-01 10:05:42 PDT
(In reply to Zan Dobersek from comment #0)
> In the SimulatedInputDispatcher::transitionInputSourceToState() method, the
> `eventDispatchFinished` completion handler can be incorrectly reused when
> multiple WebAutomationSession::simulateKeyboardInteraction() calls are done.
> 

Here's the problematic code:
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.cpp#L299
Comment 2 Radar WebKit Bug Importer 2018-11-01 15:50:47 PDT
<rdar://problem/45745569>
Comment 3 Brian Burg 2018-11-01 15:50:55 PDT
Zan, what wpt test cases does this affect?
Comment 4 Zan Dobersek 2018-11-03 12:20:32 PDT
This crash occurs at least on the /webdriver/tests/perform_actions/key_modifiers.py test, as contained in the tip-of-tree wpt repository:
https://github.com/web-platform-tests/wpt/blob/master/webdriver/tests/perform_actions/key_modifiers.py

This happens on the `test_shift_modifier_and_non_printable_keys` subtest.

In the first meaningful SimulatedInputDispatcher::transitionInputSourceToState() invocation, the new state's pressed-virtual-keys set only contains the Shift key, as expected. In the following invocation, the new state's pressed-virtual-keys set only contains the Backspace key. This means the Backspace key press is simulated first, followed by the Shift key release. Upon that second simulation the `eventDispatchFinished` functor is already empty because of the first WTFMove().

Here's the backtrace that leads to the crash (which occurs later, when flushing pending events):

#0  0x00007f3c57ea3045 in WTF::Function<void (std::optional<WebKit::AutomationCommandError>)>::CallableWrapper<WebKit::WebAutomationSession::simulateKeyboardInteraction(WebKit::WebPageProxy&, Inspector::Protocol::Automation::KeyboardInteractionType, WTF::Variant<Inspector::Protocol::Automation::VirtualKey, char>&&, WTF::CompletionHandler<void (std::optional<WebKit::AutomationCommandError>)>&&)::{lambda(std::optional<WebKit::AutomationCommandError>)#1}>::call(std::optional<WebKit::AutomationCommandError>) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#1  0x00007f3c57eaba37 in WebKit::WebAutomationSession::keyboardEventsFlushedForPage(WebKit::WebPageProxy const&) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#2  0x00007f3c57dd8a02 in WebKit::WebPageProxy::didReceiveEvent(unsigned int, bool) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#3  0x00007f3c57c10af5 in WebKit::WebPageProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#4  0x00007f3c57d1aab3 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#5  0x00007f3c57e0400f in non-virtual thunk to WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) [clone .localalias.1033] () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#6  0x00007f3c57d1418f in IPC::Connection::dispatchMessage(IPC::Decoder&) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#7  0x00007f3c57d159fb in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#8  0x00007f3c57d16548 in IPC::Connection::dispatchIncomingMessages() () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#9  0x00007f3c5a1f6ffd in WTF::RunLoop::performWork() () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#10 0x00007f3c5a232f69 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/zan/Work/webkit/git/WebKitBuild/Release/lib/libWPEWebKit-0.1.so.2
#11 0x00007f3c51897888 in g_main_dispatch (context=0x56312a8bd040) at /home/zan/Work/webkit/git/WebKitBuild/DependenciesWPE/Source/glib-2.54.3/glib/gmain.c:3142
#12 g_main_context_dispatch (context=context@entry=0x56312a8bd040) at /home/zan/Work/webkit/git/WebKitBuild/DependenciesWPE/Source/glib-2.54.3/glib/gmain.c:3795
#13 0x00007f3c51897c48 in g_main_context_iterate (context=0x56312a8bd040, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /home/zan/Work/webkit/git/WebKitBuild/DependenciesWPE/Source/glib-2.54.3/glib/gmain.c:3868
#14 0x00007f3c51897f32 in g_main_loop_run (loop=0x56312a8bd590) at /home/zan/Work/webkit/git/WebKitBuild/DependenciesWPE/Source/glib-2.54.3/glib/gmain.c:4064
#15 0x0000563128aa69f5 in main ()
Comment 5 Brian Burg 2018-12-05 11:39:34 PST
Hi Zan, I looked into this some more. The code does indeed move the completion and operate in a loop. This doesn't affect safaridriver because it sends a differently formatted interaction sequence. safaridriver's sequence makes a separate state for each virtual key change. Thus, the problematic loop only ever executes once.

I'd prefer to add a return in the loop so it can't possibly simulate >1 action. Then, fix webkitdriver to emit more intermediate events.

It might be possible to use an accumulator-like thing so that such inputs are acceptable input for SimulatedInputDispatcher, but this code is already really async and it will be hard to get it right.
Comment 6 Brian Burg 2018-12-05 11:39:56 PST
Created attachment 356628 [details]
REST API and Automation Protocol trace (safaridriver)
Comment 7 Brian Burg 2018-12-05 11:41:29 PST
Comment on attachment 356628 [details]
REST API and Automation Protocol trace (safaridriver)

In the 2nd and 3rd from last state, you can see that pressedVirtualKeys only changes once at a time. I'd prefer we keep that semantics so that the ordering of virtual key presses is deterministic.
Comment 8 Brian Burg 2018-12-05 13:22:46 PST
Created attachment 356649 [details]
Patch
Comment 9 Joseph Pecoraro 2018-12-10 16:40:31 PST
Comment on attachment 356649 [details]
Patch

r=me
Comment 10 WebKit Commit Bot 2018-12-11 09:39:24 PST
Comment on attachment 356649 [details]
Patch

Clearing flags on attachment: 356649

Committed r239074: <https://trac.webkit.org/changeset/239074>
Comment 11 WebKit Commit Bot 2018-12-11 09:39:26 PST
All reviewed patches have been landed.  Closing bug.