WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
175315
WebDriver: timeout when JavaScript alert is shown in onload handler
https://bugs.webkit.org/show_bug.cgi?id=175315
Summary
WebDriver: timeout when JavaScript alert is shown in onload handler
Carlos Garcia Campos
Reported
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.
Attachments
Patch
(21.48 KB, patch)
2017-08-08 02:02 PDT
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Updated patch
(13.45 KB, patch)
2017-08-14 08:24 PDT
,
Carlos Garcia Campos
bburg
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2017-08-08 02:02:12 PDT
Created
attachment 317553
[details]
Patch
Radar WebKit Bug Importer
Comment 2
2017-08-08 16:45:41 PDT
<
rdar://problem/33788294
>
Blaze Burg
Comment 3
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.
Blaze Burg
Comment 4
2017-08-08 16:51:28 PDT
Is there any spec language for this issue?
Carlos Garcia Campos
Comment 5
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!
Carlos Garcia Campos
Comment 6
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.
Carlos Garcia Campos
Comment 7
2017-08-14 08:24:22 PDT
Created
attachment 318038
[details]
Updated patch
Blaze Burg
Comment 8
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|?
Carlos Garcia Campos
Comment 9
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.
Carlos Garcia Campos
Comment 10
2017-08-15 00:17:49 PDT
Committed
r220741
: <
http://trac.webkit.org/changeset/220741
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug