1743788 - Stop unconditionally using waiveXrays for "in content" script sandboxes

Status: NEW

(Remote Protocol :: Marionette, defect, P3)

Description

[James Graham [:jgraham]]

Currently marionette is simulating executing a script directly in content by creating a sandbox with wantXrays: false and using waiveXRays on the sandbox. that has a couple of problems:

• We want to remove wantXrays: False.
• When using objects returned from scripts, we actually do want X-rays for DOM objects to allow us to safely access the underlying object properties.

(There is also a third problem: the semantics of running in a sandbox are different to that of running in content in a way that's observable and causes compliance issues. But that should be considered a seperate issue).

I think we should create all sandboxes with Xrays enabled by default, and only waive Xrays as required (i.e. when actually executing the user-supplied script, or when serializing a non-DOM object).

Comment 1

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

The usage James is referring to can be found at:
https://searchfox.org/mozilla-central/rev/e9cd2997be1071b9bb76fc14df0f01a2bd721c30/remote/marionette/evaluate.js#546

Note that such a mutable sandbox is only created when a sandbox name has been specified. The parameter for the WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript is called sandbox. And that argument is not supported by the WebDriver spec and as such not accepted and forwarded to Marionette by geckodriver.

So this only applies to the Marionette client and other not official WebDriver clients that might make use of it.

Comment 2

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

Hm, but with bug 1491490 we want to have a similar behavior as with wantXRays set to false. So it means setting that value should be ok, but we should make use of expanded principals (see bug 1491490 comment 6)?

Comment 3

[James Graham [:jgraham]]

It's the opposite way around, we create a mutable sandbox when we don't have a sandbox name, which is the case for WebDriver execute[Async]Script commands.

I'm not sure if we need expanded principals here. We aren't giving the scripts any capabilities the content wouldn't have, apart from resolving the promise to indicate that script execution is complete.

Having said that, from a brief experiment I didn't get this working; I unset wantsXrays when creating the sandbox and tried to call waiveXrays when evaluating the script, but tests involving returning expando properties on DOM objects still failed, so clearly we're not getting the non-Xray view at the appropriate point.

Comment 4

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

We will most likely get the correct evaluation in content implemented in WebDriver BiDi first, and then update Marionette accordingly.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.