1005119 - Cycle collector does not properly handle minor garbage collections

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Terrence added some assertions for seeing if we GC during a CC in bug 1004276, and unfortunately it turns out we do.  js::IterateGrayObjects runs a minor GC, because apparently it isn't okay to iterate over cells when things can be in the nursery.  This can also happen with incremental CC, because we allow arbitrary other code to run, and I believe that minor GCs don't trigger the DOM GC callback, which is the mechanism ICC tries to use to ensure a GC doesn't run during it.

This is bad because the cycle collector has pointers to GC things in the mPointer field of PtrInfo which it will later dereference, and it doesn't tell the GC about them.  A minor GC can potentially move these objects, leaving the cycle collector with dangling pointers.  The cycle collector only reads from those pointers, and only does a very limited set of operations on them, in a way that doesn't leak to content, so I think it would be hard to turn this into arbitrary code execution, but it still sounds bad.

Another place the CC stores pointers to GC things is as keys in the mPtrToNodeMap hashtable, so for correctness those will have to be updated as well, but the CC never dereferences those pointers, so that should not be a security issue.

Comment 1

[Andrew McCreight [:mccr8]]

The simplest fix pre-ICC would be to do a minor GC before we add anything to the CC graph, assuming we can be assured there is zero JS allocation during a CC.  For ICC, we need to actually tell the GC about all of the CC pointers.

Comment 2

[Andrew McCreight [:mccr8]]

Thinking about this some more, this is probably not a problem.  The cycle collector only saves pointers to gray GC things[1], and I think only tenured objects can end up being gray (please correct me if I'm wrong).  Therefore, the CC can't contain pointers to objects that move.

[1] The exception to this is when it is in AllTraces() mode.  This only happens when the user manually triggers one from about:memory, and in any event this forces a full GC at the start of the CC, which should probably at least handle the non-ICC case.  The button in about:memory that triggers one does not start an ICC.  We could probably just forbid AllTraces() incremental CCs in non-debug builds, or something, as that's pointless.

Comment 3

[Terrence Cole [:terrence]]

Olli and I discussed this today on IRC. Our conclusion was that we need to do a minor GC to evict at the start of every CC slice (like we do for major GC slices) and defer major GCs that happen during unlinking. I think the best way to do this is to have 2 RAII guards in jsfriendapi. The first would encompass the entire CC slice and cause us to assert if we enter JS, allocate a GC thing, or trigger a GC (major or minor). It would also automatically do a minor GC when entered to clear the nursery so that we can safely walk the heap for CC. The second guard would go around just unlinking and would undo most of the assertions on the first guard and additionally cause major GC activity to be deferred. I'm thinking we could have it set a flag on the first guard that would cause leaving the first guard to do a GC. Additionally, on leaving the unlinking guard we would automatically do a minor GC, once again to ensure that the nursery is always clear when we are running under the CC.

There is the possibility that this will cause us to OOM; however, if we're OOMing frequently when unlinking, it's probably running enough code that it should be deferred to the mainloop anyway -- we can solve the OOM by doing exactly this.

Comment 4

[Andrew McCreight [:mccr8]]

After a certain point in the CC, we don't touch JS objects directly any more, such as during the CC unlinking, so I'd think that during CollectWhite and later we could run GCs?

See also bug 1158558 where we are doing major GCs during CC.  I think because we're running callbacks that can be implemented in JS.

Comment 5

[Andrew McCreight [:mccr8]]

This is hopefully okay...