1478359 - Store a global object in nsXPCWrappedJS

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

This is the same issue as bug 1477923 but for nsXPCWrappedJS.

The JS object stored in nsXPCWrappedJS can be a CCW but if we can no longer get a CCW's global, we need some other way to get a global to pass to JSAutoRealm. The simplest option is probably to store the right JS global in nsXPCWrappedJS.

Comment 1

[Andrew McCreight [:mccr8]]

I was vaguely concerned about the memory overhead, but according to my about:memory each process is only using like 1.5kb at most for wrappedjs, so another pointer shouldn't matter.

Comment 2

[Bobby Holley (:bholley)]

Do we actually have a good use-case for an XPCWrappedJS around a CCW? Could we assert that the CCW is always system->system and then unwrap it?

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Do we actually have a good use-case for an XPCWrappedJS around a CCW? Could
> we assert that the CCW is always system->system and then unwrap it?

I'm not sure. I can look into this when I get back to it, but this is also the kind of bug I could use help with :)

Do we still use XPCWrappedJS in content?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > Do we actually have a good use-case for an XPCWrappedJS around a CCW? Could
> > we assert that the CCW is always system->system and then unwrap it?
> 
> I'm not sure. I can look into this when I get back to it, but this is also
> the kind of bug I could use help with :)
> 
> Do we still use XPCWrappedJS in content?

In theory no. Verifying with a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62d98e4d3fc7f3607c3834212fe84954e812b805

Comment 5

[Boris Zbarsky [:bzbarsky]]

There might be issues around XBL implements="" in non-system bits (like tests) because that uses nsXPCWrappedJS.

There might also be issues around consumers of CallbackObject::ToXPCOMCallback.  We killed that stuff off from event listeners, at least.  But there are a few consumers left around, looks like: idle observers (filed bug 1478721 to remove that), frame message manager (maybe chromeonly?), getUserMedia stuff, and browser-element next-paint listeners (pretty sure we can kill this one too).

Comment 6

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #5)
> There might be issues around XBL implements="" in non-system bits (like
> tests) because that uses nsXPCWrappedJS.

Yeah, the try push hit that. In theory we shouldn't have CCWs there, so we could make the condition "XPCWJS unwraps CCWs i.f.f. they're system->system, and then asserts against them". Trying that with a new push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ab3d301428520e5f421be6a5b3409ba99e066ed
 
> There might also be issues around consumers of
> CallbackObject::ToXPCOMCallback.  We killed that stuff off from event
> listeners, at least.  But there are a few consumers left around, looks like:
> idle observers (filed bug 1478721 to remove that), frame message manager
> (maybe chromeonly?), getUserMedia stuff, and browser-element next-paint
> listeners (pretty sure we can kill this one too).

We'll see what the try push comes up with.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I've killed off the uses of ToXPCOMCallback except the one in nsFrameMessageManager.cpp.  For that one, I'm quite sure we always have system stuff.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> OK, I've killed off the uses of ToXPCOMCallback except the one in
> nsFrameMessageManager.cpp.  For that one, I'm quite sure we always have
> system stuff.

Did this land in a bug somewhere? If so, can you point me to it? I can fast-forward my try patch over it and repush to see if that fixed those couple of oranges.

Comment 9

[Bobby Holley (:bholley)]

Great. Thanks Tom!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e89fd94cc8b74843aee5d7f6911cced6399d4e1d

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Did this land in a bug somewhere?

Tom got the right bug numbers there.

Comment 11

[Bobby Holley (:bholley)]

So one of the failures is in nsXPCComponents_Utils::GenerateXPCWrappedJS , which is a test function we could presumably fix.

The rest all look like this:

#01: XPCConvert::JSObject2NativeInterface(void**, JS::Handle<JSObject*>, nsID const*, nsISupports*, nsresult*) [js/xpconnect/src/XPCConvert.cpp:1056]
#02: nsXPConnect::WrapJSAggregatedToNative(nsISupports*, JSContext*, JSObject*, nsID const&, void**) [js/xpconnect/src/nsXPConnect.cpp:715]
#03: nsBindingManager::GetBindingImplementation(nsIContent*, nsID const&, void**) [dom/xbl/nsBindingManager.cpp:650]
#04: nsXULElement::QueryInterface(nsID const&, void**) [dom/xul/nsXULElement.cpp:306]

Most come from nsXULElement::IsFocusableInternal, though one comes from nsMenuFrame::GetParentMenuListType.

Comment 12

[Bobby Holley (:bholley)]

Ah, and that, of course, takes us here: https://searchfox.org/mozilla-central/rev/196560b95f191b48ff7cba7c2ba9237bba6b5b6a/dom/xbl/nsBindingManager.cpp#633

Which is a comment I wrote years ago explaining why we need to use the Xray view to make XBL native/JS aggregation work properly on content scopes.

So we probably do need to store the extra global for now, but we should add a comment that we can drop it and assert against CCWs once in-content XBL is eliminated (which should happen in the coming months). Probably worth filing a dependent bug on bug 1443836 reminding us to drop it.

Comment 13

[Jan de Mooij [:jandem]]

Thanks for investigating that, Bobby.

The remaining JSAutoRealmAllowCCW instances in XPConnect not addressed by in-flight patches are either in XrayWrapper.cpp or related to XPCWrappedJS. I'll try to take a stab at this.

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Store a global object in nsXPCWrappedJS and use it for realm-entering

Would you mind giving this a quick sanity check? This is green on Try but I still want to verify a few things before requesting review.

We pass a JSContext to nsXPCWrappedJS::GetNewOrUsed, then release-assert cx and the object are same-compartment. Then we store a global object in nsXPCWrappedJS and use that instead of the object for JSAutoRealm(AllowCCW).

Who's a good reviewer for the final version, maybe mccr8? (Though I know he's on PTO next week.)

Comment 15

[Bobby Holley (:bholley)]

I'm out for the next two weeks, and slammed for the end of this week finishing things up. Andrew, do you have the cycles to help Jan land this before you head off?

Comment 16

[Andrew McCreight [:mccr8]]

I can look at it (I've looked at a bunch of the patch here), but I don't know if I really understand any of what is going on, beyond the mechanical changes like passing around a context.

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8996788 [details] [diff] [review]
Store a global object in nsXPCWrappedJS and use it for realm-entering

Thanks Andrew. If you feel unsure doing the review, feel free to delegate to mrbkap or peterv. I'm out of time, so clearing the flag for myself.

Comment 18

[Jan de Mooij [:jandem]]

Bug 1477923 should probably land first so we can use it here.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Store a global object in nsXPCWrappedJS and use it for realm-entering

Beefed up the commit message a bit.

Comment 20

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) (PTO through August 20th) from comment #2)
> Do we actually have a good use-case for an XPCWrappedJS around a CCW? Could
> we assert that the CCW is always system->system and then unwrap it?

Coming across this while doing Triage, so I'm not fully up-to-date on what's happening here - we might wrap XRays in XPCWrappedJS sometimes, although I'm not completely certain.

Comment 21

[Bobby Holley (:bholley)]

(In reply to :Nika Layzell from comment #20)
> (In reply to Bobby Holley (:bholley) (PTO through August 20th) from comment
> #2)
> > Do we actually have a good use-case for an XPCWrappedJS around a CCW? Could
> > we assert that the CCW is always system->system and then unwrap it?
> 
> Coming across this while doing Triage, so I'm not fully up-to-date on what's
> happening here - we might wrap XRays in XPCWrappedJS sometimes, although I'm
> not completely certain.

Yeah, that was where we finally ended up in comment 12. ;-)

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8996953 [details] [diff] [review]
Store a global object in nsXPCWrappedJS and use it for realm-entering

Review of attachment 8996953 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/CustomElementRegistry.cpp
@@ +1212,5 @@
>        nsCOMPtr<nsIJSID> iid = nsJSID::NewID(aIID);
>        func->Call(aElement, iid, &customInterface);
>        if (customInterface) {
>          RefPtr<nsXPCWrappedJS> wrappedJS;
> +        // XXX: use func's callbackGlobal instead, but needs bug 1477923 to land

There's an r+ed patch up in that bug. Can this be improved before landing?

::: js/xpconnect/src/xpcprivate.h
@@ +1705,4 @@
>                                              JSObject* jsobj, REFNSIID aIID);
>  
>      static nsresult BuildPropertyEnumerator(XPCCallContext& ccx,
> +                                            JS::HandleObject scope,

In bz's patches, and in this patch for XPCWrappedJS's ctor, the scope appears after the object, while in these three functions the scope is before the object. It kind of makes more sense to me for the scope to be first, but I think consistency is also useful. So maybe they should be in the other order?

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Use nsXPCWrappedJS's object global as nonCCWObject in nsFrameMessageManager::ReceiveMessage

Just a trivial follow-up to bug 1477923.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8997656 [details] [diff] [review]
Use nsXPCWrappedJS's object global as nonCCWObject in nsFrameMessageManager::ReceiveMessage

Review of attachment 8997656 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameMessageManager.cpp
@@ +740,5 @@
>            continue;
>          }
>  
>          object = wrappedJS->GetJSObject();
> +        nonCCWObject = wrappedJS->GetJSObjectGlobal();

I'd be happy to rename nonCCWObject to objectGlobal if you prefer, as it will always be a global now.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8997656 [details] [diff] [review]
Use nsXPCWrappedJS's object global as nonCCWObject in nsFrameMessageManager::ReceiveMessage

> I'd be happy to rename nonCCWObject to objectGlobal if you prefer

Yes, please!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c093b141e2
Store a global object in nsXPCWrappedJS and use it for realm-entering. r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e079d9d6641
Use nsXPCWrappedJS's object global as nonCCWObject in nsFrameMessageManager::ReceiveMessage. r=bz

Comment 27

[Jan de Mooij [:jandem]]

(Pushed with all comments addressed.)

Comment 28

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b3c093b141e2
https://hg.mozilla.org/mozilla-central/rev/0e079d9d6641