927601 - Make XPCWrappedNative always keep its JS reflector object alive

Status: RESOLVED INACTIVE

(Core :: XPConnect, defect)

Comment 1

[Andrew McCreight [:mccr8]]

The reason I want to do this is to enable dynamic soundness checking for ICC, which works like this:
1. At the start of an ICC, run a regular CC that doesn't collect anything, but just records what is garbage.
2. Finish the ICC.
3. Make sure that everything the ICC thinks is garbage is something the regular CC thought was garbage.

The problem is that if in between steps 1 and 2 an XPCWN drops in refcount to 1, then we no longer hold the JS object alive, which means that something held alive by the JS object may be seen as garbage by the CC, when it wasn't garbage at the start of the CC. (Note that this is a false positive from the checker.)

The specific instance of this I've seen is the XPCWN of a window which holds onto a JS object that keeps alive the XPCWN for 2 DOM prototypes and an nsXPCComponents.

---

The main thing I am interested in doing is getting rid of the thing in the XPCWN traverse method where we only traverse the mFlatJSObject if the refcount is greater than one (which really should be a call to HasExternalReference()).  That shouldn't be necessary, because the cycle collector can break cycles itself.  That's easy enough to do.

We want to keep the garbage collector and cycle collector in synch with regards to what they think is alive or not, so we have to also modify WrappedNativeJSGCThingTracer and WrappedNativeSuspecter.  If we just drop the HasExternalReference() check from those, then we end up with weird shutdown behavior, where things are not cleaned up immediately.  On IRC, Peter pointed out that XPCWNs can wrap non-cycle-collected natives, which seemed to match with what I'm seeing.  I have tried to work around that by only checking HasExternalReferences when the underlying object isn't CCed, which we can check by trying to QI to one of the magic CC interfaces.

Anyways, I'm not sure if the resulting code is really simpler.  But what we run in the CCed XPCWN case is a little simpler...

---

A minor cleanup I'd like to do is inlining XPCJSRuntime::SuspectWrappedNative, because there's no reason for it to exist as a separate entity.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: WIP

https://tbpl.mozilla.org/?tree=Try&rev=c88df5b41447

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: WIP

With the patches in bug 931283 and bug 931285, the RSS feed unit tests don't have an extra shutdown cycle collection with this patch.

Comment 4

[Andrew McCreight [:mccr8]]

A cheesier solution to this false positive would be to unmark gray the JS object if we're in the middle of a GC when we unroot the JS object.

Comment 5

[Andrew McCreight [:mccr8]]

The only real point of this is to eliminate false positives from CC checking, which hasn't yet actually found any errors, so I think it can be delayed to the later polishing-for-enabling-by-default phase of things.

Comment 6

[Andrew McCreight [:mccr8]]

Bug 942528 made XPCWNs use the normal refcounting, but there's still some odd behavior where the XPCWN doesn't keep alive its wrapper if the refcount drops to 1. This makes XPCWN less leaky. If we can fix the leaks some other way, then we might be able to make XPCWN into a normal SCRIPT_HOLDER class and remove the weird XPConnect tracing machinery.

Comment 7

[Andrew McCreight [:mccr8]]

I have a patch that does the simple thing, but it hits odd JS assertions in some tests, like in here:
  ./mach mochitest browser/devtools/webconsole/test/
I think there's some issue with when unlink is happening, and how much it is cleaning up.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Make XPCWrappedNative always keep alive its reflector. WIP

Comment 9

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.