166371 - Crashes at [@ nsThreadPool::Shutdown] in Trunk, N700

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[greer]

Crashes with the nsThreadPool::Shutdown signature are near the top of the list
of N7.00 crashes. The signature is also showing up in Trunk builds as recently
as 8/26. This needs to be fixed in the next point release.

The stack for this crash is mentioned in bug 134724 comment #19, but may be a
different issue.

Assigning to timeless (owner of bug 134724) for first look.

 Stack trace(Frame) 

	 nsThreadPool::Shutdown	[d:\builds\seamonkey\mozilla\xpcom\threads\nsThread.cpp
 line 745] 
	 nsFileTransportService::Shutdown
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsFileTransportService.cpp  line 226] 
	 nsIOService::Observe
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsIOService.cpp  line 1045] 
	 nsObserverService::NotifyObservers
[d:\builds\seamonkey\mozilla\xpcom\ds\nsObserverService.cpp  line 213] 
	 NS_ShutdownXPCOM	[d:\builds\seamonkey\mozilla\xpcom\build\nsXPComInit.cpp 
line 544] 
	 main	[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 1847] 
	 WinMain	[d:\builds\seamonkey\mozilla\xpfe\bootstrap\nsAppRunner.cpp  line 1857] 
	 WinMainCRTStartup()  
	 KERNEL32.DLL + 0x1b560 (0xbff8b560)  
	 KERNEL32.DLL + 0x1b412 (0xbff8b412)  
	 KERNEL32.DLL + 0x19dd5 (0xbff89dd5)

Comment 1

[greer]

Attachment: comments

FWIW, user comments from the N700 data.

Comment 2

[timeless]

:-(, i'll look, but i could probably use some help

Comment 3

[timeless]

ok, i'm going to bet that multiple threads call shutdown

thread 1 and 2 enter:
721 nsThreadPool::Shutdown

thread 1 reaches
733     nsAutoLock lock(mLock);
thread 2 reaches it and gets stuck
thread 1 reaches
756     mThreads = nsnull;
757     return rv;
thread 2 gets the lock and decides to do:
742     rv = mThreads->EnumerateForwards(nsThreadPool::InterruptThreads, nsnull
which is reported as line 745 by talkback just to annoy me.

How's my driving?

Comment 4

[Andrew Hagen]

Related to bug 160922 (multiple calls for shutdown)?

Comment 5

[timeless]

Attachment: condition and assertion are negotiable

so this patch should theoretically work assuming i got the right cause.

mShuttingDown and mThreads are really equivalent conditions both are made false
by ::Shutdown() after it acquires the lock.

This might also be a caller bug, but I don't have an environment here to try to
figure that out. For 1.2a I think a variant of this should do. darin said he'd
look at it.

Comment 6

[timeless]

Attachment: same patch, but for the caller

Comment 7

[Darin Fisher]

Comment on attachment 97723 [details] [diff] [review]
condition and assertion are negotiable

>Index: mozilla/xpcom/threads/nsThread.cpp

>+        NS_ASSERTION(mThreads, "Bug 166371 was Shutdown() called more than once or did someone forget to call Init()?");

can a bug really be shutdown??	perhaps a ':' would do the trick ;-)

this patch seems like the right way to protect against multiple calls
to Shutdown() on different threads.  i'd really like to know if that's
actually the problem here or not.  there is the possibility that
nsFileTransportService::Shutdown is being called while another thread
is inside nsFileTransportService::Add/RemoveSuspendedTransport because
the locking in nsFileTransportService seems pretty broken to me.

sr=darin (fwiw)

Comment 8

[Darin Fisher]

Comment on attachment 97727 [details] [diff] [review]
same patch, but for the caller

>Index: mozilla/netwerk/base/src/nsFileTransportService.cpp

>     PR_Lock(mLock);
>-    mShuttingDown = 1;
>+    if (mShuttingDown) {
>+        NS_ASSERTION(!mShuttingDown, "Bug 166371 was Shutdown() called more than once!");
>+        return NS_OK;
>+    }
>+    mShuttingDown = PR_TRUE;
>     PR_Unlock(mLock);


this locking is silly...  assignment to a PRUint32 is atomic on all the
processors
we care about.	if you want, you could rewrite this like so:

  if (mShuttingDown)
      return NS_OK;
  mShuttingDown = PR_TRUE;

Comment 9

[timeless]

Attachment: combined version per darin (fixed1.0.2)

Comment 10

[Darin Fisher]

Comment on attachment 97733 [details] [diff] [review]
combined version per darin (fixed1.0.2)

sr=darin, what about the assertion text? :-)

we also need to seriously look into protecting mSuspendedTransportList inside
Shutdown.  things would blow up in a big way if Add/RemoveSuspendedTransport
were called while inside Shutdown :(

Comment 11

[timeless]

Comment on attachment 97733 [details] [diff] [review]
combined version per darin (fixed1.0.2)

r=bz on irc
checked in
leaving open to ponder locking for mSuspendedTransportList

Comment 12

[Brian Ryner (not reading)]

The assignment is atomic, but a check + assignment is not atomic, is it?  i.e.
could we have a situation where two threads enter Shutdown at approximately the
same time, thread 1 finds mShuttingDown to be false, and before it sets
mShuttingDown to true, thread 2 has checked mShuttingDown and found it to be
false also?  Then both threads would execute the entire function... it seems
like we need a lock around the check _and_ assignment.

Comment 13

[Darin Fisher]

i should have qualified my last comment with :
nsFileTransportService::Shutdown() is only called on the UI thread ;-)  we know
this because it is called from nsIOService::Observe() in response to the
"xpcom-shutdown" event.  i don't see any way for this notification to arrive on
any thread other than the UI thread.

i think the crash may actually be caused by "xpcom-shutdown" happening twice.

or, it may be caused by "xpcom-shutdown" happening while a background file
transport thread is inside Add/RemoveSuspendedTransport.  see comment #7.

Comment 14

[Shiva Thirumazhusai]

timeless: Is the patch checked into Trunk only ?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

namachi - seeing that it does not have any approval, I'd hope that it's only in
trunk.

Comment 16

[Jay Patel [:jay]]

Have we verified timeless's fix on the Trunk?  This is currently the #3
topcrasher for Netscape 7.0.  

If his patch is safe and we know it fixed most of the crashes...it might be
worth a shot getting it into Blackbird?  I doubt it, but just thought I would
throw that out there.

Comment 17

[[e-mail] (gone - use [e-mail] instead)]

According to Talkback:
http://climate/reports/VeryFastSearchStackSigNEW.cfm?stacksig=nsThreadPool%3A%3AShutdown

this frame hasn't crashed since 8/26/2002.

Comment 18

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

Isn't there a (very small) hole in this code?

 nsresult
 nsFileTransportService::Shutdown(void)
 {
-    PR_Lock(mLock);
-    mShuttingDown = 1;
-    PR_Unlock(mLock);
+    if (mShuttingDown)
+        return NS_OK;
+
+    mShuttingDown = PR_TRUE;

Shouldn't that be more like:

     PR_Lock(mLock);
     if (mShuttingDown)
     {
         PR_Unlock(mLock);
         return NS_OK;
     }
     mShuttingDown = PR_TRUE;
     PR_Unlock(mLock);

Comment 19

[Darin Fisher]

no, because Shutdown is only called on the UI thread.

Comment 20

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

As timeless pointed out, Darin already answered the race-condition question.  I
still prefer using locking, since it's not good to make this sort of unchecked
assumption about how other modules will call you, but in practice we're ok for
now with it.

Comment 21

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

Comment on attachment 97733 [details] [diff] [review]
combined version per darin (fixed1.0.2)

a=[e-mail] for 1.0 branch; change mozilla1.0.2+ to fixed1.0.2 when
checked in

Comment 22

[Darin Fisher]

rjesup: adding a lock here would buy you very little.  we'd crash and burn
elsewhere if someone tried to shutdown the file transport service on a
background thread.  shutdown is only called from the io service when it receives
a xpcom-shutdown notification, which always occurs on the main thread.  if that
ever changes, much code would need to be rev'd.

Comment 23

[benc]

timeless: can you explain how this is keyword==fixed1.0.2 while the
status==assigned?

Comment 24

[Jimmy Lee]

I see checkins on 9/3 for the TRUNK.  I cannot find any for the BRANCH.

Ben, please verify this fix on the TRUNK.  If it's good, then we should feel
better about it landing on the BRANCH.  Thanks.

Comment 25

[timeless]

Comment on attachment 97733 [details] [diff] [review]
combined version per darin (fixed1.0.2)

http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=MOZILL
A_1_0(_0)?_BRANCH&branchtype=regexp&cvsroot=/cvsroot&date=explicit&mindate=1034
201640&maxdate=1034202180&who=timeless%25mac.com

Comment 26

[benc]

Sorry. I'm drawing the line here. Someeone else is going to have to take this
verification.

Comment 27

[Tom Everingham]

timeless, darin.  Is this being left open for additional work (comment# 11)?  Or
can it be marked fixed?

I am not seeing this crash in any recent talkback reports and the patch is
checked in.

Comment 28

[Darin Fisher]

marking FIXED

Comment 29

[Tom Everingham]

verified trunk and branch