Closed
Bug 1452483
Opened 6 years ago
Closed 6 years ago
Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace
Categories
(Remote Protocol :: Marionette, enhancement, P3)
Remote Protocol
Marionette
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 | ||
Updated•6 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•6 years ago
|
||
There appears to be a few test failures that I will look at tomorrow.
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
Summary: Evaluated script incorrectly assumes line 0 → Fix WebDriver:{ExecuteScript,ExecuteAsyncScript} stacktrace
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
Attachment #8966046 -
Flags: review?(mjzffr) → review+
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
Attachment #8966174 -
Flags: review?(mjzffr) → review+
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
Attachment #8966175 -
Flags: review?(mjzffr) → review+
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?)
Attachment #8966176 -
Flags: review?(mjzffr) → review+
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
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
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•