Bug 223017

Summary: Web Automation: Get Title command sometimes hangs inside evaluateJavaScriptFunction
Product: WebKit Reporter: BJ Burg <bburg>
Component: WebDriverAssignee: BJ Burg <bburg>
Status: NEW ---    
Severity: Normal CC: bburg, hi, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch v1.0 hi: review+, bburg: commit-queue-

Description BJ Burg 2021-03-09 23:39:41 PST
.
Comment 1 BJ Burg 2021-03-09 23:39:59 PST
<rdar://75239013>
Comment 2 BJ Burg 2021-03-09 23:48:28 PST
Created attachment 422805 [details]
Patch
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, { }))
        ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound);
```
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.