Closed Bug 1514672 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XPConnect, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files)

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?
Priority: -- → P3
> 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?
(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.
(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.
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
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.
This fixes some test failures exposed by the previous patch.

Depends on D14849
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b16ce0e3114
part 1 - Move XPCWrappedNativeScope from RealmPrivate to CompartmentPrivate. r=bzbarsky
Keywords: leave-open
Keywords: leave-open
Pushed by jdemooij@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8290d9948a7e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.