WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
174672
WebDriver: implement page load timeout
https://bugs.webkit.org/show_bug.cgi?id=174672
Summary
WebDriver: implement page load timeout
Carlos Garcia Campos
Reported
2017-07-20 04:48:53 PDT
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
Attachments
Patch
(26.33 KB, patch)
2017-07-20 05:02 PDT
,
Carlos Garcia Campos
bburg
: review+
Details
Formatted Diff
Diff
Rebased patch
(26.19 KB, patch)
2017-07-21 05:54 PDT
,
Carlos Garcia Campos
bburg
: review+
Details
Formatted Diff
Diff
Patch for landing
(26.40 KB, patch)
2017-07-23 02:32 PDT
,
Carlos Garcia Campos
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2017-07-20 05:02:16 PDT
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 2
2017-07-20 14:26:04 PDT
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
Comment 3
2017-07-20 23:10:06 PDT
(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
Comment 4
2017-07-21 05:54:16 PDT
Created
attachment 316083
[details]
Rebased patch This should apply now
Blaze Burg
Comment 5
2017-07-21 10:32:31 PDT
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
Comment 6
2017-07-21 10:33:35 PDT
<
rdar://problem/33456873
>
Carlos Garcia Campos
Comment 7
2017-07-23 02:25:42 PDT
(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
Comment 8
2017-07-23 02:32:29 PDT
Created
attachment 316225
[details]
Patch for landing
Carlos Garcia Campos
Comment 9
2017-07-23 22:58:57 PDT
Committed
r219794
: <
http://trac.webkit.org/changeset/219794
>
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