Closed Bug 1452483 Opened 7 years ago Closed 7 years ago

Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace

Categories

(Remote Protocol :: Marionette, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(4 files)

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
Attachment #8966046 - Flags: review?(mjzffr) → review+
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
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: