916753 - Deadlock on the operation callback lock?

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I got up this morning to Firefox being frozen.  When I sampled it, all mainthread samples were under __psynch_mutexwait when acquiring the lock in JSRuntime::triggerOperationCallback.

The attachment has the stacks for all threads.

I assume we somehow deadlocked on this lock...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Stacks

Comment 2

[Jan de Mooij [:jandem]]

This could be the same issue as bug 916531... Unfortunately I don't see a useful stack there so it's hard to say for sure.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And indeed.  The mainthread stack has CodeGenerator::link (which locks the operation callback lock) calling newCode (done while the lock is held), which ends up allocating an arena, which ends up calling TriggerZoneGC (inlined?) which does TriggerOperationCallback (also inlined?).

The AutoLockForOperationCallback in link() was added in bug 864220, looks like.

Comment 4

[Jan de Mooij [:jandem]]

Thanks Boris, requesting needinfo from bhackett to make sure we don't forget about this.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Bug 916531 does have stacks.  See the breakpad incidents, and show all threads.  They look unrelated to this issue.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: patch

We need to prevent the main thread from trying to reenter the operation callback lock, and in this case can't really do so when allocating the IonCode as that allocation happens in the same call that allocates the code memory from the Ion executable allocator (accesses to which need to be protected by the operation callback lock).  The cleanest solution seems to be to make rt->currentThreadOwnsOperationCallbackLock() available to release builds, and just avoid triggering GCs when allocating in such cases.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 805311 [details] [diff] [review]
patch

Is PR_GetCurrentThread cheap enough or AutoLockForOperationCallback rare enough?

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 805311 [details] [diff] [review]
> patch
> 
> Is PR_GetCurrentThread cheap enough or AutoLockForOperationCallback rare
> enough?

Both.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 805311 [details] [diff] [review]
patch

Review of attachment 805311 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Runtime.h
@@ -737,1 @@
>              rt->operationCallbackOwner = PR_GetCurrentThread();

Can we JS_ASSERT(!rt->currentThreadOwnsOperationCallbackLock()) here, to turn this deadlock into an assertion failure next time?

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bcf9b261b94

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4bcf9b261b94

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/290e1e44e8b3

Comment 13

[Avi Halachmi (:avih)]

According to bug 918862, this bug supposedly caused some regressions:
- Inbound-non-pgo: TART (tab animations) on Ubuntu (3.4%), Ubuntu 64 (5.8%), TP5-optimized win8 (2.7%), Paint win8 (4.2%).
- Aurora (pgo?): TART Ubuntu 64 (9.4%), Ubuntu (5%), win XP (2.7%).

- Are these expected from this bug or can be explained in retrospective?
- Is this a legitimate price to pay for this bug? If not, can the cost be reduced?

Comment 14

[Boris Zbarsky [:bzbarsky]]

> - Are these expected from this bug or can be explained in retrospective?

You're asking me?  Shouldn't you be asking Brian?