1485580 - [wdspec] Parsing timeout values given as floats fail when equal to result of toFixed()

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P2)

Description

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

The following tests make use of the `valid_data` from the `create.py` module:

https://searchfox.org/mozilla-central/rev/f2ac80ab7dbde5400a3400d463e07331194dec94/testing/web-platform/tests/webdriver/tests/new_session/support/create.py#9

And that contains a line like:

> {"script": 0, "pageLoad": 2.0, "implicit": 2**53 - 1}

As given by the spec only integers are allowed.

It means that the tests are invalid, and need to be corrected.

Comment 1

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

Attachment: [wdspec] Correct create_alwaysMatch.py and create_firstMatch.py for float not being a valid timeout value

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91d1a9f349023cb791d2b2ad600d4bb61fe21e3c

Comment 3

[James Graham [:jgraham]]

Comment on attachment 9003379 [details] [diff] [review]
[wdspec] Correct create_alwaysMatch.py and create_firstMatch.py for float not being a valid timeout value

Review of attachment 9003379 [details] [diff] [review]:
-----------------------------------------------------------------

2.0 is an integer. Javascript (and hence JSON) doesn't have a distinction between floats and ints, so we have to accept any input that can be exactly represented as an integer.

Comment 4

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

Hm, interesting. Looks like I missed the following part of the spec:

> An integer is a Number that is unchanged under the ToInteger operation. 

Actually `ToInteger()` is obsolete and never made it from the draft into the final ES6 specification as what MDN says:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toInteger

Comment 5

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

So I filed a WebDriver spec issue: https://github.com/w3c/webdriver/issues/1295

I think that we want to have this check in Marionette, but also geckodriver. Given that Serde is in use now, we would need a custom deserialization routine.

As such I will keep this bug in the geckodriver component.

Comment 6

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

Actually Marionette handles that all very well, so I only had to update the unit tests for a better coverage.

For geckodriver I got the changes implemented for `Set Timeout`, and soon also for the capabilities in `New Session`. To test all the various cases I completed all the tests for `Set Timeout`.

I should have an updated patch soon.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3aca8a655cf0afb09134d1d3bc4495508e53f330

Comment 8

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

I accidentally pushed an artifact build to try, whereby it needs to be a non-artifact build due to the geckodriver changes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7613eab46178d698bf3c7d8f2d6c0476c535a96d

Comment 9

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

Attachment: [wdspec] Improve timeouts tests for "Set Timeout" and "New Session"

Comment 10

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

Attachment: [marionette] Update unit tests for invalid timeout values

Comment 11

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

Attachment: [geckodriver] Make parsing of float timeout values spec conforming

Comment 12

[James Graham [:jgraham]]

Comment on attachment 9003749 [details] [diff] [review]
[geckodriver] Make parsing of float timeout values spec conforming

Review of attachment 9003749 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/webdriver/src/capabilities.rs
@@ +331,2 @@
>                      );
> +                    if timeout < 0.0 || timeout.fract() != 0.0 {

I wonder if we should have a tolerance for nonzero fractional parts? Probably not necessary for this bug but could be needed in a followup.

@@ +341,5 @@
> +                    if (timeout as u64) > MAX_SAFE_INTEGER {
> +                        return Err(WebDriverError::new(
> +                            ErrorStatus::InvalidArgument,
> +                            format!(
> +                                "'{}' timeouts valus is outside of maximum safe integer: {}",

typo "valus"

::: testing/webdriver/src/command.rs
@@ +525,5 @@
> +                )));
> +            }
> +            if (n as u64) > MAX_SAFE_INTEGER {
> +                return Err(de::Error::custom(format!(
> +                    "'{}' is outside of maximum safe integer",

s/Outside of/Greater than/

Comment 13

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

(In reply to James Graham [:jgraham] from comment #12)
> > +                    if timeout < 0.0 || timeout.fract() != 0.0 {
> 
> I wonder if we should have a tolerance for nonzero fractional parts?
> Probably not necessary for this bug but could be needed in a followup.

Handling the epsilon is also something which I mentioned in the created webdriver spec issue yesterday. I decided against using it for now as long as the issue is open. A follow-up sounds fine to me, once the issue has been resolved, and handling the epsilon is required.

The other two issues will be fixed with the next update.

Thanks.

Comment 14

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

Attachment: [geckodriver] Make parsing of float timeout values spec conforming

Comment 15

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

Comment on attachment 9003791 [details] [diff] [review]
[geckodriver] Make parsing of float timeout values spec conforming

Updated patch with the two issues fixed. Taking over the r+.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd335726c00
[wdspec] Improve timeouts tests for "Set Timeout" and "New Session". r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bc3167b0240
[marionette] Update unit tests for invalid timeout values. r=jgraham
https://hg.mozilla.org/integration/mozilla-inbound/rev/26632fd3312a
[geckodriver] Make parsing of float timeout values spec conforming. r=jgraham

Comment 17

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

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

Comment 18

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

Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12675
* continuous-integration/travis-ci/pr (https://travis-ci.org/web-platform-tests/wpt/builds/420136488?utm_source=github_status&utm_medium=notification)

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/1fd335726c00
https://hg.mozilla.org/mozilla-central/rev/4bc3167b0240
https://hg.mozilla.org/mozilla-central/rev/26632fd3312a

Comment 20

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

Upstream PR merged