Closed Bug 1398095 Opened 2 years ago Closed 8 months ago

WebDriver:{ExecuteScript,ExecuteAsyncScript} to use promises internally

Categories

(Testing :: Marionette, enhancement, P1)

Version 3
enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
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
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.
Summary: Make Execute Script/Execute Async Script spec conforming. → Make Execute Script/Execute Async Script spec conforming
Depends on: 1419317
The WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript
commands should use promises internally.
Summary: Make Execute Script/Execute Async Script spec conforming → WebDriver:{ExecuteScript,ExecuteAsyncScript} to use promises internally
Duplicate of this bug: 1335472
Duplicate of this bug: 1503252
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.
Depends on: 1503238
Blocks: 1121698

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?

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
Priority: P2 → P1

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.

Flags: needinfo?(ato)

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.

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.

Blocks: 1527713
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
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/15417 for changes under testing/web-platform/tests
Upstream PR merged
You need to log in before you can comment on or make changes to this bug.