Bug 175315

Summary: WebDriver: timeout when JavaScript alert is shown in onload handler
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Updated patch bburg: review+

Description Carlos Garcia Campos 2017-08-08 01:42:04 PDT
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.
Comment 1 Carlos Garcia Campos 2017-08-08 02:02:12 PDT
Created attachment 317553 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2017-08-08 16:45:41 PDT
<rdar://problem/33788294>
Comment 3 BJ Burg 2017-08-08 16:51:13 PDT
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.
Comment 4 BJ Burg 2017-08-08 16:51:28 PDT
Is there any spec language for this issue?
Comment 5 Carlos Garcia Campos 2017-08-08 23:55:26 PDT
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!
Comment 6 Carlos Garcia Campos 2017-08-09 00:10:41 PDT
(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.
Comment 7 Carlos Garcia Campos 2017-08-14 08:24:22 PDT
Created attachment 318038 [details]
Updated patch
Comment 8 BJ Burg 2017-08-14 10:04:01 PDT
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|?
Comment 9 Carlos Garcia Campos 2017-08-15 00:12:25 PDT
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.
Comment 10 Carlos Garcia Campos 2017-08-15 00:17:49 PDT
Committed r220741: <http://trac.webkit.org/changeset/220741>