Investigate turning off Xrays for Marionette

RESOLVED FIXED

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jgriffin, Assigned: ato)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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.
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.
here's a try run with xrays turned off: https://tbpl.mozilla.org/?tree=Try&rev=87c873f382b8
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.
(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.
Updated tests to use x.constructor.name, and pushed to try again:  https://tbpl.mozilla.org/?tree=Try&rev=4fefb6f66db6
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
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?
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.
Depends on: 857138
This area of the code has been refactored heavily and from what I can see we no longer us xrays
(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?
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.
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.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
So, for context, this was fixed when bug 1123506 landed.
Assignee: nobody → ato
Depends on: 1123506
You need to log in before you can comment on or make changes to this bug.