1398095 - WebDriver:{ExecuteScript,ExecuteAsyncScript} to use promises internally

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[David Burns :automatedtester]

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

[Andreas Tolfsen ❲:ato❳]

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

[Andreas Tolfsen ❲:ato❳]

The WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript
commands should use promises internally.

Comment 5

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

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

[Andreas Tolfsen ❲:ato❳]

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

Comment 7

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

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

[Andreas Tolfsen ❲:ato❳]

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.

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f57b94780b3080166446cbcab1d63be40059ee6c

Comment 10

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

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.

Comment 11

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

Attachment: working async scratchpad example

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.

Comment 12

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

Here the new try build without the await support:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49623603dda3eb3e704eb70cc345b4be7f23b5c

Comment 13

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

Attachment: Bug 1398095 - [wdspec] Use "script timeout" instead of "timeout error" for promise tests. r?ato

Comment 14

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

Attachment: Bug 1398095 - [marionette] Always clean-up registered timers and listeners in evaluate.sandbox().

Depends on D19751

Comment 15

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

Attachment: Bug 1398095 - [marionette] Use promises internally for Execute (Async) Script. r?ato

Depends on D19753

Comment 16

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

Attachment: Bug 1398095 - [wdspec] Add promise tests for Execute Async Script command. r?ato

Depends on D19755

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f390a56e3820
[wdspec] Use "script timeout" instead of "timeout error" for promise tests. r=ato

Comment 18

[Pulsebot]

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

[Daniel Varga [:dvarga]]

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

Comment 20

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

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

Comment 21

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

Upstream PR merged