141479 - nsThreadPool::~nsThreadPool crash [@ nsThreadPool::Shutdown]

Status: VERIFIED FIXED

(Core :: XPCOM, defect)

Description

[Mathieu Fenniak]

I've been developing a stand-alone application using XPCOM (based off RC1
source) and I've come across a crash in nsThreadPool::~nsThreadPool caused by
uninitialized variables. Specifically, mLock, mThreadExit, mPendingRequestAdded,
and mPendingRequestsAtZero are all NULL-checked before being cleaned up, but are
never initialized. The following patch fixed my crash by initializing these
values in nsThreadPool::nsThreadPool, in xpcom/threads/nsThread.cpp:

diff -c -r1.40.22.1 nsThread.cpp
*** xpcom/threads/nsThread.cpp  10 Apr 2002 03:38:22 -0000      1.40.22.1
--- xpcom/threads/nsThread.cpp  1 May 2002 14:28:36 -0000
***************
*** 480,485 ****
--- 480,489 ----
        mShuttingDown(PR_FALSE)
  {
      NS_INIT_REFCNT();
+     mLock = nsnull;
+     mThreadExit = nsnull;
+     mPendingRequestAdded = nsnull;
+     mPendingRequestsAtZero = nsnull;
  }

  nsThreadPool::~nsThreadPool()

Comment 1

[Olivier Cahagne]

Didn't find dupes. Marking NEW.

Might be related: bug 106585.

Comment 2

[Doug Turner (:dougt)]

Attachment: Mathieu Fenniak patch

The patch looks good, but I wonder why Init() on the threadpool was never
called or  why Init() is failing prior to we init those variables.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 81894 [details] [diff] [review]
Mathieu Fenniak patch

I guess, if for any reason that anything failed, we could get into this state,
r=dougt

Comment 4

[greer]

Attachment: Recent stack and comments from talkback data

Mathieu, Talkback data has 73 incidents under the nsThreadPool::Shutdown
signature (nsThread.cpp  line 741). Doug T thinks its this bug. Do the user
comments sound like the problems you see without your patch?

Comment 5

[Mathieu Fenniak]

The crash I was experiencing before this patch was on line 492 of nsThread.cpp.
My application was busy doing something that had absolutely nothing to do with
XPCOM at the time, yet still this crash was intermittant. I found a set of
printf()s eventually that, when removed, caused my crash consistantly. The
actions occuring in my program seemed to have no relation to the actual crash.

Such happens occasionally with multi-threading.

Comment 6

[Darin Fisher]

what if Init() fails after mLock is allocated?  then PR_DestroyLock would be
called twice?  once in the "cleanup:" section and again in ~nsThreadPool.  we
should NULL out mLock (and friends) in the "cleanup:" section of Init().

Comment 7

[Doug Turner (:dougt)]

Attachment: Addresses darin's comments

great point darin.

Comment 8

[Mathieu Fenniak]

The variable mLock had gdb's uninitialized variable value when I was debugging
my crash. The value of these variables shouldn't be null-checked in the
destructor if they could be never initialized to null.

Comment 9

[Doug Turner (:dougt)]

 Mathieu, the last patch should fix that.  Can you verify?

Comment 10

[Mathieu Fenniak]

Looks good to me.

Comment 11

[Darin Fisher]

Comment on attachment 83077 [details] [diff] [review]
Addresses darin's comments

nice, sr=darin

Comment 12

[Suresh Duddi (gone)]

Comment on attachment 83077 [details] [diff] [review]
Addresses darin's comments

r=dp

Comment 13

[Doug Turner (:dougt)]

checked into trunk.


Checking in nsThread.cpp;
/cvsroot/mozilla/xpcom/threads/nsThread.cpp,v  <--  nsThread.cpp
new revision: 1.41; previous revision: 1.40
done

Comment 14

[Aleksey Nogin]

According to brad TBox, this commit have added a new warning on brad:

/mnt/3/tinderbox/brad/Linux_2.4.18-pre3_Clobber/mozilla/xpcom/threads/nsThread.
h:112: warning: member initializers for `struct PRCondVar * nsThreadPool::mPend
ingRequestsAtZero'
/mnt/3/tinderbox/brad/Linux_2.4.18-pre3_Clobber/mozilla/xpcom/threads/nsThread.
h:119: warning:   and `PRUint32 nsThreadPool::mMinThreads'
/mnt/3/tinderbox/brad/Linux_2.4.18-pre3_Clobber/mozilla/xpcom/threads/nsThread.
cpp:482: warning:   will be re-ordered to match declaration order
nsProcessCommon.cpp

Comment 15

[scottputterman]

any ideas about whether or not we should be worried about these warnings before
landing this fix?

Comment 16

[Jaime Rodriguez, Jr.]

Doug - We'd like to take this asap, but we'd like to have the question Scott
asked in the last comment addressed first. thanks!

paw - who can verify this fix on the trunk for us?

Comment 17

[Paul Wyskoczka]

Are there steps to reproduce the crash?  

Comment 18

[Doug Turner (:dougt)]

putterman: 
those variables are now inited in the constructor; nothing to worry about now. 
Still a warning about re-ordered, but that doesn't matter here.

Comment 19

[scottputterman]

adding adt1.0.0+.  Please get drivers approval before checking in.

Comment 20

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch for the Mozilla1.0.1
milestone.  Please get drivers approval before checking in.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 83077 [details] [diff] [review]
Addresses darin's comments

please check into the 1.0.1 branch ASAP. once landed remove the 
mozilla1.0.1+ keyword and add the fixed1.0.1 keyword

Comment 22

[Doug Turner (:dougt)]

Checking in nsThread.cpp;
/cvsroot/mozilla/xpcom/threads/nsThread.cpp,v  <--  nsThread.cpp
new revision: 1.40.22.2; previous revision: 1.40.22.1
done

Comment 23

[Jay Patel [:jay]]

v.fixed per talkback data.  this crash i long gone.