Andreas Tolfsen ❲:ato❳ Assignee
	Description • 6 years ago

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.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 6 years ago

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

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 3 • 6 years ago

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 12 • 6 years ago
mozreview-review

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

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 13 • 6 years ago
mozreview-review

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

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 14 • 6 years ago
mozreview-review

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

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

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 15 • 6 years ago
mozreview-review

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?)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 16 • 6 years ago
mozreview-review-reply

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.

	Pulsebot
	Comment 21 • 6 years ago

Pushed by atolfsen@mozilla.com:
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

	Andreea Pavel [:apavel]
	Comment 22 • 6 years ago
bugherder

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