853287 - Investigate turning off Xrays for Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

Marionette currently creates sandboxes with wantXrays=true, which according to bholley, exposes us to some edge cases that are hard to debug/fix.  We may be better off running with them.

I don't recall the exact reason why we run with them on, so we should try turning them off and see if anything breaks.

Comment 1

[Bobby Holley (:bholley)]

In an ideal world, I'd be in favor of keeping marionette scripts on Xrays, because it would force us to find and fix all the places where same-origin Xray vision isn't entirely invisible to the consumer. But in practice, marionette failures tend to be a pain to debug, and I'm guessing developers don't realize that their code is using Xray vision to access the associated DOM.

Comment 2

[Jonathan Griffin (:jgriffin)]

here's a try run with xrays turned off: https://tbpl.mozilla.org/?tree=Try&rev=87c873f382b8

Comment 3

[Jonathan Griffin (:jgriffin)]

with xrays turned off, we get a number of test failures, all of which are assertions that foo is an instanceof Date.  e.g.,

http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_incoming.js#35

bholley, any idea why this might be happening?   Other instanceof checks are passing.

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jonathan Griffin (:jgriffin) from comment #3)
> bholley, any idea why this might be happening?   Other instanceof checks are
> passing.

Yeah. In pure JS land |x instanceof y| literally checks whether y is on the prototype chain of x. So if |x| is a Date comes from a scope whose global |g| is different from yours, |x instanceof Date| will fail, even though |x instanceof g.Date| will pass. We sometimes do funny shenanigans for XPCOM things, which is why some of the other checks might be passing.

In other words, instanceof is kinda broken on the Web, but the spec community doesn't seem too interested in fixing it... :-(

Do we have an easy way to get to |g| here? The other solution is just to replace the check with |is(x.constructor.name, "Date", ...)|, which should work regardless of what scope the objects are in.

Comment 5

[Jonathan Griffin (:jgriffin)]

Updated tests to use x.constructor.name, and pushed to try again:  https://tbpl.mozilla.org/?tree=Try&rev=4fefb6f66db6

Comment 6

[Jonathan Griffin (:jgriffin)]

There's still one failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=22291090&tree=Try&full=1#error0

In this test, we construct some dates locally, and then pass them to SMS's getMessages() using an nsIDOMSmsFilter:  http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/interfaces/nsIDOMSmsFilter.idl

With xrays, this works fine, without it fails.

Failing test:  http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/tests/marionette/test_filter_date_notfound.js#148

Comment 7

[Jonathan Griffin (:jgriffin)]

I stepped through the SMS filter code, and without xrays, the JS Date object that's passed to SmsFilter::SetStartDate is interpreted as 0 by js_DateGetMsecSinceEpoch() at http://mxr.mozilla.org/mozilla-central/source/dom/mobilemessage/src/SmsFilter.cpp#85.

:bholley, does this seem like a bug in SmsFilter, or is something else going on here?

Comment 8

[Bobby Holley (:bholley)]

What's almost certainly happening here is that we're ending up with a cross-compartment wrapper to a Date rather than a Date itself (Xrays generally shield you from entering foreign compartments, and without them you'll be doing that a lot more).

The JS_ObjectIsDate() check works, because JS_ObjectIsDate uses ObjectClassIs, which knows how to forward through cross-compartment wrappers. But js_DateGetMsecSinceEpoch just checks ->isDate(), and returns 0 if it isn't one.

The bug here is that js_DateGetMsecSinceEpoch doesn't use CallNonGenericMethod, even though it should. It's a relatively trivial change - if you look at jsdate.cpp you'll get a very clear idea of how it's supposed to look.

So please file a bug. If you're comfortable fixing it to work like the other methods in jsdate please do, otherwise I'm happy to write the patch.

Comment 9

[David Burns :automatedtester]

This area of the code has been refactored heavily and from what I can see we no longer us xrays

Comment 10

[Bobby Holley (:bholley)]

(In reply to David Burns :automatedtester from comment #9)
> This area of the code has been refactored heavily and from what I can see we
> no longer us xrays

Xrays are the default for code running in a sandbox - do you see us disabling or waiving them somewhere?

Comment 11

[Andreas Tolfsen ❲:ato❳]

I won’t pretend I understand all the nuances of sandboxes and Xrays, but as far as I can tell we still use Xrays for all Mozilla tests and harnesses that execute scripts through Marionette.

Bug 1123506 rewrote the subsystem for evaluating scripts in Marionette and introduces a new non-Xray evaluation method implemented in sandbox.createMutable:

    https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/evaluate.js#L231

This function explicitly disables Xrays, but it’s only used by WebDriver conformant clients.  The Python Marionette client (testing/marionette/client) still uses the Xray-enabled sandbox.create:

    https://github.com/mozilla/gecko-dev/blob/master/testing/marionette/evaluate.js#L210

I don’t think there is any particular reason we couldn’t run most Mozilla tests without Xray, but we will have to go through a lot of code to update references to window.wrappedJSObject and clean up certain expectations about state between calls to the Execute Script command.

Comment 12

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

We use wrappedJSObject for mostly all the calls but not waveXraxs(). See also bug 1287904 for a side-effect of wrappedJSObject in combination with a xul element.

Comment 13

[Andreas Tolfsen ❲:ato❳]

So, for context, this was fixed when bug 1123506 landed.