1955903 - NewSessionParameters type no longer constructable since webdriver 0.52

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P2)

Description

[Jon Gjengset]

Hey folks!

In Rust, the internals of an enum are always public, whereas the fields of a struct are not. In the move of webdriver::command::NewSessionParameters from an enum to a struct to remove the legacy version (https://phabricator.services.mozilla.com/D199799), the new capabilities field was not marked as pub. This means it's no longer possible to construct a NewSessionParameters in code that uses the webdriver crate. This in turn blocks crates like fantoccini1, which implements a WebDriver client using webdriver and thus needs to construct these types, not just deserialize them, from upgrading to any 0.51+ version of webdriver.

I recommend the capabilities field be make pub to rectify this.

Cheers,
Jon (maintainer of fantoccini)

Comment 1

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

Thank you for the report. This was an oversight that we indeed should fix.

Note that the fix we can ship without having to as well release geckodriver immediately.

Comment 2

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

Attachment: Bug 1955903 - [rust-webdriver] Make entries in NewSessionParameters public.

Comment 3

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

Attachment: Bug 1955903 - [rust-webdriver] Release version 0.52.1.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a6a882f21a2
[rust-webdriver] Make entries in NewSessionParameters public. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/ab15b94c2e9b
[rust-webdriver] Release version 0.52.1. r=webdriver-reviewers,jgraham

Comment 5

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

Once merged to mozilla-central I have to push the release to crates.io.

Comment 6

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

This bug has been marked as a regression. Setting status flag for Nightly to affected.

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8a6a882f21a2
https://hg.mozilla.org/mozilla-central/rev/ab15b94c2e9b

Comment 8

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

Version 0.52.1 of webdriver has been published to crates.io: https://crates.io/crates/webdriver/0.52.1

Jon, please let us know how it works. I hope it's all fine now for you.

Comment 9

[Jon Gjengset]

Yes, that seems to have done it, thank you!

I now hit the fact that tests fail because the versions of Firefox I test against (which are all quite recent) do not accept floating-point values in MoveBy and MoveToElement pointer actions, presumably because they haven't adopted the corresponding change in the webdriver change either for when they deserialize? It's not clear to me what the intended upgrade path here is — it effectively means that there will need to be a flag-day migration where users of older Firefox cannot use newer clients (based on webdriver), is that right? If so, what version of Firefox will have the webdriver change to deserialize as floating point values in it, so that I can explicitly list that as the required Firefox version for fantoccini following this upgrade?

Comment 10

[Jon Gjengset]

To add: this also applies to Chrome, which similarly refuses to deserialize WebDriver requests with floating point values in pointer actions. Was this an actual change to the WebDriver spec, or were both browsers simply not in compliance?

Comment 11

[Kyle Huey [:khuey]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #8)

Version 0.52.1 of webdriver has been published to crates.io: https://crates.io/crates/webdriver/0.52.1

This version should be yanked because it's not backwards compatible with 0.52.0.

One of the changes that shipped in 0.52.1 is Bug 1894795. That bumped the base64 version from 0.21 to 0.22. The webdriver crate contains an impl From<base64::DecodeError> for WebDriverError. And anyone who is using that impl (and thus is using base64 0.21) who is not pinned to exactly 0.52.0 will break with webdriver 0.52.1.

You can see the problem in action with cargo install --version 0.36.0 geckodriver

Comment 12

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

Thanks a lot for the additional feedback, especially for the backward incompatible version change that cause cargo to fail building geckodriver.

I've for now yanked the webdriver 0.52.1 version of the crate, and will check why the semver checks didn't notice the build failure.

Further I'll check the backward incompatible issue with floating vs. decimal values. The floating point type will be supported starting with Firefox 137 that is going to be released tomorrow. Lets see which options we will have.

For now I'm reopening this bug.

Comment 13

[Kyle Huey [:khuey]]

Thanks Henrik.

Comment 14

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

(In reply to Jon Gjengset from comment #9)

I now hit the fact that tests fail because the versions of Firefox I test against (which are all quite recent) do not accept floating-point values in MoveBy and MoveToElement pointer actions, presumably because they haven't adopted the corresponding change in the webdriver change either for when they deserialize? It's not clear to me what the intended upgrade path here is — it effectively means that there will need to be a flag-day migration where users of older Firefox cannot use newer clients (based on webdriver), is that right? If so, what version of Firefox will have the webdriver change to deserialize as floating point values in it, so that I can explicitly list that as the required Firefox version for fantoccini following this upgrade?

Sorry for that. This is actually a change (bug 1946774) in Firefox 137, which has been released today. You can find an entry in the developer release notes on MDN.

Could you please tell me if you are passing in a decimal or floating point number? We did not floor nor ceil the numbers before bug 1946774.

(In reply to Jon Gjengset from comment #10)

To add: this also applies to Chrome, which similarly refuses to deserialize WebDriver requests with floating point values in pointer actions. Was this an actual change to the WebDriver spec, or were both browsers simply not in compliance?

Yes, this changed in https://github.com/w3c/webdriver/pull/1881 and https://github.com/w3c/webdriver-bidi/pull/874.

Comment 15

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

(In reply to Kyle Huey [:khuey] from comment #11)

You can see the problem in action with cargo install --version 0.36.0 geckodriver

Thanks! I’m able to reproduce the build error. However, I’m curious why it doesn’t occur in mozilla-central when building geckodriver in testing/geckodriver using cargo build. I also ran cargo semver-checks to verify that a bugfix release would be compatible, and it reported no issues.

I’m wondering now what steps are needed to reliably reproduce this issue locally, and how we can add an extra check to prevent it from happening again in the future.

Comment 16

[Kyle Huey [:khuey]]

However, I’m curious why it doesn’t occur in mozilla-central when building geckodriver in testing/geckodriver using cargo build.

Because the in-tree geckodriver was changed to use the new base64 at the same time webdriver was (https://hg.mozilla.org/mozilla-central/rev/17869e572bbd). But that is not live on crates.io.

I also ran cargo semver-checks to verify that a bugfix release would be compatible, and it reported no issues.

I have never used that tool but searching its bug tracker I found https://github.com/obi1kenobi/cargo-semver-checks/issues/447 which I think is essentially the same issue. The only difference is that the use of the dependency in your case is in an trait impl, whereas there it's in a public struct member.

Comment 17

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

(In reply to Kyle Huey [:khuey] from comment #16)

However, I’m curious why it doesn’t occur in mozilla-central when building geckodriver in testing/geckodriver using cargo build.

Because the in-tree geckodriver was changed to use the new base64 at the same time webdriver was (https://hg.mozilla.org/mozilla-central/rev/17869e572bbd). But that is not live on crates.io.

Oh, yes. That's indeed what I completely overlooked here. That totally explains it but also makes it impossible for us to test locally. The only thing that I could do here is to really add a note to our releasing documentation for geckodriver that when the webdriver crate gets released out of band extra care needs to be taken of - especially for any bump of a dependency crate.

I'll bump the version of the webdriver crate to 0.53.0 so that it will not be used with geckodriver 0.36.0, but be part of the next 0.37.0 release, which is not targeted yet.

I have never used that tool but searching its bug tracker I found https://github.com/obi1kenobi/cargo-semver-checks/issues/447 which I think is essentially the same issue. The only difference is that the use of the dependency in your case is in an trait impl, whereas there it's in a public struct member.

Thanks. I'll observe this issue.

Comment 18

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

Attachment: Bug 1955903 - [geckodriver] Update release notes for fractional x and y position support.

Comment 19

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

Attachment: Bug 1955903 - [geckodriver] Update documentation for out-of-band webdriver releases.

Comment 20

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

Attachment: Bug 1955903 - [webdriver-rust] Release version 0.53.0.

Comment 21

[Jon Gjengset]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+2] from comment #14)

(In reply to Jon Gjengset from comment #9)

I now hit the fact that tests fail because the versions of Firefox I test against (which are all quite recent) do not accept floating-point values in MoveBy and MoveToElement pointer actions, presumably because they haven't adopted the corresponding change in the webdriver change either for when they deserialize? It's not clear to me what the intended upgrade path here is — it effectively means that there will need to be a flag-day migration where users of older Firefox cannot use newer clients (based on webdriver), is that right? If so, what version of Firefox will have the webdriver change to deserialize as floating point values in it, so that I can explicitly list that as the required Firefox version for fantoccini following this upgrade?

Sorry for that. This is actually a change (bug 1946774) in Firefox 137, which has been released today. You can find an entry in the developer release notes on MDN.

Could you please tell me if you are passing in a decimal or floating point number? We did not floor nor ceil the numbers before bug 1946774.

The problem here is that when you use serde_json to serialize an f64, it always serializes as a floating point number, even if it happens to be an integer value. That is

println!("{}", serde_json::to_string(&1.0f64).expect("serialize"));

will print "1.0", not "1". On the deserialization side, a floating point value, even if it happens to be an integer value, will fail to deserialize. That is

let x: i32 = serde_json::from_str(
    &serde_json::to_string(&1.0f64).unwrap()
).unwrap();

will panic with

invalid type: floating point `1.0`, expected i32

As a result, even when passing integer values to webdriver, the fact that the fields are now f64 means that they'll be serialized as a floating point value, which will then fail to deserialize in any older Firefox version. It will also be rejected by Chrome/Chromium, because they follow the spec as of before the change you linked, and thus only accept integer literals for x and y.

I don't know that there's an optimal solution to this. In the short term, maybe we could change the impl Serialize for the relevant type to serialize the floating points to integers for the time being (by floor/ceil-ing them), but still deserialize as f64? That way, down the line, we could change the serialization to actually produce floating point values, and some non-zero number of Firefox versions would accept them by then. That would also allow aligning with Chrom* adopting the change.

Comment 22

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

Thanks for the details Jon! Given that this is a different issue I moved it to bug 1959464.

To unblock any user who directly uses the webdriver crate I'm going to land the patches once reviewed.

Comment 23

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

Marking as leave-open because I have to sync the documentation and release notes with the GitHub release branch + release notes.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6093be2522b6
[geckodriver] Update release notes for fractional x and y position support. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/2fab9550fb22
[geckodriver] Update documentation for out-of-band webdriver releases. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/bd61fb4081be
[webdriver-rust] Release version 0.53.0. r=webdriver-reviewers,jgraham

Comment 25

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/6093be2522b6
https://hg.mozilla.org/mozilla-central/rev/2fab9550fb22
https://hg.mozilla.org/mozilla-central/rev/bd61fb4081be

Comment 26

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

Published the webdriver 0.53.0 crate on crates.io: https://crates.io/crates/webdriver/0.53.0

I've also verified that geckodriver 0.36.0 can be successfully built. So it looks good now.

I'll close the bug once the documentation upstream has been updated.

Comment 27

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

Upstream PR for doc changes: https://github.com/mozilla/geckodriver/pull/2225

I as well updated all release notes for 0.36.0, 0.35.0, and 0.34.0.