514077 - Nuke XPCNativeWrapper.cpp:MirrorWrappedNativeParent

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Nobody remembers why this was done (other than the fact that it's semantically more correct to wrap the parent chain) and as far as I can tell, it is impossible to use an XPCNativeWrapped node as the scope chain (with doesn't count).

In <button onclick="foo"> the function compiled around "foo" is parented by the button node, but this is lexically limited to the untrusted node.

The other methods of setting event listeners only take functions whose scope chains are set at compile time, lexcially.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Other than purity of bodily fluids, the parent chain is what we walk to get the object principal.   I seem to recall this mattering at the time.  I also seem to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally toss an unwrapped node at the chrome and screw it over and other such worries.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #1)
> Other than purity of bodily fluids, the parent chain is what we walk to get the
> object principal.   I seem to recall this mattering at the time.

I don't think this matters anymore -- we've moved the security checks inside the wrappers and if we create a wrapper for an object, the enclosing scope had better be able to access the wrapper itself.

> I also seem
> to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally
> toss an unwrapped node at the chrome and screw it over and other such worries.

We don't have to worry about __parent__ because it won't resolve in the idl (unless someone actually has an idl property called __parent__) and we null out the prototype, so the magical obj_getSlot getter will never be called.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Reparent XPCNWs when their wrapped object is reparented

This is needed for making the outer window not a global object. Otherwise, we end up asserting when using the XPCNativeWrappers, whose scope was still the old inner window.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> (In reply to comment #1)
> > I also seem
> > to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally
> > toss an unwrapped node at the chrome and screw it over and other such worries.
> 
> We don't have to worry about __parent__ because it won't resolve in the idl
> (unless someone actually has an idl property called __parent__) and we null out
> the prototype, so the magical obj_getSlot getter will never be called.

__parent__ is toast. Waldo killed it.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to comment #2)
> (In reply to comment #1)
> > Other than purity of bodily fluids, the parent chain is what we walk to get the
> > object principal.   I seem to recall this mattering at the time.
> 
> I don't think this matters anymore -- we've moved the security checks inside
> the wrappers and if we create a wrapper for an object, the enclosing scope had
> better be able to access the wrapper itself.

Let me echo this back. I only partly understand.

1. The first patch here changes the principals of XPCNativeWrappers. With the patch, they'll be parented to the wrapped object's global, so they'll have the same principals as the wrapped object.  (...But what were they before? NULL?)

2. This does not change the subject principals of any code, because as comment 0 explains, XPCNWs are never on the scope chain.

3. This does change the object principals of some objects, but that's OK.

4a. This does not change the result of Components.utils.getGlobalForObject(xpcnw) when xpcnw wraps a WN whose global is an inner window. It still returns the outer window.

4b. What about when xpcnw wraps something whose global is a sandbox? We should return a wrapper of the sandbox, right? Do we? Will we continue to do so after this change?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #6)
> 1. The first patch here changes the principals of XPCNativeWrappers. With the
> patch, they'll be parented to the wrapped object's global, so they'll have the
> same principals as the wrapped object.  (...But what were they before? NULL?)

They haven't changed. When MirrorWrappedNativeParent when creating an XPCNativeWrapper around a global object, we parent that native wrapper to the global for that wrapped native scope. So the parent chain would look like XPCNW(obj1)->XPCNW(obj1.parent)->XPCNW(obj1.parent.parent)->obj1.parent.parent. This patch just gets rid of the intermediate XPCNativeWrappers.

> 3. This does change the object principals of some objects, but that's OK.

Actually, given the above, I don't even think there should be a change here.

> 4a. This does not change the result of
> Components.utils.getGlobalForObject(xpcnw) when xpcnw wraps a WN whose global
> is an inner window. It still returns the outer window.

Yes, but...

> 4b. What about when xpcnw wraps something whose global is a sandbox? We should
> return a wrapper of the sandbox, right? Do we? Will we continue to do so after
> this change?

This is an existing problem. We need to make sure that the return value of getGlobalForObject is wrapped in the right security/cross compartment wrapper.

Comment 8

[Brendan Eich [:brendan]]

Intuitive argument from the old days: NW lineage should mirror WN in case some crazy DOM event handler compatibility thing requires it.

Is 4b on file separately?

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> Intuitive argument from the old days: NW lineage should mirror WN in case some
> crazy DOM event handler compatibility thing requires it.

Yeah. I'm putting my foot down here and saying we don't have to worry about that. 4b is bug 586823.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 464668 [details] [diff] [review]
Fix

OK. Looks good.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/601d80023858
http://hg.mozilla.org/tracemonkey/rev/e5098ff8e0ce

Comment 12

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/601d80023858