175440 - [FIXr]use nsCOMArray in nsThreadPool

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Attaching patch to switch nsThreadPool to nsCOMArray for its request arrays.  I
considered switching mThreads as well, but was hampered by lack of an equivalent
for RemoveLastElement() (which deletes the _last_ occurrence of an element in
the array).

So the question, of course, is whether we want this.  ;)  The net win is clearer
code; I think the one place where we were returning an addrefed request may have
leaked under some odd conditions....  In any case, what do you think?  Yes?  No?
 Maybe?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: proposed patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: patch v 1.0.1, fixes uninitialized rv (thanks, timeless!)

Comment 3

[Doug Turner (:dougt)]

why are your modifying mShuttingDown?  

Comment 4

[Boris Zbarsky [:bzbarsky]]

Mostly because it seems to make a lot more sense this way (and the
comments/assertions in the file seem to assume that mShuttingDown is false when
we are not initialized).

But that was not supposed to be in the patch, really; please ignore that part.

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 103468 [details] [diff] [review]
patch v 1.0.1, fixes uninitialized rv (thanks, timeless!)

r=dougt

Comment 6

[Doug Turner (:dougt)]

alec, can you sr this?

Comment 7

[Alec Flett]

Comment on attachment 103468 [details] [diff] [review]
patch v 1.0.1, fixes uninitialized rv (thanks, timeless!)

oops, thought I had...
sr=alecf

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: same thing but without the mShuttingDown change

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104905 [details] [diff] [review]
same thing but without the mShuttingDown change

marking the reviews

Comment 10

[Doug Turner (:dougt)]

fixed.  thanks bz

Comment 11

[Nicholas Allen]

This fix looked like the obvious target to blame.

Seen with a 1hr old cvs trunk build Win2k:

###!!! ASSERTION: nsVoidArray::ElementAt(index past end array) - note on bug 
96108: 'aIndex < Count()', file 
d:/mozilla/mozilla/xpcom/build/../ds\nsVoidArray.h, line 78

nsVoidArray::ElementAt(int 0x00000001) line 78 + 35 bytes
nsCOMArray_base::ObjectAt(int 0x00000001) line 95
nsCOMArray<nsIRunnable>::ObjectAt(int 0x00000001) line 144
nsThreadPool::GetRequest(nsIThread * 0x02bf9a60) line 588 + 15 bytes
nsThreadPoolRunnable::Run(nsThreadPoolRunnable * const 0x02c6d888) line 896 + 
29 bytes
nsThread::Main(void * 0x02bf9a60) line 120 + 26 bytes
_PR_NativeRunThread(void * 0x037d3b78) line 433 + 13 bytes
_threadstartex(void * 0x02c641b0) line 212 + 13 bytes
KERNEL32! 77e887dd()

Comment 12

[timeless]

Attachment: try this

Comment 13

[Boris Zbarsky [:bzbarsky]]

> +                break;  

Why?

> +                // if we are breaking here, it means that either we have a bad 

The comment is now bogus

>             if (pendingThread != -1) {

Why not just if (pendingThread < requestCnt) ? 

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: more like that

Comment 15

[timeless]

Comment on attachment 105559 [details] [diff] [review]
more like that

yeah, i ended up with that as draft 2 :-).  In the third draft i converted the
other break into a loop condition.

Comment 16

[Boris Zbarsky [:bzbarsky]]

reopening to keep this on my radar...

Comment 17

[Boris Zbarsky [:bzbarsky]]

I missed taking this the first time; it shall not happen again.  ;)

Comment 18

[Alec Flett]

Comment on attachment 105559 [details] [diff] [review]
more like that

sr=alecf

Comment 19

[Nicholas Allen]

I haven't seen the assertion after 3 hours of general browsing with bz's patch.

Comment 20

[Boris Zbarsky [:bzbarsky]]

checked in