1959464 - Fractional x/y support in pointer actions breaks compatibility with geckodriver 0.36.0 and Firefox < 137

Status: NEW

(Testing :: geckodriver, defect, P3)

Description

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

(In reply to Jon Gjengset from bug 1955903 comment #21)

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.

This is actually an issue with geckodriver 0.36.0 and any version of Firefox before 137.0. Given that we offer support to the last ESR release, which is 128 at the moment, we should do something about it. We have the following options:

1. Changing the entry of the support matrix for geckodriver 0.36.0 to list Firefox 137 as minimum. It would require users who are using older Firefox releases for testing purposes to use geckodriver 0.35.0 instead. It would also break with our claim to support back to the latest ESR.
2. Mentioning this as a known bug in the release notes and wait if other users are affected as well. Depending on the amount of feedback we could work on step 3.
3. Fixing the issue by serializing the x and y positions for Marionette as integers in case of a Firefox version older than 137. This would require another geckodriver release.

Firefox 137 got released on April 1st, so it should basically be the default in most CI setups soon. Also so far we have a single person who is actually affected by that, and haven't heard complains otherwise. As such maybe we go with option 1 for now and if needed do option 3?

Comment 1

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

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

2. Mentioning this as a known bug in the release notes and wait if other users are affected as well. Depending on the amount of feedback we could work on step 3.

I actually did that as part of https://phabricator.services.mozilla.com/D244143, which we are going to land soon for the webdriver 0.53.0 release.

Comment 2

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

This is likely less critical than initially assumed, as it only occurs when a client explicitly sends a fractional value for one or both fields. Previously, the type was an integer, so clients have been sending integer values - possibly rounded, floored, or ceiled internally. This is because values like 2.0 (with no fractional part) will be handles as integer in JavaScript.

As a result, the impact is most likely minimal for now, at least as long as clients don't update their implementations. They could also choose to conditionally send fractional values based on the Firefox version.

Comment 3

[Jon Gjengset]

The challenge here is that the webdriver crate serializes even integer values as floating point, so any clients that use that crate for interfacing with WebDriver hosts will hit this issue. Could the webdriver crate be updated to conditionally serialize integer values for x/y?

Comment 4

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

The modified values are parameters for the PointerMove action so for clients only the deserialization will be important. Where does it actually fail for you in detail in your client?

Comment 5

[James Graham [:jgraham]]

I think it makes sense that if you're using the webdriver crate in a client you might end up creating values that some remote ends don't support. Since we are only using the crate in a scenario where the serialization doesn't matter (because the values are consumed by JS which doesn't care about floats vs int), we don't see the problem.

I think this is a case where we'd really happily accept a patch, but as it's not affecting our usage of the crate it's not a high priority internally.

Comment 6

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

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit BugBot documentation.