947049 - Potential leaks with XPCWrappedJS that use aggregated natives and have weak references to them

Status: RESOLVED WORKSFORME

(Core :: XPConnect, defect, P3)

Description

[Andrew McCreight [:mccr8]]

An XPCWrappedJS (henceforth WJS) has a magical extra ref count.  The CC basically ignores this by having a random extra self-edge.  When the WJS drops to a refcount of 1 and doesn't have a weak reference, it is deleted.  But if it _does_ have a weak reference to it, then it sticks around.  I think the idea is that the lifetime of the WJS is now tied to the lifetime of its JS object.

Because the WJS has a refcount of 1, nothing actually has an owning pointer to it.  It is also no longer in the list of wrapped JS roots, so it won't be traced by the CC, so the CC can't kill it.  In fact, I believe the only thing that can delete it is if the JS object dies and then XPCJSRuntime will find and kill the WJS..  (Note that this is true, as far as I can tell, even if later the WJS no longer has a weak reference to it!)

The WJS no longer owns the JS object, so there's no direct cycle there.  But, it can point to an aggregated native, which in general can be an arbitrary nsISupports object, which could in theory then keep the WJS's JS object alive, causing a cycle which can never be collected.

Beyond shutdown leaks, which I guess we haven't noticed before, it could also cause chains that take a long time to break down.  If you have a WJS's object that needs a CC to clean up, and the WJS keeps alive an aggregated native that owns stuff that needs a CC to clean up, you have to CC, GC, CC to clear up everything.  You can create a chain like this that would require an arbitrary number of GCs and CCs to clean up.

In practice, it seems like aggregated natives are only used in limitied circumstances, and I'm sure not many WJS have weak references to them, so hopefully this doesn't actually happen in practice.

To fix this, one option would be to make the CC see these edges. You can't just add an edge from the JS object to the WJS, because the JS object doesn't know it owns the WJS. Fortunately, this is basically the same set up as weak maps, so I could probably hack up something using the weak map mechanism to represent the edge. The annoyance here is that it is set up only to deal with JS to JS edges.

A second option, suggested by Olli, would be to just disallow weak references to WJS with aggregated natives, and hope nobody uses them.

Comment 1

[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.

Comment 2

[Andrew McCreight [:mccr8]]

I think my patch in bug 1543537 will fix this issue by making the CC understand weak references to WJS.

Comment 3

[Andrew McCreight [:mccr8]]

I think this is fixed, now that we properly cycle collected WJS that are subject to finalization.