Closed Bug 1514672 Opened 6 years ago Closed 6 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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: