1452483 - Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

Scripts evaluated by WebDriver:{ExecuteScript,ExecuteAsyncScript}
incorrectly assumes that the script starts at line 0 of the injected
resource.  This information is provided by the line parameters
provided to us, so we should use this.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1452483 - Set correct line number for JS evaluation.

Marionette incorrectly sets the JavaScript context line number to 0.
The line number is provided to us in in the input, so we should
use this.  The default fallback if line is not provided is 0 as before.

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

Comment 2

[Andreas Tolfsen ❲:ato❳]

There appears to be a few test failures that I will look at tomorrow.

Comment 3

[Andreas Tolfsen ❲:ato❳]

On further investigation it turns out the source of the problem is
the way we try to synthesise stacktrace for injected scripts.

A current stacktrace might look like this:

> execute_script @test_execute_async_script.py, line 63
> inline javascript, line 65
> src: "undefined"
> Stack:
> @test_execute_async_script.py:65:17
> @test_execute_async_script.py:63:59
> evaluate.sandbox/promise<@chrome://marionette/content/evaluate.js:163:13
> evaluate.sandbox@chrome://marionette/content/evaluate.js:114:17
> GeckoDriver.prototype.execute_@chrome://marionette/content/driver.js:1022:19
> async*GeckoDriver.prototype.executeAsyncScript@chrome://marionette/content/driver.js:960:27
> Async*despatch@chrome://marionette/content/server.js:293:20
> async*execute@chrome://marionette/content/server.js:267:11
> async*onPacket/<@chrome://marionette/content/server.js:242:15
> async*onPacket@chrome://marionette/content/server.js:241:8
> _onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9

We use the line number parameter to try to extract the offending
line from the script source, but the line number points to the point
of failure in the Python file’s stackframe; not in the anonymous script.

I will use this bug to fix the stacktraces Marionette produces once
and for all, so that it yields something of this nature:

> @testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:64:17
> @testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:63:59
> evaluate.sandbox/promise<@chrome://marionette/content/evaluate.js:163:13
> evaluate.sandbox@chrome://marionette/content/evaluate.js:114:17
> GeckoDriver.prototype.execute_@chrome://marionette/content/driver.js:1022:19
> async*GeckoDriver.prototype.executeAsyncScript@chrome://marionette/content/driver.js:960:27
> Async*despatch@chrome://marionette/content/server.js:293:20
> async*execute@chrome://marionette/content/server.js:267:11
> async*onPacket/<@chrome://marionette/content/server.js:242:15
> async*onPacket@chrome://marionette/content/server.js:241:8
> _onJSONObjectReady/<@chrome://marionette/content/transport.js:500:9

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1452483 - Strip whitespace from injected scripts.

The injected script may contain a lot of whitespace padding on
either side of the string when using multi-line strings ("""foo""")
in Python.  To improve readability of the trace log we can strip
it off before sending it to Marionette.

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

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1452483 - Use relative path to source file for injected scripts.

The basename is not sufficient to locate the file.  Using the file's
relative path will match the behaviour of JavaScript stacktraces.

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

Comment 6

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1452483 - Preserve stacktrace from sandbox evaluation.

Now that the correct filename and line number is being passed to
Cu.evalInSandbox, the stacktrace of the thrown error is correct.
JavaScriptError uses the line number to index the injected source
script, but the line number refers to the file represented by the
"filename" parameter and not to the script.

This effectively means that the line numbers in the produced
stacktrace are wrong because line number 0 was hard-coded as an
argument to Cu.evalInSandbox.

This patch harmonises the stacktraces returned from
WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript with
stacktraces from normal JavaScript errors, by removing some extra clutter.

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

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966046 [details]
Bug 1452483 - Set correct line number for JS evaluation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234768/diff/1-2/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966046 [details]
Bug 1452483 - Set correct line number for JS evaluation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234768/diff/2-3/

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966174 [details]
Bug 1452483 - Strip whitespace from injected scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234908/diff/1-2/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966175 [details]
Bug 1452483 - Use relative path to source file for injected scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234910/diff/1-2/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966176 [details]
Bug 1452483 - Preserve stacktrace from sandbox evaluation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234912/diff/1-2/

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8966046 [details]
Bug 1452483 - Set correct line number for JS evaluation.

https://reviewboard.mozilla.org/r/234768/#review241196

Comment 13

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8966174 [details]
Bug 1452483 - Strip whitespace from injected scripts.

https://reviewboard.mozilla.org/r/234908/#review241198

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8966175 [details]
Bug 1452483 - Use relative path to source file for injected scripts.

https://reviewboard.mozilla.org/r/234910/#review241200

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8966176 [details]
Bug 1452483 - Preserve stacktrace from sandbox evaluation.

https://reviewboard.mozilla.org/r/234912/#review241204

r+wc

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:68
(Diff revision 2)
>                  let a = 1;
>                  foo(bar);
>                  """)
> -            self.assertFalse(True)
> -        except JavascriptException as inst:
> -            self.assertTrue('foo(bar)' in inst.stacktrace)
> +            self.fail()
> +        except JavascriptException as e:
> +            self.assertIsNotNone(e.stacktrace)

What's the benefit of removing the foo(bar) check? Is it not relevant?

::: testing/marionette/harness/marionette_harness/tests/unit/test_execute_async_script.py:76
(Diff revision 2)
>      def test_execute_async_js_exception(self):
> -        self.assertRaises(JavascriptException,
> -            self.marionette.execute_async_script, """
> -            var callback = arguments[arguments.length - 1];
> -            callback(foo());
> +        try:
> +            self.marionette.execute_async_script("""
> +                let [resolve] = arguments;
> +                resolve(foo());
>              """)

Should add a `self.fail` here, too, in case the JavascriptException is not thrown.

::: testing/marionette/test_error.js:138
(Diff revision 2)
>    let e1 = new WebDriverError("a");
>    let e1s = e1.toJSON();
>    equal(e1s.message, e1.message);
>    equal(e1s.stacktrace, e1.stack);
>  
> -  let e2 = new JavaScriptError("first", {
> +  let e2 = new JavaScriptError("foo")

missing ; (Shouldn't the linter point that out?)

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966176 [details]
Bug 1452483 - Preserve stacktrace from sandbox evaluation.

https://reviewboard.mozilla.org/r/234912/#review241204

> What's the benefit of removing the foo(bar) check? Is it not relevant?

The new stacktrace no longer contains information about the failing
line of source code.

> missing ; (Shouldn't the linter point that out?)

We don’t run the linter on our xpcshell tests.

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966046 [details]
Bug 1452483 - Set correct line number for JS evaluation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234768/diff/3-4/

Comment 18

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966174 [details]
Bug 1452483 - Strip whitespace from injected scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234908/diff/2-3/

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966175 [details]
Bug 1452483 - Use relative path to source file for injected scripts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234910/diff/2-3/

Comment 20

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8966176 [details]
Bug 1452483 - Preserve stacktrace from sandbox evaluation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234912/diff/2-3/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e73d9a52e2e4
Set correct line number for JS evaluation. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/3d1f361a7a4f
Strip whitespace from injected scripts. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/956ec8535d67
Use relative path to source file for injected scripts. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/14b2d3f79612
Preserve stacktrace from sandbox evaluation. r=maja_zf

Comment 22

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/e73d9a52e2e4
https://hg.mozilla.org/mozilla-central/rev/3d1f361a7a4f
https://hg.mozilla.org/mozilla-central/rev/956ec8535d67
https://hg.mozilla.org/mozilla-central/rev/14b2d3f79612