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

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

6 months ago
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?

Updated

6 months ago
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?
Assignee

Comment 2

6 months ago
(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.
Assignee

Comment 4

6 months ago
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
Assignee

Comment 5

6 months ago
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.
Assignee

Comment 6

6 months ago
This fixes some test failures exposed by the previous patch.

Depends on D14849

Comment 7

6 months ago
Pushed by jdemooij@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7b16ce0e3114
part 1 - Move XPCWrappedNativeScope from RealmPrivate to CompartmentPrivate. r=bzbarsky
Assignee

Updated

6 months ago
Keywords: leave-open
Assignee

Updated

6 months ago
Keywords: leave-open

Comment 9

6 months ago
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

Comment 10

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8290d9948a7e
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.