Closed Bug 1337899 Opened 7 years ago Closed 6 years ago

Marionette rejects valid function body for "execute script"

Categories

(Remote Protocol :: Marionette, defect, P2)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: jugglinmike, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

17.02.08-geckodriver-script-err.log
5.19 KB, text/plain
Details
Bug 1337899 - [marionette] - Fix construction of script src.
59 bytes, text/x-review-board-request
ato
: review+
Details
Bug 1337899 - [wdspec] Add "Execute Script" test for final comment.
59 bytes, text/x-review-board-request
ato
: review+
Details 
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
Attachment #8996721 - Flags: review?(ato)
Attachment #8996722 - Flags: review?(ato)
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 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+
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.
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)
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.
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)
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
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
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!
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: