Closed Bug 1583761 Opened 2 months ago Closed 2 months ago

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

Categories

(Testing :: geckodriver, defect, P1)

Version 3
defect

Tracking

(firefox71 fixed)

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file)

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.

Flags: needinfo?(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?

Flags: needinfo?(ato)

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

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Blocks: 1573798
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fa4963968d40
[geckodriver] Invalid capability types always have to raise an "invalid argument" error. r=webdriver-reviewers,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.

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.

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.