Bug 174672 – WebDriver: implement page load timeout

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

https://www.w3.org/TR/webdriver/#dfn-session-page-load-timeout


selenium/webdriver/common/page_load_timeout_tests.py FF
================================================================================== short test summary info ===================================================================================
FAIL selenium/webdriver/common/page_load_timeout_tests.py::testShouldTimeoutOnPageLoadTakingTooLong[WebKitGTK]
FAIL selenium/webdriver/common/page_load_timeout_tests.py::testClickShouldTimeout[WebKitGTK]

========================================================================================== FAILURES ==========================================================================================
____________________________________________________________________ testShouldTimeoutOnPageLoadTakingTooLong[WebKitGTK] _____________________________________________________________________

driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="6f75f398-cd74-4b81-9f3b-0a72079c3323")>, pages = <conftest.Pages object at 0x7f65ac738190>

    @pytest.mark.xfail_marionette(
        reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1309231')
    @pytest.mark.xfail_phantomjs(
        reason='PhantomJS does not implement page load timeouts')
    def testShouldTimeoutOnPageLoadTakingTooLong(driver, pages):
        driver.set_page_load_timeout(0.01)
        with pytest.raises(TimeoutException):
>           pages.load("simpleTest.html")
E           Failed: DID NOT RAISE <class 'selenium.common.exceptions.TimeoutException'>

selenium/webdriver/common/page_load_timeout_tests.py:30: Failed

_____________________________________________________________________________ testClickShouldTimeout[WebKitGTK] ______________________________________________________________________________

driver = <selenium.webdriver.webkitgtk.webdriver.WebDriver (session="c0147747-bb31-4599-925f-87f1079cdaf6")>, pages = <conftest.Pages object at 0x7f65ac952450>

    @pytest.mark.xfail_marionette(
        reason='https://bugzilla.mozilla.org/show_bug.cgi?id=1309231')
    @pytest.mark.xfail_phantomjs(
        reason='PhantomJS does not implement page load timeouts')
    def testClickShouldTimeout(driver, pages):
        pages.load("simpleTest.html")
        driver.set_page_load_timeout(0.01)
        with pytest.raises(TimeoutException):
>           driver.find_element_by_id("multilinelink").click()
E           Failed: DID NOT RAISE <class 'selenium.common.exceptions.TimeoutException'>

selenium/webdriver/common/page_load_timeout_tests.py:41: Failed

[Carlos Garcia Campos]

Created attachment 315982 [details]
Patch

Note that this fixes testShouldTimeoutOnPageLoadTakingTooLong but not testClickShouldTimeout. The wait after a click is racy. Right after the click happens in the UI process, the page load hasn't started yet, so we can't start the wait there like we do with navigation commands, and when the command returns and thr web driver asks the browser to wait, the load has already happened.  I don't think it's possible to solve that, and I don't think it's worth it either, since the test case is actually testing a very weird case (very small timeout and very fast load).

[Joseph Pecoraro]

Comment on attachment 315982 [details]
Patch

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

Nice! I'll let Brian review. If I remember correctly this could address a number of the failures in the Selenium test suite that expected some timeout.

> Source/WebDriver/ChangeLog:9
> +        commands. Also fix the setTimeouts command that was still using the legacy name of rthe page load timeout,

Typo: "of rthe" => "for the"

[Carlos Garcia Campos]

(In reply to Joseph Pecoraro from comment #2)
> Comment on attachment 315982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315982&action=review
> 
> Nice! I'll let Brian review. If I remember correctly this could address a
> number of the failures in the Selenium test suite that expected some timeout.

Not really, only the two tests I mentioned before set a page load timeout:

$ git grep set_page_load_timeout
page_load_timeout_tests.py:    driver.set_page_load_timeout(0.01)
page_load_timeout_tests.py:    driver.set_page_load_timeout(0.01)

And the default timeout is so long (300 seconds) that it doesn't affect any other test.

> > Source/WebDriver/ChangeLog:9
> > +        commands. Also fix the setTimeouts command that was still using the legacy name of rthe page load timeout,
> 
> Typo: "of rthe" => "for the"

Oops, thanks!

[Carlos Garcia Campos]

Created attachment 316083 [details]
Rebased patch

This should apply now

[Blaze Burg]

Comment on attachment 315982 [details]
Patch

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

r=me with a few questions

> Source/WebDriver/ChangeLog:8
> +        Handle timeout errors and pass the pageLoadtimeout to waitForNavigationToComplete and all other navigation

Nit: pageLoadTimeout (or just in English)

> Source/WebKit/ChangeLog:8
> +        Always start a timer when waiting for a navigation to complete. When the timer fired callbacks pending to be

"when the timer fires, pending callbacks for navigations are removed and invoked with a timeout error."

> Source/WebKit/ChangeLog:11
> +        not provided the default timeout (300 seconds) is used.

Nice. I really like the stateless/explicit arguments to each command approach, as you may have figured out.

> Source/WebKit/UIProcess/Automation/Automation.json:282
> +                { "name": "pageLoadTimeout", "type": "integer", "optional": true, "description": "The timeout in milliseconds that the navigation is expected to be completed in, otherwise a <code>Timeout</code> error is returned." }

It would be okay to use a "type": "number" here and pass as a double. If we are using int milliseconds elsewhere, then it's probably better to be consistent though.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:52
> +static const Seconds defaultPageLoadTimeout = 300_s;

I love these new duration classes, so much easier to use than std::chrono!

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:407
> +    ASSERT(!m_loadTimer.isActive());

I had to think about this change for a minute, but I think it's okay.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:431
> +    for (auto frameID : m_pendingNavigationInBrowsingContextCallbacksPerFrame.keys()) {

Now I'm wondering how we could have multiple callbacks at all. Is there a way that one command can enqueue multiple pending navigation callbacks, aside from a programming error like calling this protocol method twice? It seems that the duration of two calls to this method shouldn't really overlap inside a command's implementation, and the timer should either be cleared or fire only once at a time. Am I missing something?

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:450
> +    waitForNavigationToCompleteOnPage(*page, optionalPageLoadTimeout ? Seconds::fromMilliseconds(*optionalPageLoadTimeout) : defaultPageLoadTimeout, WTFMove(callback));

Yuck, hopefully I'll find time to make optional parameters use std::optional in generated code.

[Radar WebKit Bug Importer]

<rdar://problem/33456873>

[Carlos Garcia Campos]

(In reply to Brian Burg from comment #5)
> Comment on attachment 315982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=315982&action=review
> 
> r=me with a few questions
> 
> > Source/WebDriver/ChangeLog:8
> > +        Handle timeout errors and pass the pageLoadtimeout to waitForNavigationToComplete and all other navigation
> 
> Nit: pageLoadTimeout (or just in English)

Sure.

> > Source/WebKit/ChangeLog:8
> > +        Always start a timer when waiting for a navigation to complete. When the timer fired callbacks pending to be
> 
> "when the timer fires, pending callbacks for navigations are removed and
> invoked with a timeout error."

Ok.

> > Source/WebKit/ChangeLog:11
> > +        not provided the default timeout (300 seconds) is used.
> 
> Nice. I really like the stateless/explicit arguments to each command
> approach, as you may have figured out.

:-)

> > Source/WebKit/UIProcess/Automation/Automation.json:282
> > +                { "name": "pageLoadTimeout", "type": "integer", "optional": true, "description": "The timeout in milliseconds that the navigation is expected to be completed in, otherwise a <code>Timeout</code> error is returned." }
> 
> It would be okay to use a "type": "number" here and pass as a double. If we
> are using int milliseconds elsewhere, then it's probably better to be
> consistent though.

The spec explicitly says the tiemout must be an integer in the range 0 to 2^64 - 1. We are doing the same for the script timeouts. See https://w3c.github.io/webdriver/webdriver-spec.html#set-timeouts

> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:52
> > +static const Seconds defaultPageLoadTimeout = 300_s;
> 
> I love these new duration classes, so much easier to use than std::chrono!
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:407
> > +    ASSERT(!m_loadTimer.isActive());
> 
> I had to think about this change for a minute, but I think it's okay.
> 
> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:431
> > +    for (auto frameID : m_pendingNavigationInBrowsingContextCallbacksPerFrame.keys()) {
> 
> Now I'm wondering how we could have multiple callbacks at all. Is there a
> way that one command can enqueue multiple pending navigation callbacks,
> aside from a programming error like calling this protocol method twice? It
> seems that the duration of two calls to this method shouldn't really overlap
> inside a command's implementation, and the timer should either be cleared or
> fire only once at a time. Am I missing something?

No, I wondered the same while working on this. That's also why added the ASSERT to ensure the timer is not active (which would mean another command is still waiting for navigations).

> > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:450
> > +    waitForNavigationToCompleteOnPage(*page, optionalPageLoadTimeout ? Seconds::fromMilliseconds(*optionalPageLoadTimeout) : defaultPageLoadTimeout, WTFMove(callback));
> 
> Yuck, hopefully I'll find time to make optional parameters use std::optional
> in generated code.

[Carlos Garcia Campos]

Created attachment 316225 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r219794: <http://trac.webkit.org/changeset/219794>