Last Comment Bug 785201 - IonMonkey: WorkerThreadState::wait() is not threadsafe
: IonMonkey: WorkerThreadState::wait() is not threadsafe
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-23 13:00 PDT by Sean Stangl [:sstangl]
Modified: 2012-08-23 16:42 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (812 bytes, patch)
2012-08-23 16:30 PDT, Brian Hackett (:bhackett)
sstangl: review+
Details | Diff | Splinter Review

Description User image Sean Stangl [:sstangl] 2012-08-23 13:00:56 PDT
There are two bugs in WorkerThreadState::wait() that break thread safety.

1) wait() takes a parameter 'millis', which signals to PR_WaitCondVar() that locking should abort after a certain interval has expired. Aborting returns an error, not success, but the function erroneously presumes that the lock is always held after PR_WaitCondVar() returns. Fortunately it seems that "millis" is never a used argument, so we also have the option of removing it instead of fixing it.

The proposed solution from MDN is: "To detect the expiration of the specified interval, call PR_IntervalNow() before and after the call to PR_WaitCondVar and compare the elapsed time with the specified interval."

2) The return value of PR_WaitCondVar() is ignored. Threads can be forcibly jostled out of blocking on cvars without acquiring the lock via PR_Interrupt; it is unsafe to ignore the return value.
Comment 1 User image Brian Hackett (:bhackett) 2012-08-23 13:26:28 PDT
(In reply to Sean Stangl from comment #0)
> 1) wait() takes a parameter 'millis', which signals to PR_WaitCondVar() that
> locking should abort after a certain interval has expired. Aborting returns
> an error, not success, but the function erroneously presumes that the lock
> is always held after PR_WaitCondVar() returns.

I don't think the 'attempt to reaqcuire the lock' part of the docs is very clear.  When the thread is rescheduled, either due to being notified or due to an interval timeout, it will reacquire the lock.  This is (well, had better be) infallible, as per PR_Lock; otherwise we'd need to test the return value on every PR_WaitCondVar, whether there is a timeout specified or not.

> 2) The return value of PR_WaitCondVar() is ignored. Threads can be forcibly
> jostled out of blocking on cvars without acquiring the lock via
> PR_Interrupt; it is unsafe to ignore the return value.

Eh, we don't use the PR_Interrupt API and have no plans to, but asserting on the PR_WaitCondVar's return value would be good, along with the places we use PR_WaitCondVar for other threads (e.g. the GC background thread).
Comment 2 User image Sean Stangl [:sstangl] 2012-08-23 14:28:33 PDT
(In reply to Brian Hackett (:bhackett) from comment #1)
> (In reply to Sean Stangl from comment #0)
> > 1) wait() takes a parameter 'millis', which signals to PR_WaitCondVar() that
> > locking should abort after a certain interval has expired. Aborting returns
> > an error, not success, but the function erroneously presumes that the lock
> > is always held after PR_WaitCondVar() returns.
> 
> I don't think the 'attempt to reaqcuire the lock' part of the docs is very
> clear.  When the thread is rescheduled, either due to being notified or due
> to an interval timeout, it will reacquire the lock.  This is (well, had
> better be) infallible, as per PR_Lock; otherwise we'd need to test the
> return value on every PR_WaitCondVar, whether there is a timeout specified
> or not.

That makes sense; re-reading the documentation with the above text in mind, it's clear that interval expiry just causes the thread to unblock on the cvar and block on the external mutex.

Adding the assert sounds fine.
Comment 3 User image Brian Hackett (:bhackett) 2012-08-23 16:30:59 PDT
Created attachment 654838 [details] [diff] [review]
patch

Add assert for PR_WaitCondVar.
Comment 4 User image Brian Hackett (:bhackett) 2012-08-23 16:42:10 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/e58be9409de7

Note You need to log in before you can comment on or make changes to this bug.