WebDriver:{ExecuteScript,ExecuteAsyncScript} to use promises internally
Categories
(Remote Protocol :: Marionette, enhancement, P1)
Tracking
(firefox67 fixed)
| Tracking | Status | |
|---|---|---|
| firefox67 | --- | fixed |
People
(Reporter: automatedtester, Assigned: whimboo)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files)
We need to make sure that our version of the execute[Async]Scipt is conforming to the specification. We have https://github.com/mozilla/geckodriver/issues/904 with an example
|
||
Comment 1•6 years ago
|
||
I’m not sure what this bug actually covers. Can we get a better description of what changes are necessary? Happy to use this for tracking.
|
|
||
Comment 2•6 years ago
|
||
The WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript commands should use promises internally.
|
Assignee | |
Comment 5•5 years ago
|
||
Note that we will get new tests landed soon via bug 1503238, which can be used to ensure that a fix for this bug works as expected.
|
|
||
Comment 6•5 years ago
|
||
Following the tests, we are failing six WPT tests related to promises:
https://wpt.fyi/results/webdriver/tests/execute_script/promise.py?label=master&label=experimental&product=chrome&product=firefox&product=safari
|
|
Assignee | |
Comment 7•5 years ago
|
||
So I got all of those ExecuteScript tests working locally. Actually the tests were faulty too, because they checked for "timeout error" which first doesn't exist, and second should be "script timeout".
I wonder why we don't have those tests for ExecuteAsyncScript, because wrapping the code with a Promise has to happen there too. Andreas, you reviewed the tests for ExecuteScript, so maybe you can remember?
|
|
||
Comment 8•5 years ago
|
||
It’s good that you got the tests working!
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)
I wonder why we don't have those tests for ExecuteAsyncScript,
because wrapping the code with a Promise has to happen there
too. Andreas, you reviewed the tests for ExecuteScript, so maybe
you can remember?
It seems like an oversight since both the sync and the async command
uses the same promise-based internal language.
|
|
Assignee | |
Comment 9•5 years ago
|
||
|
|
Assignee | |
Comment 10•5 years ago
|
||
The try build shows a couple of failures across Mn and wpt jobs which are all related to the addition of async to allow scripts to use await. The following is visible in the log files:
[Child 56590, Main Thread] WARNING: Silently denied access to property "toJSON": value is callable (@chrome://marionette/content/evaluate.js:281:0): file /builds/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp, line 223
Interestingly this only happens when calling scripts in content scope. In chrome scope it works fine. So whether this is again a problem in Firefox (similar to bug 1453057), or it's in the Marionette code maybe when sending the data to the parent process.
|
|
Assignee | |
Comment 11•5 years ago
|
||
I replicated the code of Marionette for Marionette, and when it gets run in the browser environment it works. I cannot run for content because it's not a framescript and wouldn't have the permissions to access Cu.
So the code itself should be fine, but there is a problem in Firefox which doesn't allow the object to be JSON serialized.
I would really suggest that we move the await support to a follow-up bug, which will then also make it easier for Firefox devs to check.
|
|
Assignee | |
Comment 12•5 years ago
|
||
Here the new try build without the await support:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49623603dda3eb3e704eb70cc345b4be7f23b5c
|
|
Assignee | |
Comment 13•5 years ago
|
||
|
|
Assignee | |
Comment 14•5 years ago
|
||
Depends on D19751
|
|
Assignee | |
Comment 15•5 years ago
|
||
Depends on D19753
|
|
Assignee | |
Comment 16•5 years ago
|
||
Depends on D19755
|
||
Comment 17•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f390a56e3820 [wdspec] Use "script timeout" instead of "timeout error" for promise tests. r=ato
|
|
||
Comment 18•5 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/38fe474b9025 [marionette] Always clean-up registered timers and listeners in evaluate.sandbox(). r=ato https://hg.mozilla.org/integration/autoland/rev/c69465c47848 [marionette] Use promises internally for Execute (Async) Script. r=ato https://hg.mozilla.org/integration/autoland/rev/b8393b9641e5 [wdspec] Add promise tests for Execute Async Script command. r=ato
|
||
Comment 19•5 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/f390a56e3820
https://hg.mozilla.org/mozilla-central/rev/38fe474b9025
https://hg.mozilla.org/mozilla-central/rev/c69465c47848
https://hg.mozilla.org/mozilla-central/rev/b8393b9641e5
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15417 for changes under testing/web-platform/tests
Upstream PR merged
|
||
Updated•7 months ago
|
Description
•