2008345 - Allow configuration of "implicit" and "pageLoad" timeouts to accept "null" values

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P3)

Description

[Euclid Ye]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:147.0) Gecko/20100101 Firefox/147.0

Steps to reproduce:

I ran wpt tests and look at the code base.

Actual results:

https://hg-edge.mozilla.org/mozilla-central/file/tip/testing/webdriver/src/capabilities.rs#l377

We currently only allow script timeout to be None.

Expected results:

Spec allows implicit wait timeout and page load timeout to be None:
https://w3c.github.io/webdriver/#dfn-deserialize-as-timeouts-configuration

Comment 1

[Euclid Ye]

cc @whimbo

Comment 2

[Euclid Ye]

This should be very easy to fix by: x @ "script" | x @ "pageLoad" | x @ "implicit" if value.is_null() => {}
https://hg-edge.mozilla.org/mozilla-central/file/tip/testing/webdriver/src/capabilities.rs#l377

I don't have proper environment to do this tho.

See WPT updates:
https://github.com/web-platform-tests/wpt/pull/56959

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::DOM: Core & HTML' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for letting us know. Interesting that this hasn't been noticed yet. It would be good to wait until the upstream PR has merged and downstream synced via bug 2008343.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried that little fix but it turned out that the tests are still not working as expected. Fixing those turned out issues with the webdriver client as well when using the Set Timeout command. As such I put together a patch including the fixes and adding more tests to ensure that null actually doesn't break any other command (navigate, refresh, back, forward, and all the variants of find element(s)) as well.

Here a try build: https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=172018

Comment 7

[Euclid Ye]

We probably need to change following struct as well:

https://hg-edge.mozilla.org/mozilla-central/file/tip/testing/webdriver/src/response.rs#l103

They should all be Option<u64>.

Also following to Option<Option<u64>>:
https://hg-edge.mozilla.org/mozilla-central/file/tip/testing/webdriver/src/command.rs#l772

Comment 8

[Euclid Ye]

I'm not familiar with the interface here. How can I see your @whimbo proposed PR?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

In my last try build there was still a bug in our wait for navigation completed logic where we tried to use a TimedPromise even when the pageLoad timeout is set to null. Here a new try build with the fix:

https://treeherder.mozilla.org/jobs?repo=try&landoCommitID=172034

(In reply to Euclid Ye from comment #8)

I'm not familiar with the interface here. How can I see your @whimbo proposed PR?

Open that try URL and on the left side you can see the individual commits that I pushed. It's covering Marionette, Remote Agent, geckodriver, webdriver, wpt's webdriver client and wdspec tests.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 2008345 - [remote] Allow "null" for "implicit" and "pageLoad" timeouts.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 2008345 - [marionette] Don't use TimedPromise if pageload timeout is "null".

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 2008345 - [geckodriver] Allow "null" for "implicit" and "pageLoad" timeouts.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 2008345 - [webdriver-client] Support "null" value when setting timeouts.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 2008345 - [wdspec] Improve tests when "implicit" and "pageLoad" timeouts are set to "null".

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6c232d4bd431
https://hg.mozilla.org/integration/autoland/rev/41a0e5b4cf8d
[remote] Allow "null" for "implicit" and "pageLoad" timeouts. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/43e61790716f
https://hg.mozilla.org/integration/autoland/rev/d9b4b9a79e8e
[marionette] Don't use TimedPromise if pageload timeout is "null". r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/3c8816932cb0
https://hg.mozilla.org/integration/autoland/rev/5c847856a5ef
[geckodriver] Allow "null" for "implicit" and "pageLoad" timeouts. r=geckodriver-reviewers,jdescottes,jgraham
https://github.com/mozilla-firefox/firefox/commit/77e77081f54b
https://hg.mozilla.org/integration/autoland/rev/a8f759981834
[webdriver-client] Support "null" value when setting timeouts. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/c85534cff4bd
https://hg.mozilla.org/integration/autoland/rev/a98e106b55b0
[wdspec] Improve tests when "implicit" and "pageLoad" timeouts are set to "null". r=jdescottes

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/57151 for changes under testing/web-platform/tests

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f6a8991120af
https://hg.mozilla.org/integration/autoland/rev/fdb7dd7448fc
Revert "Bug 2008345 - [wdspec] Improve tests when "implicit" and "pageLoad" timeouts are set to "null". r=jdescottes" for causing build bustages @rust.mk.

Comment 18

[agoloman]

Backed out for causing build bustages @rust.mk.

• Backout link
• Push with failures
• Failure Log

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There are additional tests under testing/geckodriver/marioneette which were not executed when I run the tests locally with cargo test under testing/geckodriver. :( It's annoying that you have to be in each crate's folder. I fixed those tests and to be fully sure now I have pushed to try:

https://treeherder.mozilla.org/jobs?repo=try&revision=87208943144aaee2b249d53bdd1d491ecb3d982c

If it's all fine I'll reland the patches.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/2119d14e6a1e
https://hg.mozilla.org/integration/autoland/rev/6e5cba21e444
[remote] Allow "null" for "implicit" and "pageLoad" timeouts. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/f34cfc5f6729
https://hg.mozilla.org/integration/autoland/rev/009c9e0c717b
[marionette] Don't use TimedPromise if pageload timeout is "null". r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/511b64fa8fc6
https://hg.mozilla.org/integration/autoland/rev/e27a9a0758d6
[geckodriver] Allow "null" for "implicit" and "pageLoad" timeouts. r=geckodriver-reviewers,jdescottes,jgraham
https://github.com/mozilla-firefox/firefox/commit/61ad2a2ac48c
https://hg.mozilla.org/integration/autoland/rev/8e1f4ed0592e
[webdriver-client] Support "null" value when setting timeouts. r=jdescottes
https://github.com/mozilla-firefox/firefox/commit/1e0b69eb6b19
https://hg.mozilla.org/integration/autoland/rev/ebfab89fc1d5
[wdspec] Improve tests when "implicit" and "pageLoad" timeouts are set to "null". r=jdescottes

Comment 21

[pstanciu]

https://hg.mozilla.org/mozilla-central/rev/6e5cba21e444
https://hg.mozilla.org/mozilla-central/rev/009c9e0c717b
https://hg.mozilla.org/mozilla-central/rev/e27a9a0758d6
https://hg.mozilla.org/mozilla-central/rev/8e1f4ed0592e
https://hg.mozilla.org/mozilla-central/rev/ebfab89fc1d5

Comment 22

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot