Closed Bug 1486556 Opened Last year Closed Last year

Helper threads release the global lock at the wrong time when coordinating with web replay checkpoints

Categories

(Core :: Web Replay, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: bhackett, Assigned: bhackett)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
When saving or restoring checkpoints the main thread needs other threads to enter the right idle state, which requires special handling for JS helper threads.  To this end, each helper thread checks to see if it needs to idle before any point where it blocks indefinitely while waiting for an incoming task.  In doing so, however, the thread releases the helper thread state lock, so we have a sequence of:

1) check for new task
2) release lock
3) acquire lock
4) block until a new task arrives

If a new task arrives between 2) and 3), it is incorrect to block in 4), and if all helper threads block incorrectly, and the main thread blocks while waiting for a helper thread task to complete, then we deadlock.

The attached patch fixes this by checking for the need to idle before checking for a new task, so the lock is held by the thread continuously from 1) to 4).  This also simplifies the web replay unrecorded wait API a little, as JS helper threads are the only remaining user of this API and don't need one of its behaviors.
Attachment #9004302 - Flags: review?(nfitzgerald)
Assignee: nobody → bhackett1024
Comment on attachment 9004302 [details] [diff] [review]
patch

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

I'm not super comfortable reviewing this -- forwarding to jandem.
Attachment #9004302 - Flags: review?(nfitzgerald) → review?(jdemooij)
Comment on attachment 9004302 [details] [diff] [review]
patch

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

::: js/src/vm/HelperThreads.cpp
@@ +2416,5 @@
>      }
>      cx.setHelperThread(this);
>      JS_SetNativeStackQuota(&cx, HELPER_STACK_QUOTA);
>  
> +    if (mozilla::recordreplay::IsRecordingOrReplaying()) {

Nit: no {}
Attachment #9004302 - Flags: review?(jdemooij) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc437d64c831
Avoid unlocking the helper thread state lock after looking for tasks to perform, r=jandem.
I just backed this out because I didn't adequately test or think this patch through when writing it.  Helper thread coordination *does* need the old semantics for NotifyUnrecordedWait, as otherwise it is possible for the main thread to wake up the helper threads before they have actually started blocking.  When they begin blocking later, the main thread will not try to wake them up again, and deadlocks.

Backout:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=54c5a8af9831a0a36ed39c6f4581101acdcae79b
Attached patch patchSplinter Review
Updated patch, sorry about the churn.
Attachment #9004302 - Attachment is obsolete: true
Attachment #9005215 - Flags: review?(jdemooij)
Attachment #9005215 - Flags: review?(jdemooij) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcecbdfea114
Avoid unlocking the helper thread state lock after looking for tasks to perform, r=jandem.
https://hg.mozilla.org/mozilla-central/rev/dcecbdfea114
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.