Bug 223017 - Web Automation: Get Title command sometimes hangs inside evaluateJavaScriptFunction
Summary: Web Automation: Get Title command sometimes hangs inside evaluateJavaScriptFu...
Status: NEW
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: 2021-03-09 23:39 PST by BJ Burg
Modified: 2021-03-11 00:18 PST (History)
3 users (show)

See Also:

Patch (3.43 KB, patch)
2021-03-09 23:48 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Patch v1.0 (3.43 KB, patch)
2021-03-09 23:48 PST, BJ Burg
drousso: review+
bburg: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2021-03-09 23:39:41 PST
Comment 1 BJ Burg 2021-03-09 23:39:59 PST
Comment 2 BJ Burg 2021-03-09 23:48:28 PST
Created attachment 422805 [details]
Comment 3 BJ Burg 2021-03-09 23:48:52 PST
Created attachment 422806 [details]
Patch v1.0
Comment 4 Devin Rousso 2021-03-10 10:28:15 PST
Comment on attachment 422806 [details]
Patch v1.0

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

r=me, nice fix :)

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:771
> +        // would no longer be valid and the command would also fail also with WindowNotFound, just earlier.

"also fail also"

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:774
> +            for (auto key : copyToVector(m_evaluateJavaScriptFunctionCallbacks.keys())) {
> +                auto callback = m_evaluateJavaScriptFunctionCallbacks.take(key);

Rather than `copyToVector`, can you `std::exchange`?
    for (auto&& [key, callback] : std::exchange(m_evaluateJavaScriptFunctionCallbacks, { }))
I think you could also drop the `isEmpty` check if you do this too.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:775
> +                ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound);

I believe this will `return`.  Based on your comment it sounds like we want that, but I just want to make sure.
Comment 5 BJ Burg 2021-03-11 00:18:03 PST
Looking at this more, I think the bug is actually that WebAutomationSessionProxy is not seeing these main frame navigations. It already has cancellation logic. I'm going to look into this with a debugger to see if it ever gets triggered.