Closed Bug 1452483 Opened 5 years ago Closed 5 years ago

Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace

Categories

(Testing :: Marionette, enhancement, P3)

Product:
Testing
Component:
Marionette
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(4 files)

Bug 1452483 - Set correct line number for JS evaluation.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1452483 - Strip whitespace from injected scripts.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1452483 - Use relative path to source file for injected scripts.
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1452483 - Preserve stacktrace from sandbox evaluation.
59 bytes, text/x-review-board-request
impossibus
: review+
Details 
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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P3
There appears to be a few test failures that I will look at tomorrow.
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
Summary: Evaluated script incorrectly assumes line 0 → Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace
Comment on attachment 8966046 [details]
Bug 1452483 - Set correct line number for JS evaluation.

https://reviewboard.mozilla.org/r/234768/#review241196
Attachment #8966046 - Flags: review?(mjzffr) → review+
Comment on attachment 8966174 [details]
Bug 1452483 - Strip whitespace from injected scripts.

https://reviewboard.mozilla.org/r/234908/#review241198
Attachment #8966174 - Flags: review?(mjzffr) → review+
Comment on attachment 8966175 [details]
Bug 1452483 - Use relative path to source file for injected scripts.

https://reviewboard.mozilla.org/r/234910/#review241200
Attachment #8966175 - Flags: review?(mjzffr) → 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?)
Attachment #8966176 - Flags: review?(mjzffr) → review+
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.
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
You need to log in before you can comment on or make changes to this bug.