1337899 - Marionette rejects valid function body for "execute script"

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P2)

Description

[Mike Pennisi [:jugglinmike]]

Attachment: 17.02.08-geckodriver-script-err.log

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.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

[Andreas Tolfsen ❲:ato❳]

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

[Andreas Tolfsen ❲:ato❳]

Implementation of promises-based injected scripts: https://bugzilla.mozilla.org/show_bug.cgi?id=1335472

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1337899 - [marionette] - Fix construction of script src.

Marionette builds the final javascript source to execute by
using the specified body, and wrapping it into an anonymouse
function.

Because all of this happens in a single line, a comment in
the last line of the script body will cause the source only
half-wise constructed, and the post-wrapping code assumed
as comment too.

Review commit: https://reviewboard.mozilla.org/r/260782/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260782/

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.

Review commit: https://reviewboard.mozilla.org/r/260784/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260784/

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8996721 [details]
Bug 1337899 - [marionette] - Fix construction of script src.

https://reviewboard.mozilla.org/r/260782/#review267796

Clever fix!

Comment 8

[Andreas Tolfsen ❲:ato❳]

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?

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996722 [details]
Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260784/diff/1-2/

Comment 11

[Mozilla Autoland]

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 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996721 [details]
Bug 1337899 - [marionette] - Fix construction of script src.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260782/diff/1-2/

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996722 [details]
Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260784/diff/2-3/

Comment 14

[Mozilla Autoland]

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)

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996721 [details]
Bug 1337899 - [marionette] - Fix construction of script src.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260782/diff/2-3/

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996722 [details]
Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260784/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

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

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996721 [details]
Bug 1337899 - [marionette] - Fix construction of script src.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260782/diff/3-4/

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8996722 [details]
Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260784/diff/4-5/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b0fb26553824
https://hg.mozilla.org/mozilla-central/rev/068631fb32af

Comment 25

[Mike Pennisi [:jugglinmike]]

Thanks, folks!

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(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?

Comment 27

[James Graham [:jgraham]]

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.

Comment 28

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12736 for changes under testing/web-platform/tests

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks James!

Comment 30

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged