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.
Created attachment 317553 [details] Patch
<rdar://problem/33788294>
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.
Is there any spec language for this issue?
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!
(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.
Created attachment 318038 [details] Updated patch
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 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.
Committed r220741: <http://trac.webkit.org/changeset/220741>