Bug 191155 - SimulatedInputDispatcher::transitionInputSourceToState() can reuse a moved-from completion handler
Summary: SimulatedInputDispatcher::transitionInputSourceToState() can reuse a moved-fr...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: BJ Burg
Keywords: InRadar
Depends on:
Reported: 2018-11-01 10:04 PDT by Zan Dobersek
Modified: 2018-12-11 09:39 PST (History)
9 users (show)

See Also:

REST API and Automation Protocol trace (safaridriver) (3.01 KB, text/plain)
2018-12-05 11:39 PST, BJ Burg
no flags Details
Patch (3.57 KB, patch)
2018-12-05 13:22 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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:
Comment 2 Radar WebKit Bug Importer 2018-11-01 15:50:47 PDT
Comment 3 BJ 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:

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 BJ 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 BJ Burg 2018-12-05 11:39:56 PST
Created attachment 356628 [details]
REST API and Automation Protocol trace (safaridriver)
Comment 7 BJ 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 BJ Burg 2018-12-05 13:22:46 PST
Created attachment 356649 [details]
Comment 9 Joseph Pecoraro 2018-12-10 16:40:31 PST
Comment on attachment 356649 [details]

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

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.