75141 - JS scope deadlock avoidance doesn't handle GC dependencies

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

If a thread is blocked waiting for the GC to finish, and the GC is in a
finalizer waiting for a scope lock, it's possible for that scope to be owned
exclusively by the first thread's context (I don't understand how the deadlock
comes about yet -- it may require at least three threads).

The deadlock detection in jslock.c's ClaimScope handles the PR_WaitCondVar on
rt->scopeSharingDone, but it doesn't do diddly for the JS_AWAIT_GC_DONE that
follows, when resuming the request.

/be

Comment 1

[Brendan Eich [:brendan]]

Gotta fix fast.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Comment 3

[Brendan Eich [:brendan]]

jband: you are my best buddy from bug 54743.  How about another sr=?  Shaver,
can you r=?  Thanks.

/be

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I had to read the comment and related code about 5 times, but I think I get it
now.  (Finalizers having to share scopes on the dead objects, who'da thunk it?)

r=shaver

Comment 5

[John Bandhauer]

sr=jband

Comment 6

[Brendan Eich [:brendan]]

Attachment: better fix, but conservative/pessimal: GC thread context can't claim a scope, rather forces sharing

Comment 7

[Brendan Eich [:brendan]]

I also fixed the bogus assertion that rt->requestCount > 0 if the ClaimScope cx
is in a request.  The cx running the GC and calling finalizers will be in a
request according to its cx->requestDepth, but the GC has already temporarily
reduced rt->requestCount to 0.

Not only should ClaimScope not assert so bogusly, it shouldn't even fiddle with
rt->requestCount or do notifies based on its 0/non-0 transition at all, if cx is
running the GC (or on the same thread as a cx that's running the GC).

/be

Comment 8

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 9

[Brendan Eich [:brendan]]

Phil, I think we should do an RC3a tarball, or whatever -- this bug is biting
RC3 downloaders, all around the same time.

/be

Comment 10

[Brendan Eich [:brendan]]

WillDeadlock nicely checks whether the GC is active on cx's thread, and returns
true -- but ClaimScope calls WillDeadlock iff ownercx->scopeToShare.  D'oh!  The
"GC thread is cx's thread" test must happen in ClaimScope, before WillDeadlock
and as part of the same condition that leads to ShareScope.

/be

Comment 11

[Brendan Eich [:brendan]]

Patch coming right up.

/be

Comment 12

[Brendan Eich [:brendan]]

Attachment: best fix

Comment 13

[Brendan Eich [:brendan]]

Slightly improved code will be checked in: use rt->gcThread rather than
cx->runtime->gcThread in ClaimScope (where rt is available in a local).

I don't know whether anyone can claim to review this code and find bugs before
hand -- I have failed.  But a hindsight-review from jband or shaver would make
me feel better.  Also, we need to tickle this with jband's MT stress test, which
seems not to do enough finalization of objects claimed but slated for sharing by
other threads.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixed for good.

/be

Comment 15

[Brendan Eich [:brendan]]

Urgh.

/be

Comment 16

[Phil Schwartau]

Marking Verified -