Open Bug 774869 Opened 12 years ago Updated 2 years ago

comparison of two variables which point to the same XUL node using == fails in Marionette

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

People

(Reporter: jgriffin, Unassigned)

References

Details

In Marionette, there is a test which compares two variables that point to the same XUL node.  The comparison evaluates to false, even though a string representation of the nodes is equal (e.g., [object XULElement @ 0x7f2144dd4040 (native @ 0x7f2144c510d0)]).

I attempted to create a mochitest-chrome test for this problem, but was unable to reproduce it that way; apparently the problem is somewhat subtle.

To reproduce it using Marionette:

- download a debug Firefox build from TBPL, or make your own debug build with ENABLE_MARIONETTE=1 in your mozconfig
- add the pref marionette.defaultPrefs.enabled = true, and restart Firefox
- go to mozilla-central/testing/marionette/client/marionette
- execute the following to run the test (this assumes your browser is still running from the above):
   python runtests.py --address localhost:2828 tests/unit/test_findelement.py

Results:  All the chrome tests (the tests executed in chrome context) fail, but similar tests run in the content context pass.

The comparison that is failing is here:

http://mxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-elements.js#65

...even though both variables being compared point to the same XUL node, and have the same string representation.
Bobby, does == unwrap various wrappers?
(In reply to Boris Zbarsky (:bz) from comment #1)
> Bobby, does == unwrap various wrappers?

What kind of wrappers are you curious about? Anyway, generally not.

Sometimes this happens with Xray waiving, where one object is the waiver identity (XPCWrappedNative.unwrap(obj) or obj.wrappedJSObject) and the other isn't. What happens if you try putting the objects on both sides of the == in XPCNativeWrapper()?

i.e., if a == b fails, try XPCNativeWrapper(a) == XPCNativeWrapper(b).
> What kind of wrappers are you curious about?

Well, for example CrossCompartmentWrappers around two Xrays that come from different compartments around the same DOM object.  Assuming that can happen.

Jonathan, from a look at the code, it looks like some of the things here run in different sandboxes.  Is it possible that the two references the code is comparing came out of different sandboxes?
(In reply to Boris Zbarsky (:bz) from comment #3)
> > What kind of wrappers are you curious about?
> 
> Well, for example CrossCompartmentWrappers around two Xrays that come from
> different compartments around the same DOM object.  Assuming that can happen.

No. We unwrapping cross-compartment wrappers when re-wrapping. If the node forces a parent with PreCreate, the canonical XPCWN lives in that compartment, and everybody else gets a cross-compartment wrapper to it. Otherwise, we get one XPCWN per scope.

XUL elements force a parent in PreCreate, so we should have one canonical object here. Though if the precreate hook were messing up somehow, that could explain how we might get two different wrappers.
> We unwrapping cross-compartment wrappers when re-wrapping. 

So if I have a sandbox and I touch some DOM node from there, inside the sandbox I get an Xray for it.  If I then return that Xray out of the sandbox (to code running in the chrome), what do I get in that code?  Do I get another Xray around the original object?  Or a CrossCompartmentWrapper around the first Xray?
(In reply to Boris Zbarsky (:bz) from comment #5)
> > We unwrapping cross-compartment wrappers when re-wrapping. 
> 
> So if I have a sandbox and I touch some DOM node from there, inside the
> sandbox I get an Xray for it.  If I then return that Xray out of the sandbox
> (to code running in the chrome), what do I get in that code?  Do I get
> another Xray around the original object?  Or a CrossCompartmentWrapper
> around the first Xray?

You get another Xray. This was the source of our expando sharing problem in bug 758415.
(In reply to Boris Zbarsky (:bz) from comment #3)
> > What kind of wrappers are you curious about?
> 
> Jonathan, from a look at the code, it looks like some of the things here run
> in different sandboxes.  Is it possible that the two references the code is
> comparing came out of different sandboxes?

One of the references comes out of a sandbox, the other does not.
(In reply to Bobby Holley (:bholley) from comment #2)
> (In reply to Boris Zbarsky (:bz) from comment #1)
> > Bobby, does == unwrap various wrappers?
> 
> What kind of wrappers are you curious about? Anyway, generally not.
> 
> Sometimes this happens with Xray waiving, where one object is the waiver
> identity (XPCWrappedNative.unwrap(obj) or obj.wrappedJSObject) and the other
> isn't. What happens if you try putting the objects on both sides of the ==
> in XPCNativeWrapper()?
> 
> i.e., if a == b fails, try XPCNativeWrapper(a) == XPCNativeWrapper(b).

Wrapping both variables with XPCNativeWrapper() solves the problem; shall we just update our code to do that?
Is it possible that one is an Xray and the other is not?  If you compare XPCNativeWrapper.unwrap(obj1) to XPCNativeWrapper.unwrap(obj2), do _those_ test equal?
(In reply to Boris Zbarsky (:bz) from comment #9)
> Is it possible that one is an Xray and the other is not?  If you compare
> XPCNativeWrapper.unwrap(obj1) to XPCNativeWrapper.unwrap(obj2), do _those_
> test equal?

They do not.
(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> > i.e., if a == b fails, try XPCNativeWrapper(a) == XPCNativeWrapper(b).
> 
> Wrapping both variables with XPCNativeWrapper() solves the problem; shall we
> just update our code to do that?

(In reply to Jonathan Griffin (:jgriffin) from comment #10)
>  If you compare
> > XPCNativeWrapper.unwrap(obj1) to XPCNativeWrapper.unwrap(obj2), do _those_
> > test equal?
> 
> They do not.

These two statements should be contradictory.

By default, privileged code gets Xray vision when touching unprivileged objects. This doesn't mean that it's in anyway "wrapped in an Xray wrapper", just that the object lives in another (unprivileged) compartment and the cross-compartment wrappers we provide use Xray delegation.

However, sometimes chrome wants non-Xray vision. In this case, it does obj.wrappedJSObject or XPCNativeWrapper.unwrap(obj) (both are equivalent), which return a separate, parallel view to the object. This is called Transparent delegation or "having an Xray waiver". Xray waivers are transitive, so pulling a property off an Xray-waived object results in another Xray-waived object. the XPCNativeWrapper constructor can be used to "un-waive", stripping the waiver off the wrapper and returning to the default view.

Because the delegation is different, we provide a separate identity for wrappers with transparent delegation. This is where the identity confusion comes in. If |obj| is a wrapper with Xray delegation, |obj.wrappedJSObject != obj|, even though they refer to the same content object.

So this is why what you say doesn't make sense to me. If you have two objects obj1 and obj2 such that |XPCNativeWrapper(obj1) === XPCNativeWrapper(ob2)|, then it should very much be the case that |XPCNativeWrapper.unwrap(obj1) === XPCNativeWrapper.unwrap(obj2)|.

If you can put together a reduced test case that violates this invariant, I'd be very interested to see it.
(In reply to Bobby Holley (:bholley) from comment #11)
y delegation.
> 
> So this is why what you say doesn't make sense to me. If you have two
> objects obj1 and obj2 such that |XPCNativeWrapper(obj1) ===
> XPCNativeWrapper(ob2)|, then it should very much be the case that
> |XPCNativeWrapper.unwrap(obj1) === XPCNativeWrapper.unwrap(obj2)|.
> 
> If you can put together a reduced test case that violates this invariant,
> I'd be very interested to see it.

I'll try.  Now that I know the problem involves sandboxing, I have higher hopes of being able to reproduce this with a mochitest.
https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven’t been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.