Summary: Simplify XPCWrappedNative traversal → Simplify XPCWrappedNative cycle collector traversal
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.
Created attachment 822807 [details] [diff] [review] 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.
Attachment #819239 - Attachment is obsolete: true
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.
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.
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.
Summary: Simplify XPCWrappedNative cycle collector traversal → Make XPCWrappedNative always keep alive its wrapper
Summary: Make XPCWrappedNative always keep alive its wrapper → Make XPCWrappedNative always keep its JS reflector object alive
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.
Created attachment 8635525 [details] [diff] [review] Make XPCWrappedNative always keep alive its reflector. WIP
Attachment #822807 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.