857238 - compartment mismatch with location in nsWindowSH::NewResolve with Xrays

Status: RESOLVED INCOMPLETE

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

I saw a couple of these on Aurora

DefinePropertyById
LocationSetter<nsIDOMWindow>
JSCompartment::wrap
nsWindowSH::NewResolve
xpc::XPCWrappedNativeXrayTraits::resolveOwnProperty

https://crash-stats.mozilla.com/report/index/d726d7fe-ae3f-45a2-abf5-0d37d2130326
https://crash-stats.mozilla.com/report/index/b37cf30c-57ae-41ff-a1aa-f721a2130326

Comment 1

[Bobby Holley (:bholley)]

Hm, so I think part of that stack is garbage, but my guess is that the call to JS_DefinePropertyById in the (sLocation_id == id) branch of nsWindowSH::NewResolve is busted in the Xray case:

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#5305

What happens in this function _looks_ highly suspicious when accessed over Xrays, but I can't actually find anything wrong with it. |cx| and |obj| should be same-compartment, |id| should be an interned atom, and we appear to be wrapping |v| into the cx compartment right before the DefineProperty call.

bz, see anything I don't in that code block?

Comment 2

[Boris Zbarsky [:bzbarsky]]

The obvious thing I think I see is that if WrapNative depends on cx and scope being same-compartment then there's no reason they would be in this case, right?

But apart from that, this code looks sane....

I wish the same-compartment assert were broken up into several separate asserts for the obj, value, id, etc...  then we'd know which one is failing.  :(

Comment 3

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> The obvious thing I think I see is that if WrapNative depends on cx and
> scope being same-compartment then there's no reason they would be in this
> case, right?

Yeah, my thoughts exactly. It looks like the WrapNative immediately invokes nsXPConnect::WrapNativeToJSVal, which invokes the static NativeInterface2JSObject doppelganger in nsXPConnect.cpp, which, as its first action, enters the compartment of aScope on the cx:

http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/nsXPConnect.cpp#1144


> I wish the same-compartment assert were broken up into several separate
> asserts for the obj, value, id, etc...  then we'd know which one is failing.
> :(

Me too!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, we can change that and see what crash-stats has to say for itself...

Or I guess we could try to reproduce, right?

Comment 5

[Doug Turner (:dougt)]

who can own this between you three? :)

Comment 6

[Andrew McCreight [:mccr8]]

The obvious zapping location with Xrays didn't cause any badness, so this is probably just some weird addon.  Given that none of the three of us can identify any problem here by inspection, I'm going to downgrade this to sec-other.  If it shows up some more, I'll consider landing some diagnostics, like maybe just splitting up the same compartment assertion.

Comment 7

[Andrew McCreight [:mccr8]]

This has changed enough in the last few years that I think it isn't worth keeping this open.