926533 - Make cycle collection rooting/unlinking/unrooting incremental

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

This is mostly straightforward.  The major problem here is the interaction with any kind of weak reference that becomes a strong reference during unlinking.

From my comment in bug 850065:
I've been investigating a timeout in content/canvas/test/crossorigin/test_video_crossorigin.html for a while, and I figured out what the problem is.  The sequence of steps is something like:
1. Create an XPCWrappedJS (WJS) x for a JS object y (for use as an event listener).
2. The WJS becomes part of a garbage cycle, but the JS object is still alive.
3. Start an ICC, do MarkRoots and ScanRoots. We decide the WJS is dead.
4. Try to create a WJS for y. We see that there's already one around, x, in the WrappedJSMap table, so use that. (again, as an event listener)
5. Finish the ICC. We still think x is dead, so we unlink it, zeroing out the pointer to y.
6. The event fires on x, but x doesn't point to anything, so nothing happens.
7. The event handler was supposed to finish the test, so we time out.

The general problem is that if any kind of weak reference becomes strong after ScanRoots, we can end up unlinking a live object, because we don't change our mind about something being dead at that point.  This isn't a problem if it happens before ScanRoots, because we won't free anything that was AddRefed or Released during the current CC.

I'm not sure what the best way to solve this is.  I maybe could add a mechanism for letting the weakly-held object know that it is going to go away in ScanRoots.

Comment 1

[Andrew McCreight [:mccr8]]

Another idea I had was to tie the lifetime of the XPCWJS to its underlying JS object, essentially doing wrapper preservation for XPCWJS, but keeping them around might cause perf problems, and it would also require messing around more with XPCWJS reference counting, which is pretty weird already.

Comment 2

[Andrew McCreight [:mccr8]]

If CollectWhite is incremental, then we could change nsCycleCollector::PrepareForGarbageCollection() to only advance past ScanRoots().  At that point in theory we shouldn't need to touch JS objects any more, so it is okay if the GC frees them.  We may want to null out the mPointer for JS nodes in that case, to prevent UAFs in case there are bugs.  We can't null out the mParticipant, which indicates the object isn't in the graph.

Comment 3

[Andrew McCreight [:mccr8]]

More broadly, I am concerned about how any code anywhere can easily create a new weak reference to some C++ object, which can then be resurrected in the middle of incremental unlinking after something that is reachable from it is unlinked.

We could analyze what exactly is taking a lot of time in unlinking, and create some kind of opt-in for incremental unlinking.  In effect, this is already being done in an ad hoc way for things like nodes which create their own little runnables to do teardown after the CC has finished running.  It would be nice to do that in a general way, that avoids making everybody who wants to do that write their own little weird runnable class.

Of course, in general this is bad, because something that is reachable from a weak referencable object could still get unlinked.  The "right" way to do this would probably be to assume that all objects that haven't opted in could get resurrected, and then not incrementally unlink anything reachable from them.  Though if this was really a problem in general, I'd think we'd already see problems for nodes, so maybe that's overkill.

Comment 4

[Andrew McCreight [:mccr8]]

I think the weak reference problem means we can't really do this.  I think the best way to reduce unlinking times is a more ad-hoc approach, by adding more things like the Lazy Content Blaster to particular classes that take a long time to unlink.