Marionette rejects valid function body for "execute script"

RESOLVED FIXED in Firefox 63

Status

defect
P2
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: jugglinmike, Assigned: whimboo)

Tracking

(Blocks 1 bug)

Version 3
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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.
Attachment #8835034 - Attachment mime type: text/x-log → text/plain
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)
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
Implementation of promises-based injected scripts: https://bugzilla.mozilla.org/show_bug.cgi?id=1335472
Priority: -- → P3
Blocks: webdriver
Priority: P3 → P2
(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)
(Assignee)

Updated

10 months ago
Attachment #8996721 - Flags: review?(ato)
(Assignee)

Updated

10 months ago
Attachment #8996722 - Flags: review?(ato)

Comment 7

10 months 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

10 months 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

10 months 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

10 months 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

10 months 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)
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

10 months 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
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)
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

10 months 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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b0fb26553824
https://hg.mozilla.org/mozilla-central/rev/068631fb32af
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Reporter)

Comment 25

10 months ago
Thanks, folks!
(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)
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
Thanks James!
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.