Andreas Tolfsen ❲:ato❳ Reporter
	Description • 7 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 1 • 7 years ago

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 2 • 7 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 3 • 7 years ago

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

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 4 • 7 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 5 • 7 years ago

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

	David Burns :automatedtester
	Comment 6 • 6 years ago

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

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 7 • 6 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 9 • 6 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 10 • 6 years ago
mozreview-review-reply

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 11 • 6 years ago
mozreview-review-reply

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 12 • 6 years ago
mozreview-review-reply

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?

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 13 • 6 years ago
mozreview-review

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.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 14 • 6 years ago
mozreview-review-reply

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.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 15 • 6 years ago

Not currently working on this.