1529976 - wdspec test fails with new serde/serde_derive

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement)

Description

[Kartikaya Gupta (email:[e-mail])]

An update of serde_derive to 1.0.88 causes a consistent failure in one of the invalid_capabilities.py tests. This is blocking updating various crates.

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=573a88c4591b4adc41434bbce466943af5905f09 is an example try push showing the problem.

I'll bisect a bit to see which version of serde_derive the problem starts with. Currently in m-c we are using 1.0.80 which doesn't have the problem.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Looks like it started with 1.0.82.

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&author=kgupta%40mozilla.com&tochange=edc388320bafde0ad768a87c58418bb951693e20&fromchange=cd0dc3d690732c12dfc824b4ac80360eb58815b3

This is the range of commits: https://github.com/serde-rs/serde/compare/v1.0.81...v1.0.82

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Henrik, do you know if this looks like a bug in serde, or a bug in the webdriver code? I'm a bit out of my comfort zone here.

Comment 3

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

By passing an invalid capability object we expect no session to be started. But here it's actually happening:

[task 2019-02-22T18:26:03.682Z] 18:26:03 INFO - STDOUT: tests/web-platform/tests/webdriver/tests/new_session/invalid_capabilities.py::test_invalid_capabilites[value3]
[task 2019-02-22T18:26:03.683Z] 18:26:03 INFO - PID 31336 | 1550859963677 webdriver::server DEBUG -> POST /session {"capabilities": []}
[task 2019-02-22T18:26:03.684Z] 18:26:03 INFO - PID 31336 | 1550859963680 mozrunner::runner INFO Running command: "/usr/bin/firefox" "-marionette" "-foreground" "-no-remote" "-profile" "/tmp/rust_mozprofile.eDYifqnSFBSa"
[..]
[task 2019-02-22T18:26:07.106Z] 18:26:07 INFO - PID 31336 | 1550859967100 Marionette TRACE 0 <- [1,1,null,{"sessionId":"cd61af88-5376-43da-8822-e67f4cfcbc15","capabilities":{"browserName":"firefox","browserVersion":"62.0. ... 0,"moz:profile":"/tmp/rust_mozprofile.eDYifqnSFBSa","moz:useNonSpecCompliantPointerOrigin":false,"moz:webdriverClick":true}}]

So the problematic code should be somewhere in:
https://searchfox.org/mozilla-central/rev/b10ae6b7a50d176a813900cbe9dc18c85acd604b/testing/webdriver/src/command.rs#451-453

The test failing here is for {"capabilities": []}.

What's causing that I don't know but maybe https://github.com/serde-rs/serde/commit/385a385c62d69ad5a702d93ff8d14bd2138bc4da is related here?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Yoric, as this is blocking the rust update you want, do you have cycles to investigate?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Well, I don't even know what wdspec is, nor what these tests are about, or how they work, so I suspect I'm the wrong person for this.

Comment 6

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

Note that I won't have the time to dig into this before next week. By the first looks it might be a regression or backward incompatible change in Serde. Feel free to point dtolnay to this bug.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

dtolnay - quick summary is that something in the serde bump from 1.0.81 -> 1.0.82 causes a difference in behaviour in some wdspec deserialization (see comment 3). It's unclear if this is a regression or just an incompatible change. Do you happen to know if a change in behaviour is expected? I'm assuming no because it was a patchlevel version bump. It might also be that the wdspec deserialization code is doing things that depend on serde internals.

Comment 8

[Aria Beingessner [:Gankra]]

Reading through our deserialization code, I am a bit confused why {"capabilities": []} is "supposed" to be considered invalid?

When we deserialize NewSessionParameters we parse a json object and look for a capabilities field, which matches that input.

Then we try to parse the field's contents as a SpecNewSessionParameters, which marks all of its fields as defaulted. So an empty array should technically be a valid SpecNewSessionParameters, as serde supports deserializing things from arrays/tuples by assuming the fields come in declaration order.

Am I missing something? Did you intend to express something along the lines of "at least one of these must be provided"? Or just "must be an object, reject all arrays"?

Comment 9

[Aria Beingessner [:Gankra]]

On the assumption that "must be an object" was the expectation, there are 3 possible paths forward:

• Change SpecNewSessionParameters to have a custom deserialize impl that demands a map (object)
• Change NewSessionParameters to expect that its field is a map (object)
• Change the [] to a null in the input and embrace that [] is a valid NewSessionParameters.

They all have subtly different effects, so it depends on how the maintainers of this subsystem feels about [] being a valid parse.

Comment 10

[James Graham [:jgraham]]

There's a spec! https://w3c.github.io/webdriver/#dfn-capabilities-processing step 1 insists that we return an error if it's not an Object.

So I think that one of the first two alternatives are correct, and I suspect we don't want to change the behaviour for NewSessionParameters since that's supporting legacy clients, so the first option seems most likely.

Comment 11

[Aria Beingessner [:Gankra]]

Attachment: Bug 1529976 - only parse spec capabilities that are objects.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2a9b9f7b3b69
only parse spec capabilities that are objects. r=jgraham

Comment 13

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

Thanks for fixing that Alexis!

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2a9b9f7b3b69