Closed
Bug 1337899
Opened 8 years ago
Closed 6 years ago
Marionette rejects valid function body for "execute script"
Categories
(Remote Protocol :: Marionette, defect, P2)
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: jugglinmike, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
Attachments
(3 files)
When the function body specified for "execute script" ends in a single-line comment, Marionette fails with an error stating, "SyntaxError: missing } after function body". I can reproduce this using GeckoDriver and both Firefox stable build (51.0.1) and Nightly (54.0a1 - 2017-02-07) on x64 Linux. I've attached the relevant log from GeckoDriver.
Assignee | ||
Updated•8 years ago
|
Attachment #8835034 -
Attachment mime type: text/x-log → text/plain
Assignee | ||
Comment 1•8 years ago
|
||
I can see the same when using the following snippet in a Marionette test:
> self.marionette.execute_script("""//""")
Workaround is to move the final closing double quotes into the next line.
Stack info:
stacktrace:
execute_script @test_minimized.py, line 26
inline javascript, line 0
src: "//"
Stack:
Traceback (most recent call last):
File "/Volumes/data/code/gecko/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 166, in run
testMethod()
File "/Volumes/data/code/gecko/_a/test_minimized.py", line 26, in test
self.marionette.execute_script("""//""")
File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 1797, in execute_script
rv = self._send_message("executeScript", body, key="value")
File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/decorators.py", line 23, in _
return func(*args, **kwargs)
File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 729, in _send_message
self._handle_error(err)
File "/Volumes/data/code/gecko/testing/marionette/client/marionette_driver/marionette.py", line 762, in _handle_error
raise errors.lookup(error)(message, stacktrace=stacktrace)
Comment 2•8 years ago
|
||
WebDriver recently decided to change how injected code is executed to wrap them in promises, and that would necessitate changes in this area for Marionette. I wonder if it makes sense to fix this and align us to the spec at the same time. Discussion: https://github.com/w3c/webdriver/issues/381 Proposal: https://github.com/w3c/webdriver/pull/755
Comment 3•8 years ago
|
||
Implementation of promises-based injected scripts: https://bugzilla.mozilla.org/show_bug.cgi?id=1335472
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Andreas Tolfsen 「:ato」 from comment #3) > Implementation of promises-based injected scripts: > https://bugzilla.mozilla.org/show_bug.cgi?id=1335472 The problem actually lays in our evaluation code and has nothing to do with Promise handling. Thing is that we use a template string to concatenate the script content with args into `src`: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/testing/marionette/evaluate.js#118 > src += `(function() { ${script} }).apply(null, ${ARGUMENTS})`; If the script contains a `//` which starts a comment, the rest of the line will be a comment. This also includes the closing bracket right after the script variable. Solution here is to split-up this single line into multiple ones: > src += `(function() { > ${script} > }).apply(null, ${ARGUMENTS})`; Now it doesn't matter what the script contains, it will no longer affect us.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8996721 -
Flags: review?(ato)
Assignee | ||
Updated•6 years ago
|
Attachment #8996722 -
Flags: review?(ato)
Comment 7•6 years ago
|
||
mozreview-review |
Comment on attachment 8996721 [details] Bug 1337899 - [marionette] - Fix construction of script src. https://reviewboard.mozilla.org/r/260782/#review267796 Clever fix!
Attachment #8996721 -
Flags: review?(ato) → review+
Comment 8•6 years ago
|
||
mozreview-review |
Comment on attachment 8996722 [details] Bug 1337899 - [wdspec] Add "Execute Script" test for final comment. https://reviewboard.mozilla.org/r/260784/#review267798 ::: testing/web-platform/tests/webdriver/tests/execute_script/execute.py:17 (Diff revision 1) > +def test_ending_comment(session): > + response = execute_script(session, "return 1; // foo") > + assert_success(response, 1) I think you’ve forgotten to regenerate the manifest for this commit?
Attachment #8996722 -
Flags: review?(ato) → review+
Assignee | ||
Comment 9•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8996722 [details] Bug 1337899 - [wdspec] Add "Execute Script" test for final comment. https://reviewboard.mozilla.org/r/260784/#review267798 > I think you’ve forgotten to regenerate the manifest for this commit? Good catch. I'm going to forget about this all the time.
Comment hidden (mozreview-request) |
Comment 11•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s b2053e4cbee0b880dbe4ac5413b67d868ceaaf82 -d 592fdc17e2d6: rebasing 476276:b2053e4cbee0 "Bug 1337899 - [marionette] - Fix construction of script src. r=ato" merging testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py rebasing 476277:39891d272430 "Bug 1337899 - [wdspec] Add "Execute Script" test for final comment. r=ato" (tip) merging testing/web-platform/meta/MANIFEST.json merging testing/web-platform/tests/webdriver/tests/execute_script/execute.py warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark') warning: conflicts while merging testing/web-platform/tests/webdriver/tests/execute_script/execute.py! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 6296476b329e58d18e043c84db868be8fdea0fa3 -d 6148cbf8e14c: rebasing 476286:6296476b329e "Bug 1337899 - [marionette] - Fix construction of script src. r=ato" merging testing/marionette/harness/marionette_harness/tests/unit/test_execute_script.py rebasing 476287:5a0e2211cec5 "Bug 1337899 - [wdspec] Add "Execute Script" test for final comment. r=ato" (tip) merging testing/web-platform/tests/webdriver/tests/execute_script/execute.py warning: conflicts while merging testing/web-platform/tests/webdriver/tests/execute_script/execute.py! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Assignee | ||
Comment 15•6 years ago
|
||
Something which is currently on autoland blocks me from rebasing my patch. I will try again tomorrow morning after the next merge to mozilla-central.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/22c54c6cc370 [marionette] - Fix construction of script src. r=ato https://hg.mozilla.org/integration/autoland/rev/998c36eeb67e [wdspec] Add "Execute Script" test for final comment. r=ato
Comment 19•6 years ago
|
||
Backed out for linting opt failure on web-platform/meta/MANIFEST.json Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-searchStr=Linting%20opt%20source-test-mozlint-wptlint-gecko%20(W)&fromchange=998c36eeb67e50502639515294a67b2efc82e577&tochange=f8bfad6ea788186cbb322a899c7e39f6f900abe1&selectedJob=191525979 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191525979&repo=autoland&lineNumber=290 Backout link: https://hg.mozilla.org/integration/autoland/rev/f8bfad6ea788186cbb322a899c7e39f6f900abe1 [task 2018-08-02T04:50:22.766Z] 0:18.12 ERROR Manifest /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json is outdated, use |mach wpt-manifest-update| to fix. [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/top_level.txt in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/dependency_links.txt in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/requires.txt in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/not-zip-safe in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/third_party/atomicwrites/tests/test_atomicwrites.py in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/PKG-INFO in manifest but removed from source. (wpt-manifest) [task 2018-08-02T04:53:55.296Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/testing/web-platform/meta/MANIFEST.json:0 | tools/wptserve/wptserve.egg-info/SOURCES.txt in manifest but removed from source. (wpt-manifest) [taskcluster 2018-08-02 04:53:55.600Z] === Task Finished === [taskcluster 2018-08-02 04:53:55.600Z] Unsuccessful task run with exit code: 1 completed in 511.149 seconds
Flags: needinfo?(hskupin)
Assignee | ||
Comment 20•6 years ago
|
||
Looks like this is a conflict with the landing of the following patch and it's backout: https://hg.mozilla.org/integration/autoland/rev/de933cb380f9 I will update the MANIFEST.json to remove all those failing entries.
Flags: needinfo?(hskupin)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•6 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b0fb26553824 [marionette] - Fix construction of script src. r=ato https://hg.mozilla.org/integration/autoland/rev/068631fb32af [wdspec] Add "Execute Script" test for final comment. r=ato
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b0fb26553824 https://hg.mozilla.org/mozilla-central/rev/068631fb32af
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Reporter | ||
Comment 25•6 years ago
|
||
Thanks, folks!
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Pulsebot from comment #23) > https://hg.mozilla.org/integration/autoland/rev/068631fb32af > [wdspec] Add "Execute Script" test for final comment. r=ato James, this changeset never got synced upstream. Can you please check?
Flags: needinfo?(james)
Comment 27•6 years ago
|
||
Ah, the combination of a backout and a merge confict seems to cause problems; it was marked as incomplete so it didn't appear on my list of open upstream syncs that had errors and presumbably when it relanded we didn't update the status. I'm manually handling it now.
Flags: needinfo?(james)
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12736 for changes under testing/web-platform/tests
Assignee | ||
Comment 29•6 years ago
|
||
Thanks James!
Upstream PR merged
Updated•2 years ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•