1583761 - Some type checks in capabilities.rs for geckodriver raise UnknownError instead of InvalidArgument

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P1)

Description

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

Not sure if that is expected but for me it looks like a bug in various places in capabilities.rs of geckodriver. Several type checks raise an UnknownError instead of InvalidArgument like that one:

https://searchfox.org/mozilla-central/rev/45f30e1d19bde27bf07e47a0a5dd0962dd27ba18/testing/geckodriver/src/capabilities.rs#347

Andreas, I assume that was an oversight and as such collides with the WebDriver spec.

Comment 1

[Andreas Tolfsen ❲:ato❳]

I think the answer is it depends on a case-by-case basis.

When an error is caused by invalid input, i.e. it breaches with the
types and bounds checks described in WebDriver, it should generally
be coerced to ErrorStatus::InvalidArgument. This is the case in
the concrete example you linked to because we expect profile to
take one particular shape and the mistake is clearly on the user’s
side.

There are other cases where the input is not at fault, e.g.
hypothetically imagining that we were unable to write the provided
Base64-encoded profile to disk, and where it is appropriate to use
ErrorStatus::UnknownError for the lack of a more specific error
code in WebDriver.

Does that make sense?

Comment 2

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

Yes, that is exactly what I was expecting. I will get this fixed.

Comment 3

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

Attachment: Bug 1583761 - [geckodriver] Invalid capability types always have to raise an "invalid argument" error.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa4963968d40
[geckodriver] Invalid capability types always have to raise an "invalid argument" error. r=webdriver-reviewers,ato

Comment 5

[Andreas Tolfsen ❲:ato❳]

In hindsight, this change should’ve been noted in the changelog.
Leaving a comment here so we remember to do so when making the release.

Comment 6

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

I prefer to make changes to the changelog when we do the release work. Think about possible backouts, follow-ups which would cause further changes. We could add a whiteboard entry for those bugs where we think it makes sense to add a changelog entry.

Comment 7

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fa4963968d40