1413326 - Consider not including stacktraces when returning WebDriver errors

Status: RESOLVED INACTIVE

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

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.

Comment 1

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

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.

Comment 2

[Andreas Tolfsen ❲:ato❳]

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.

Comment 3

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

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

Comment 4

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 5

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

Fair point. Maybe it would be good to have a patch at some point and play with it.

Comment 6

[David Burns :automatedtester]

[Mass Change 2018-01-15] Moving bugs to backlog

Comment 7

[Andreas Tolfsen ❲:ato❳]

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.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1413326 - Discard stacktraces of WebDriver errors in response.

This patch changes Marionette to discard stacktraces from errors
that extends the WebDriverError prototype.  This has the effect that
programming errors (such as TypeError, RangeError, et al.) will
be sent with a stacktrace because WebDriver related errors are
considered "expected".

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.

Programming errors will also be logged in full through error.report
in stdout, as usual.

Review commit: https://reviewboard.mozilla.org/r/213584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213584/

Comment 9

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

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.

Comment 10

[Andreas Tolfsen ❲:ato❳]

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 11

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

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 12

[Andreas Tolfsen ❲:ato❳]

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?

Comment 13

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

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 14

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

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.

Comment 15

[Andreas Tolfsen ❲:ato❳]

Not currently working on this.

Comment 16

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

Some interesting bug from the backlog that might be worth triaging. I assume that at least for UnknownError we should keep the stack trace alive.

Comment 17

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

As it can be seen in the logs of wdspec jobs the stacks for all errors are forwarded via geckodriver:
https://treeherder.mozilla.org/logviewer?job_id=403873455&repo=mozilla-central&lineNumber=88331-88361

We discussed in the triage meeting and decided to close this bug for now. If there is still interest in the future we could reopen it.