84489 - threadpool assumes runnables are threadsafe

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Doug Turner (:dougt)]

The current threadpool makes assumptions that the nsIRunnables are threadsafe.  
This causes some nasty problems such as 71397.

The threadpool should enforce a policy which allows non-threadsafe 
implementations of nsIRunnable to safely exists.  Making sure that the same 
request is not running on more than one thread at the same time will enforce 
such a policy.

Comment 1

[Doug Turner (:dougt)]

Attachment: Add Threadpool concurrency test to TestThreads

Comment 2

[Doug Turner (:dougt)]

The last patch adds a testcase to TestThreads which fingers the problem which 
has been reported as a top crasher with the stack ending in nsFileIO crashers. 
(see 71397).  

The test basically has one request run multiple times.  The request's Run() is 
not threadsafe. So, setting/getting the member var will be overwritten by other 
threads running the same request. 

CC'ing thread hackers. 

Comment 3

[Doug Turner (:dougt)]

Proposed Changes:


 Added new variable for keeping track of the currently running requests: 
mRunningRequests.

 Renamed some variables name so that we don't confuse pending requests with 
running requests.
 
 Added new function to the nsThreadPool so that I could tell when the request is 
finished: |RequestDone|.  I am a little unhappy about having to add this 
function, but I don't see any other way that I can tell the pool from the 
runnable that the runnable is done.  This function simply removes the request
 from the mRunningRequests list.

 The biggest change is that the first thing we do inside of 
nsThreadPool::GetRequest is to check to see that the request that we pull out of 
mPendingRequests is not already running.  If it is, we try the next one in the 
list and so on.  If we find that there are not request which can be run, we do 
what we have been doing - check to see if the thread should go away otherwise 
wait.

Comment 4

[Doug Turner (:dougt)]

Attachment: Proposed changes to threadpool v.1

Comment 5

[Darin Fisher]

looks good to me!  sr=darin

Comment 6

[Doug Turner (:dougt)]

great.  dan, can you r= this?  Brendan, can you give me a fat a=?

Comment 7

[Dan M]

(Me writing down what I believe I've discovered about this bug) Doug explains 
that requests are being taken off the pending list, set running, and then 
returned to the pending list when their action is cancelled. For a moment the 
pending list contains bogus entries; this patch guards against that. OK. r=danm

Comment 8

[Doug Turner (:dougt)]

I am concerned about what you think I fixed. :-)

The basic idea is that if a runnable R is already running on any of the 
threadpool's worker thread, it doesn't run the same runnable R, dispatched as a 
new request, until the running runnable is finished.

really a rad run ran rhyme.

Comment 9

[clayton]

a=clayton

Comment 10

[Doug Turner (:dougt)]

Fix checked in.