723971 - black-gray edges due to shapes in UnmarkGrayChildren

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This is another instance of the "use after unlink" problem described in bug 690970, where a black-gray edge can cause the cycle collector to think something is dead when it is alive, and call Unlink on it.  This probably only can causes null derefs, but Unlink functions are written by hand in C++, so who knows.

A shape can hold onto a non-shape, but UnmarkGrayChildren stops if it reaches a shape.  Thus there can be an edge from a black object to a gray shape.

We can't just remove this check, because it may cause the stack limit to be hit more often, which will trigger GCs more often.  Instead, we need to use JS_TraceShapeCycleCollectorChildren.

Comment 1

[Andrew McCreight [:mccr8]]

This is a little weird because JS_TraceShapeCycleCollectorChildren won't actually unmarkgray the shape or base shapes it encounters, because it doesn't visit them.

Of course, the CC never actually sees these objects, and because they are left gray not black it won't skip through them.  So it seems technically okay, because the CC graph will never contain black-gray edges.

It still seems bad, because any sort of black-gray detection that looks at the GC's view of the heap and not the CC's will be confused.  Maybe I'll make a variation of it or something.  Either that or write another JS tracer that traces through and marks shapes and base shapes, but nothing else...

Comment 2

[Johnny Stenback (:jst)]

Andrew says he can work on this.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: WIP

This code is a little tricky.

I seem to consistently get shutdown leaks.  I should figure out what that's about.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think it could be cleaner and easier to understand if each JS_TraceChildren call uses its own fresh UnmarkGrayTracer.

Comment 5

[Andrew McCreight [:mccr8]]

Yeah, that's a good point.

The leaks appear to be occurring because mScopeObjects are black, and rooting everything.  I'm not really sure what they are:
  0x1185c5ba0 B mScopeObject

I also see black objects intermixed with gray objects in the roots, which is a little disturbing.  I'm not sure if that's an artifact of the heap dumping process or what.  I should see how things are without my patch...

0x11805adc0 B nsXPCWrappedJS[xpcIJSGetFactory,0x1166ecc80:0x117a96ba0].mJSObj
0x118053dc0 B nsXPCWrappedJS[xpcIJSGetFactory,0x1166ec300:0x117a965c0].mJSObj
0x1187e4f18 G mScriptObject.mObject
0x1185c5ba0 B mScopeObject
[... mScopeObject a bunch more times]
0x11a1ce380 G mJSGetterObject
0x11a1ce3c0 G mJSGetterObject
0x11a1ce400 G mJSSetterObject

I also seem to get some longish ForgetSkippable times, but I don't know if that's due to doing a lot more unmarkgray or because I'm using a debug build:

CC(T+356.9) duration: 45ms, suspected: 2721, visited: 12993 RCed and 9683 GCed, collected: 8775 RCed and 458 GCed (9233 waiting for GC)
ForgetSkippable 21 times before CC, min: 1 ms, max: 85 ms, avg: 9 ms, total: 202 ms, removed: 18377

Comment 6

[Andrew McCreight [:mccr8]]

Okay, I get shutdown leaks even without my patches.  That's a little disturbing.

Comment 7

[Andrew McCreight [:mccr8]]

The forget skippable times from my light testing don't look much worse than what I'm seeing in the bad cases on an unpatched build I've been running for a few days, so I guess it is probably okay.

Comment 8

[Andrew McCreight [:mccr8]]

It turns out that it is just this particular profile of mine that has shutdown leaks.  That's a little disturbing.  I guess I'll file a separate bug for that.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: WIP

It looks much nicer with the extra tracer rather than manually saving and restoring the state.  I do worry a bit about the extra 7 words per stack frame for the tracer, but perhaps that is a premature concern. If it ends up being a problem (we can track via telemetry), we could have some kind of TracerState that is just the two words, and swap that out instead of saving and restoring the individual parts.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I think you can simplify the last loop even more, like this:

    UnmarkGrayTracer childTracer(tracer, true);
    do {
        JS_TraceChildren(&childTracer, thing, JSTRACE_SHAPE);
        thing = childTracer.mPreviousShape;
    } while (thing);

The child tracer will take care of doing UnmarkGray, and it won't fill in mPreviousShape if it was already gray.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #10)
> I think you can simplify the last loop even more, like this:

Oh, good point! I think I still need to reset mPreviousShape to null.

I filed bug 732495 for the shutdown leaks.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> Oh, good point! I think I still need to reset mPreviousShape to null.

Oh, yeah, you're right. But you could move the declaration of childTracer into the loop to take care of that.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: only have one subtracer to try to reduce stack pressure

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 602524 [details] [diff] [review]
only have one subtracer to try to reduce stack pressure

Try run looked good.

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

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 602524 [details] [diff] [review]
only have one subtracer to try to reduce stack pressure

Looks great. Thanks.

Comment 16

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/003ffd062130

I had to make a few minor fixes due to the elimination of contexts from JSTracers.

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/003ffd062130

Comment 18

[Al Billings [:abillings - ex-MoCo]]

Are there any specific suggestions for means to verify this fix?

Comment 19

[Andrew McCreight [:mccr8]]

not really.

Comment 20

[Daniel Veditz [:dveditz]]

wontfixing this for ESR unless we find a specific way to trigger this bug.