823225 - Remove .wrappedJSObject from the nsWindowSH::GetProperty

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Why do we need this in here anyway?  Does this need to work as a bareword, or is it enough if it works as a qualified get?

Comment 1

[Jeff Walden [:Waldo]]

Assuming it should work, it should work the same both ways, as there's no spec way to differentiate bareword from qualified lookups.  (Some spec algorithms implementing the language distinguish, but they shouldn't pass that context along to the lookup process, or to the object where the lookup is happening.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sure there is.  Qualified lookups happen on the WindowProxy, because the LHS is an actual object you have, so a Window proxy, while unqualified ones happen on the Window, because that's the ES global.  Since they're on different objects you can tell them apart quite easily, where "you" means both us as implementors and web authors.  Just have a look at the testcase in http://lists.w3.org/Archives/Public/public-script-coord/2012OctDec/0328.html for an example.

So if we're ok with just doing this qualified, we can implement entirely in the WindowProxy. If not, we have to do more thinking.

Comment 3

[Jeff Walden [:Waldo]]

Oh.  Well, as long you're not relying on resolve flag gunk, I suppose I don't care.

But I would expect people to be fairly confused if |wrappedJSObject != window.wrappedJSObject| (in behavior, at least -- I dunno if you get a different wrapper each time or something, but they should work the same), for the simple reason that I am pretty much every time I see one of those location-switching testcases that shows something similar, and that's with location-switching, which is already way off the deep end.  :-)

Comment 4

[Bobby Holley (:bholley)]

Wait, why do we even need any wrappedJSObject junk in nsWindowSH? I thought this was all taken care of at the Xray (and XPCWrappedJS, for that usage) layer. Are you sure that code isn't vestigial?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Wait, why do we even need any wrappedJSObject junk in nsWindowSH?

That's my real question, yes.

> Are you sure that code isn't vestigial?

Nope.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So looking at the blame, bug 580128 added this.  The checkin comment was "Give windows a .wrappedJSObject property to help out old consumers of XPCNativeWrappers.".

Blake, do you know whether we still need this?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

One of the big differences between pre-brain transplant XPCNativeWrappers and post-brain transplant XrayWrappers was that XPCNativeWrappers would happily wrap chrome objects in content docshells for chrome (system) code whereas XrayWrappers were entirely dependent on the principal. My recollection is that I started to go through all of the tests to figure out which ones actually needed to use .wrappedJSObject, but ran out of time in the middle. We were also scared that we'd break extensions using .wrappedJSObject blindly, so we added the hack.

I assume that if we wanted to remove this we'd at least have to fix a bunch of chrome/browser-chrome mochitests.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  For that use case it sounds like leaving .wrappedJSObject on the outer window only is enough, because no one is likely to use bareword wrappedJSObject.  Great, thanks!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Move .wrappedJSObject from nsWindowSH::GetProperty to the outer window get hook.

Comment 10

[Boris Zbarsky [:bzbarsky]]

And yes, I know the new setup is slightly daft because it doesn't return a descriptor for the thing.  Neither did the old one.  If we care, I can change the behavior, but I dunno that we care.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 695124 [details] [diff] [review]
Move .wrappedJSObject from nsWindowSH::GetProperty to the outer window get hook.

>-  if (id == sWrappedJSObject_id &&
>-      xpc::AccessCheck::isChrome(js::GetContextCompartment(cx))) {
>-    obj = JS_ObjectToOuterObject(cx, obj);
>-    *vp = OBJECT_TO_JSVAL(obj);
>-    return NS_SUCCESS_I_DID_SOMETHING;
>-  }
Is sWrappedJSObject_id used anywhere else?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is sWrappedJSObject_id used anywhere else?

Yes, in the '+' lines in this patch.

Comment 13

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5fae52f44675

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5fae52f44675