1489141 - InvalidArgumentException: data did not match any variant of untagged enum PointerActionItem (ELEMENT in PointerOrigin)

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P1)

Description

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

Attachment: Selenium log

Alexei thankfully run the Java Selenium tests for the nightly version of geckodriver and sent me the following log file. As it can be seen we have a lot of regressions especially for actions.

It looks like there is a bit of work left for me to do. I will use this bug for tracking.

Comment 1

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

Actually checking the log in detail I can only see a single point of failure which is deserializing a PointerActionItem:

> InvalidArgumentException: data did not match any variant of untagged enum PointerActionItem at line %x column %y

Alexei would you mind to just run one of those failing tests with trace logging enabled? I would appreciate those details.

Comment 2

[Alexei Barantsev]

Attachment: trace.log

Comment 3

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

Thank you! Quickly glancing over the tracing output I think it is the following origin reference and specifically the `ELEMENT` key:

          "origin": {
            "ELEMENT": "cbd0b1e7-a456-4730-bf2b-b8ba7ec6aba1",
            "element-6066-11e4-a52e-4f735466cecf": "cbd0b1e7-a456-4730-bf2b-b8ba7ec6aba1"
          }

Where does `ELEMENT` come from? Is that a legacy key we still have to support? I may have missed that because it's not part of the WebDriver spec:

> An ECMAScript Object represents a web element if it has a web element identifier own property. 

> The web element identifier is the string constant "element-6066-11e4-a52e-4f735466cecf".

Comment 4

[Alexei Barantsev]

This key exists here for backward compatibility with early versions of geckodriver.

As I read the spec it does not deny having extra keys:

"The JSON serialization of an element is a JSON Object where the web element identifier key is mapped to the element’s web element reference."

Comment 5

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

Given that all bindings actually send the element as requested by the spec, we are safe to simply ignore any other key.

Comment 6

[Andreas Tolfsen ❲:ato❳]

Yes, this applies generally to the WebDriver protocol: it was
explicitly designed so that backwards compatibility can be achieved
by ignoring unknown fields.  In particular, this is what makes new
session creation work.

Of course, if a vendor introduces anything _new_ and vendor-specific,
this should go in "-vendor-prefix" styled fields.

Comment 7

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

This is actually harder than I thought because `PointerOrigin` is handled as an untagged enum, and as such only accepts known variants. Here an example in the playground:

https://play.rust-lang.org/?gist=ec22336dd31436f130b9b433f8394d61&version=stable&mode=debug&edition=2015

I asked for help, so maybe I can continue on Monday.

https://github.com/serde-rs/serde/issues/1377

Comment 8

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

Attachment: [webdriver] Ignore any unknown variant of enum PointerOrigin

To keep backward compatibility, the legacy "ELEMENT" key for an
instance of PointerOrigin has to be supported, but ignored.

This workaround can be removed once legacy support gets dropped
from geckodriver.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7599b093305248f0ba90b3efd813c0a9152424e4

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9008023 [details] [diff] [review]
[webdriver] Ignore any unknown variant of enum PointerOrigin

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

::: testing/webdriver/src/actions.rs
@@ +183,4 @@
>      }
>  }
>  
> +// TODO: Can be removed once legacy ELEMENT key is no longer supported (Bug 1489141)

Supported _where_?  I assume this means when Selenium clients are
no longer using it?

In this case we should link to a tracking bug in Selenium.

@@ +199,5 @@
> +        } else if value == "viewport" {
> +            Ok(PointerOrigin::Viewport)
> +        } else {
> +            Err(de::Error::custom(format!(
> +                "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`",

It’s not appropriate to use backticks/Markdown here.  The expected
inputs are given as strings, so these should all be in double quotes,
except the first which shouldn’t have any because we could derive
its type from the value variable.

Comment 11

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #10)
> > +// TODO: Can be removed once legacy ELEMENT key is no longer supported (Bug 1489141)
> 
> Supported _where_?  I assume this means when Selenium clients are
> no longer using it?
> 
> In this case we should link to a tracking bug in Selenium.

I will file an issue for Selenium, but I think it will go away with the 4.0 release of Selenium where they want to break backward compatibility.

> > +        } else if value == "viewport" {
> > +            Ok(PointerOrigin::Viewport)
> > +        } else {
> > +            Err(de::Error::custom(format!(
> > +                "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`",
> 
> It’s not appropriate to use backticks/Markdown here.  The expected
> inputs are given as strings, so these should all be in double quotes,

This is how Serde outputs the failures, and I want to have parity with that. See https://play.rust-lang.org/?gist=7a27ca462f2aa272fe3fd1788921488c&version=stable&mode=debug&edition=2015.

> except the first which shouldn’t have any because we could derive
> its type from the value variable.

It's not the type we would want here but the name of the key. And this could be any kind of type including a Map. So do you have an idea how to best output it?

Comment 12

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Andreas Tolfsen ❲:ato❳ from comment #10)
> 
> > > +        } else if value == "viewport" {
> > > +            Ok(PointerOrigin::Viewport)
> > > +        } else {
> > > +            Err(de::Error::custom(format!(
> > > +                "unknown value `{}`, expected `pointer`, `viewport`, or `element-6066-11e4-a52e-4f735466cecf`",
> > 
> > It’s not appropriate to use backticks/Markdown here.  The expected
> > inputs are given as strings, so these should all be in double quotes,
> 
> This is how Serde outputs the failures, and I want to have parity with that.
> See https://play.rust-lang.org/?gist=7a27ca462f2aa272fe3fd1788921488c&version=stable&mode=debug&edition=2015.

Oh OK.  So it does that for the internal Rust types which I guess
is correct.

Comment 13

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

So I will update the patch with the Selenium issue (https://github.com/SeleniumHQ/selenium/issues/6393), and push it. Thank you for the review.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/285eb0ebe26f
[webdriver] Ignore any unknown variant of enum PointerOrigin.

Comment 15

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/285eb0ebe26f

Comment 16

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

Please uplift this test-only change to beta. Thanks.

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/releases/mozilla-beta/rev/39b50c15dc4d