Open Bug 1413326 Opened 2 years ago Updated Last year

Consider not including stacktraces when returning WebDriver errors

Categories

(Testing :: Marionette, enhancement, P3)

Version 3
enhancement

Tracking

(Not tracked)

People

(Reporter: ato, Unassigned)

Details

Attachments

(1 file)

For reasons similar to https://bugzil.la/1413292 we should consider
not including stacktraces in WebDriver errors.  When Marionette
returns with an ‘element not found’ error, this is intended
behaviour and a stacktrace is of extremely limited value.  Indeed,
including the stacktrace might mislead users to think that the
returned error is an implementation problem in the driver.
I agree here, but would consider to leave in a vendor specific capability to keep it? In case of real coding issues I still want to see a stacktrace.
We have the ability to distinguish between errors of the
WebDriverError prototype chain and those that are not
(i.e. TypeError, SyntaxError, et al.), so my thinking was that we
could use that to determine which errors to include the stacks for.
It would work as long as we do not catch an eg. `TypeError` and re-throw as `InvalidArgumentError`.
(In reply to Henrik Skupin (:whimboo) from comment #3)

> It would work as long as we do not catch an eg. `TypeError` and
> re-throw as `InvalidArgumentError`.

Yes.  But these errors are intended behaviour, right?

If a client passes the wrong input to a command and we catch a
TypeError and re-throw it as InvalidArgumentError, it means we
explicitly consider this the correct behaviour according to the
specification and not a programming error in Marionette.
Fair point. Maybe it would be good to have a patch at some point and play with it.
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Another, admittedly selfish, reason for not including a stacktrace
when we return WebDriver errors is that I subscribe to most bugs in
Bugzilla and filter out those that include the “marionette” string.
Since the Marionette harness is used for a great number of the jobs
that run on try, a lot of the intermittent bugs contain this term
in path names in the stacktraces.
Assignee: nobody → ato
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219438

I have to say that I'm not a big fan of that by turning it completely off. The stacks are actually helpful even for WebDriver based exceptions when investigating log files for problems in the JS code. If I would only see the message it would require a lot of searching in the codebase. Maybe we should make that decision based on the log level, and always include the log in debug and trace, but not info. Let me know what you think.
Attachment #8943257 - Flags: review?(hskupin)
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219438

But they’re not exceptions in JS code.  They are expected errors.
In https://bugzil.la/1413292 we decided not to generate Rust
backtraces for problems originating in Rust code, and I don’t see
how this is any different.
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219438

Well, lets say we inappropriately throw an ElementNotInteractableError for a command. For users it might be fine to just see that, but how do we know where exactly this was raised? Especially if a command is doing different checks, and could raise this type of error at different places? Also we don't know if we catch a different error at another line, and just re-throw as a type of WebDriverError.

Again, it will make it very hard for debugging.
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219438

If we inappropriately throw a WebDriver error, by which I’m
assuming you’re talking about masking a JS programming error,
it will typically be done with a try { … } (catch e) { throw
ElementNotInteractableError(`Could not interact with element:
${el}`); }.  Here a programming error causes a JS error to throw
inside the try-block and that gets coerced to a kind of error for
which we throw away the stacktrace on serialisation.

In this situation the stacktrace wouldn’t be of so much help because
it would originate from where the ElementNotInteractableError is
constructed; not where the original problem occurred.  If the e
variable had been passed to ElementNotInteractableError’s ctor,
the story would be very different and I might be inclined to agree
with you.  The reality is, however, that JS does not implicitly
preserve error chains like Python or Rust does.

I’d argue that it is more confusing to users to include an
essentially pointless stacktrace, primarily because it misleads
them to think there is a problem with geckodriver.  I also believe
this more easily leads them to suspect the problem is not in their
code but in geckodriver, which generates more false-positive bug
reports for us to look at.

The bottom line here is that I don’t think this change will be such
a big deal as you make it out to be, and the alternative of always
returning errors with stacktraces from Marionette but throwing them
away in geckodriver would be a more complicated change to make.

Do you have any alternative proposals?
Flags: needinfo?(hskupin)
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219778

Something with the try build went terribly wrong. Please check that, and maybe retrigger.
Comment on attachment 8943257 [details]
Bug 1413326 - Discard stacktraces of WebDriver errors in response.

https://reviewboard.mozilla.org/r/213584/#review219438

If you think it's all fine then lets land it. If something doesn't play well, we could iterate afterward.
Flags: needinfo?(hskupin)
Not currently working on this.
Assignee: ato → nobody
You need to log in before you can comment on or make changes to this bug.