1514672 - Figure out what to do with XPCWrappedNatives in same-compartment realms

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

We have some failures on Try like this:

engine is obtainable via getEngineByName - Got [xpconnect wrapped nsISearchEngine @ 0x7f081fb922e0 (native @ 0x7f0820db2440)], expected [xpconnect wrapped nsISearchEngine @ 0x7f081fb92b20 (native @ 0x7f0820db2440)]

It looks like we have the same underlying C++ object but we created different wrapped natives for it. I wonder if this is because we use the current global's XPCWrappedNativeScope in NativeInterface2JSObject:

https://searchfox.org/mozilla-central/rev/1a4a7745fd4a14402a0de0e9abc040724d4fcf8f/js/xpconnect/src/XPCConvert.cpp#935

Should XPCWrappedNativeScope be on CompartmentPrivate?

Comment 1

[Boris Zbarsky [:bzbarsky]]

> I wonder if this is because we use the current global's XPCWrappedNativeScope in NativeInterface2JSObject

Yes, presumably.

Hanging the XPCWrappedNativeScope off the compartment seems ok to me at first glance. And probably canonically creating XPCWrappedNatives in the "first global" of the compartment?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1)
> And probably canonically creating XPCWrappedNatives in the
> "first global" of the compartment?

Yeah it might make sense to use the first global for this. I think we could also use this global instead of nsXPCWrappedJS's mJSObjGlobal when the object there is a CCW, that would let us remove the mJSObjGlobal field and simplify things. I'll Try server.

Comment 3

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #1) 
> Hanging the XPCWrappedNativeScope off the compartment seems ok to me at
> first glance. And probably canonically creating XPCWrappedNatives in the
> "first global" of the compartment?

Which will usually be the shared JSM global. Which is probably what we really want, anyway.

Comment 4

[Jan de Mooij [:jandem]]

This all seems to work great on Try (so far). It fixes the issue in comment 0 and I think it's really nice to have just one copy of these objects (and Components etc) per compartment.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1514672 part 1 - Move XPCWrappedNativeScope from RealmPrivate to CompartmentPrivate. r?bzbarsky

This needs to be on the compartment to prevent creating duplicate wrapped natives.
We now also allocate these objects in the compartment's first global for
consistency and to prevent leaks.

XPCWrappedNativeScope also stores the content XBL scope. I considered moving
this to RealmPrivate, but given the fate of in-content XBL I went with the
simpler option of keeping it on XPCWrappedNativeScope and release-asserting we
have a single realm in the XBL case.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1514672 part 2 - Use the scripted caller's global instead of the context global in a few more places. r?bzbarsky

This fixes some test failures exposed by the previous patch.

Depends on D14849

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b16ce0e3114
part 1 - Move XPCWrappedNativeScope from RealmPrivate to CompartmentPrivate. r=bzbarsky

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/7b16ce0e3114

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8290d9948a7e
part 2 - Use the scripted caller's global instead of the context global in a few more places. r=bzbarsky

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/8290d9948a7e