Closed Bug 723971 Opened 8 years ago Closed 8 years ago

black-gray edges due to shapes in UnmarkGrayChildren


(Core :: XPConnect, defect)

Not set



Tracking Status
firefox13 --- fixed
firefox14 --- fixed
firefox-esr10 --- wontfix


(Reporter: mccr8, Assigned: mccr8)



(Whiteboard: [sg:moderate][qa-] [advisory-tracking+])


(1 file, 2 obsolete files)

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.
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...
Andrew says he can work on this.
Assignee: nobody → continuation
Attached patch WIP (obsolete) — Splinter Review
This code is a little tricky.

I seem to consistently get shutdown leaks.  I should figure out what that's about.
I think it could be cleaner and easier to understand if each JS_TraceChildren call uses its own fresh UnmarkGrayTracer.
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
Okay, I get shutdown leaks even without my patches.  That's a little disturbing.
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.
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.
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #602210 - Attachment is obsolete: true
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.
(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.
(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.
Attachment #602524 - Attachment description: only have on subtracer to try to reduce stack pressure → only have one subtracer to try to reduce stack pressure
Comment on attachment 602524 [details] [diff] [review]
only have one subtracer to try to reduce stack pressure

Try run looked good.
Attachment #602524 - Flags: review?(wmccloskey)
Comment on attachment 602524 [details] [diff] [review]
only have one subtracer to try to reduce stack pressure

Looks great. Thanks.
Attachment #602524 - Flags: review?(wmccloskey) → review+

I had to make a few minor fixes due to the elimination of contexts from JSTracers.
Target Milestone: --- → mozilla13
Closed: 8 years ago
Resolution: --- → FIXED
Blocks: 651445
Are there any specific suggestions for means to verify this fix?
not really.
wontfixing this for ESR unless we find a specific way to trigger this bug.
Whiteboard: [sg:moderate] → [sg:moderate][qa-]
Whiteboard: [sg:moderate][qa-] → [sg:moderate][qa-] [advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.