Bug 174672

Summary: WebDriver: implement page load timeout
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   
Bug Depends on: 174670    
Bug Blocks: 166679    
Attachments:
Description Flags
Patch
bburg: review+
Rebased patch
bburg: review+
Patch for landing none

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+
DetailsFormatted DiffDiff
Rebased patch (26.19 KB, patch)
2017-07-21 05:54 PDT, Carlos Garcia Campos 		bburg: review+
DetailsFormatted DiffDiff
Patch for landing (26.40 KB, patch)
2017-07-23 02:32 PDT, Carlos Garcia Campos 		no flags
DetailsFormatted DiffDiff
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.