[FIXr]use nsCOMArray in nsThreadPool

RESOLVED FIXED in mozilla1.3alpha

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.3alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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?
Created attachment 103468 [details] [diff] [review]
patch v 1.0.1, fixes uninitialized rv (thanks, timeless!)
Attachment #103439 - Attachment is obsolete: true

Comment 3

16 years ago
why are your modifying mShuttingDown?  
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

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

r=dougt
Attachment #103468 - Flags: review+

Comment 6

16 years ago
alec, can you sr this?

Comment 7

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

oops, thought I had...
sr=alecf
Attachment #103468 - Flags: superreview+
Created attachment 104905 [details] [diff] [review]
same thing but without the mShuttingDown change
Attachment #103468 - Attachment is obsolete: true
Comment on attachment 104905 [details] [diff] [review]
same thing but without the mShuttingDown change

marking the reviews
Attachment #104905 - Flags: superreview+
Attachment #104905 - Flags: review+
Priority: -- → P1
Summary: use nsCOMArray in nsThreadPool → [FIXr]use nsCOMArray in nsThreadPool
Target Milestone: --- → mozilla1.3alpha

Comment 10

16 years ago
fixed.  thanks bz
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 11

16 years ago
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

16 years ago
Created attachment 105556 [details] [diff] [review]
try this
> +                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) ? 
Created attachment 105559 [details] [diff] [review]
more like that
Attachment #104905 - Attachment is obsolete: true
Attachment #105556 - Attachment is obsolete: true

Comment 15

16 years ago
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.
Attachment #105559 - Flags: review+
reopening to keep this on my radar...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I missed taking this the first time; it shall not happen again.  ;)
Assignee: dougt → bzbarsky
Status: REOPENED → NEW

Comment 18

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

sr=alecf
Attachment #105559 - Flags: superreview+

Comment 19

16 years ago
I haven't seen the assertion after 3 hours of general browsing with bz's patch.
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.