83508 - crash in thread cleanup in nsExceptionService

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John Bandhauer]

Hey Mark. Two bugs for the price of one!

In nsExceptionService::DoDropThread...
http://lxr.mozilla.org/seamonkey/search?string=nsExceptionService%3A%3ADoDrop

(I'll attach this too to make it easy to apply)

Index: nsExceptionService.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/base/nsExceptionService.cpp,v
retrieving revision 1.2
diff -u -r1.2 nsExceptionService.cpp
--- nsExceptionService.cpp      2001/05/08 17:39:22     1.2
+++ nsExceptionService.cpp      2001/05/31 18:15:23
@@ -304,9 +304,9 @@
 /*static*/ void nsExceptionService::DoDropThread(nsExceptionManager *thread)
 {
     if (thread == firstThread) {
-        firstThread = nsnull;
+        firstThread = firstThread->mNextThread;
     } else {
-        nsExceptionManager *look = firstThread->mNextThread;
+        nsExceptionManager *look = firstThread;
         while (look && look->mNextThread != thread) {
             look = look->mNextThread;
         }

1) The first change avoids a later crash. You don't want to orphan the list just 
because you're removing the first item.

2) The other change keeps you from failing to find the thread when it is the 
second item in the list; i.e. thread == firstThread->mNextThread.

I just checked in an old xpcshell multithreaded test that had been kicking 
around (mostly to find threading bugs in the JS Engine). See 
mozilla/js/src/xpconnect/tests/js/old/threads.js

If you like these changes then please take care of getting them into the tree. 
sr=jband, r=you, approval you have to get.

Comment 1

[John Bandhauer]

Attachment: proposed fix

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Eeek - sorry about that John, and thanks for the patch!

Mail sent to drivers seeking a=

Comment 3

[Brendan Eich [:brendan]]

Unify the basis and induction cases!  Patch coming up, also provides an r/sr= if
you like it (jband can't sr= his own patches, drivers will insist).

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: minimal patch

Comment 5

[Mark Hammond [:markh] [:mhammond]]

The patch looks good (and elegant) to me!  Release builds could die in 
pathological cases (ie, if the assert ever fails in debug build, the same 
scenario will crash release), but I assume we are happy with the debug only 
check?  I am :)  

I am a little confused by Brendan's comment on the the sr= - if you are now 
offering the sr=, aren't we in the same position as before - ie, you sr'ing 
your own patch?

Maybe just to save confusion jband can _now_ offer the sr!

Still need an a= tho!

Comment 6

[Christopher Blizzard (:blizzard)]

a=blizzard for the trunk

Comment 7

[John Bandhauer]

sr=jband

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Checked in rev 1.3 of nsExceptionService.cpp.