Closed Bug 1489141 Opened Last year Closed Last year

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

Categories

(Testing :: geckodriver, defect, P1)

Version 3
defect

Tracking

(firefox63 fixed, firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox63 --- fixed
firefox64 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(3 files)

Attached file 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.
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.
Flags: needinfo?(barancev)
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: -- → P1
Attached file trace.log
Flags: needinfo?(barancev)
Attachment #9006965 - Attachment mime type: application/octet-stream → text/plain
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".
Summary: [meta] Regressions in Java Selenium test suite for Serde changes in geckodriver → InvalidArgumentException: data did not match any variant of untagged enum PointerActionItem (ELEMENT in PointerOrigin)
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."
Given that all bindings actually send the element as requested by the spec, we are safe to simply ignore any other key.
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.
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
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.
Attachment #9008023 - Flags: review?(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.
Attachment #9008023 - Flags: review?(ato) → review+
(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?
(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.
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/285eb0ebe26f
[webdriver] Ignore any unknown variant of enum PointerOrigin.
https://hg.mozilla.org/mozilla-central/rev/285eb0ebe26f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Please uplift this test-only change to beta. Thanks.
Whiteboard: [checkin-needed-beta]
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.