Bug 175315 – WebDriver: timeout when JavaScript alert is shown in onload handler

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Carlos Garcia Campos]

When a JavaScript alert is shown in an onload handler, the alert prevents the load from finishing, so navigation commands or any other command for which we wait for navigation to complete end up timing out. There are two selenium tests covering this that are currently timing out: testShouldHandleAlertOnPageLoad and testShouldHandleAlertOnPageLoadUsingGet.

[Carlos Garcia Campos]

Created attachment 317553 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/33788294>

[Blaze Burg]

Comment on attachment 317553 [details]
Patch

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

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:412
> +    bool shouldFailWithUnexpectedAlertOpen = page->pageLoadState().isLoading() && m_client->isShowingJavaScriptDialogOnPage(*this, *page);

Should this depend on the page load strategy? If we only care about DOMContentLoaded then this seems incorrect.

[Blaze Burg]

Is there any spec language for this issue?

[Carlos Garcia Campos]

Comment on attachment 317553 [details]
Patch

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

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:412
>> +    bool shouldFailWithUnexpectedAlertOpen = page->pageLoadState().isLoading() && m_client->isShowingJavaScriptDialogOnPage(*this, *page);
> 
> Should this depend on the page load strategy? If we only care about DOMContentLoaded then this seems incorrect.

Good point!

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #4)
> Is there any spec language for this issue?

The spec doesn't mention the case of prompts spawned in onload specifically, but when waiting for navigations to complete it says:

6. Wait for the the current browsing context’s document readiness state to reach readiness target, or for the session page load timeout to pass, whichever occurs sooner.

7. If the previous step completed by the session page load timeout being reached and the browser does not have an active user prompt, return error with error code timeout.

So, according to the spec we should wait for the timeout to happen and only report timeout error if there isn't a prompt. In case of normal page load strategy we know it's going to timeout, so I don't think it makes sense to wait, note that the default timeout is 5 minutes! The spec doesn't say we should report unexpected alert in this case, though. So, maybe we can just return without error, because the next command will try to handle prompts anyway.

[Carlos Garcia Campos]

Created attachment 318038 [details]
Updated patch

[Blaze Burg]

Comment on attachment 318038 [details]
Updated patch

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

r=me

> Source/WebKit/ChangeLog:14
> +        expects an alert it will just work, otherwise it will fail with UnexpectedAlertOpen error when trying ot handle

Nit: trying to

> Source/WebKit/ChangeLog:29
> +        wewait until the next run loop iteration to give time for the client to show the dialog, then check if page is

Nit: we wait

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:415
> +    // we return without waiting since we know it will timeout or sure. We want to check

Nit: for sure

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:417
> +    bool shouldTimeoutDueToUnexpectedAlert = pageLoadStrategy.value() == Inspector::Protocol::Automation::PageLoadStrategy::Normal

Nice.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:504
> +        for (auto& process : m_processPool->processes()) {

This could be its own static method to keep the loop simple.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:512
> +            callback->sendSuccess(InspectorObject::create());

Will this return "data null" in the driver endpoint per step 8?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:532
> +        if (!page->isValid() || !page->pageLoadState().isLoading() || !m_client->isShowingJavaScriptDialogOnPage(*this, page))

What if the session was destroyed? Do we need a guard for that and a protectRef for |this|?

[Carlos Garcia Campos]

Comment on attachment 318038 [details]
Updated patch

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

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:512
>> +            callback->sendSuccess(InspectorObject::create());
> 
> Will this return "data null" in the driver endpoint per step 8?

Right

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:532
>> +        if (!page->isValid() || !page->pageLoadState().isLoading() || !m_client->isShowingJavaScriptDialogOnPage(*this, page))
> 
> What if the session was destroyed? Do we need a guard for that and a protectRef for |this|?

Yes, this is already protected in the lambda, but we need to null check the client.

[Carlos Garcia Campos]

Committed r220741: <http://trac.webkit.org/changeset/220741>