Bug 191155 – SimulatedInputDispatcher::transitionInputSourceToState() can reuse a moved-from completion handler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Zan Dobersek]

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.

[Zan Dobersek]

(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

[Radar WebKit Bug Importer]

<rdar://problem/45745569>

[Blaze Burg]

Zan, what wpt test cases does this affect?

[Zan Dobersek]

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 ()

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 356628 [details]
REST API and Automation Protocol trace (safaridriver)

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 356649 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 356649 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 356649 [details]
Patch

Clearing flags on attachment: 356649

Committed r239074: <https://trac.webkit.org/changeset/239074>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.