72599 - crash in file transport service at shutdown

Status: VERIFIED FIXED

(Core :: Networking: File, defect)

Description

[Darin Fisher]

#0  0x400f0472 in nsSupportsArray::ElementAt (this=0x8172ab8, aIndex=168) at
nsSupportsArray.cpp:126
#1  0x400f130a in nsSupportsArray::GetElementAt (this=0x8172ab8, aIndex=168,
result=0xbffff6f0)
    at nsSupportsArray.h:43
#2  0x40d7d66b in nsFileTransportService::Shutdown (this=0x8172aa0) at
nsFileTransportService.cpp:159
#3  0x40d6f233 in nsIOService::~nsIOService (this=0x814a2c8, __in_chrg=3) at
nsIOService.cpp:110
#4  0x40d6f590 in nsIOService::Release (this=0x814a2c8) at nsIOService.cpp:154
#5  0x40128120 in nsServiceManagerImpl::ReleaseService (this=0x806d8c0,
aClass=@0x4055e190, 
    service=0x814a2c8, shutdownListener=0x0) at nsServiceManager.cpp:403
#6  0x401286ea in nsServiceManager::ReleaseService (aClass=@0x4055e190,
service=0x814a2c8, 
    shutdownListener=0x0) at nsServiceManager.cpp:569
#7  0x40544de8 in ?? () from /builds2/darin/mozilla/dist/bin/components/libuconv.so
#8  0x4054ef24 in ?? () from /builds2/darin/mozilla/dist/bin/components/libuconv.so
#9  0x4054eaa0 in ?? () from /builds2/darin/mozilla/dist/bin/components/libuconv.so
#10 0x401279f8 in DeleteEntry (aKey=0x8163800, aData=0x81637e8, closure=0x0) at
nsServiceManager.cpp:258
#11 0x400e8cf0 in _hashEnumerateRemove (he=0x8163820, i=71, arg=0xbffff898) at
nsHashtable.cpp:369
#12 0x402a3cb1 in ?? () from ./libplds4.so
#13 0x400e8d87 in nsHashtable::Reset (this=0x806d8d8, 
    destroyFunc=0x401279bc <DeleteEntry(nsHashKey *, void *, void *)>,
closure=0x0) at nsHashtable.cpp:385
#14 0x400e991e in nsObjectHashtable::Reset (this=0x806d8d8) at nsHashtable.cpp:627
#15 0x400e97ed in nsObjectHashtable::~nsObjectHashtable (this=0x806d8d8,
__in_chrg=3) at nsHashtable.cpp:593
#16 0x40127b6a in nsServiceManagerImpl::~nsServiceManagerImpl (this=0x806d8c0,
__in_chrg=3)
    at nsServiceManager.cpp:283
#17 0x40127cb6 in nsServiceManagerImpl::Release (this=0x806d8c0) at
nsServiceManager.cpp:292
#18 0x401285da in nsServiceManager::ShutdownGlobalServiceManager (result=0x0) at
nsServiceManager.cpp:544
#19 0x400e067e in NS_ShutdownXPCOM (servMgr=0x0) at nsXPComInit.cpp:471
#20 0x08057af3 in main (argc=2, argv=0xbffffab4) at nsAppRunner.cpp:1306
#21 0x403849cb in __libc_start_main (main=0x80578ac <main>, argc=2,
argv=0xbffffab4, init=0x8051920 <_init>, 
    fini=0x80661ac <_fini>, rtld_fini=0x4000ae60, stack_end=0xbffffaac) at
../sysdeps/generic/libc-start.c:92

Comment 1

[Doug Turner (:dougt)]

darin, I am not seeing this in today build.

Comment 2

[Darin Fisher]

dougt: i think this is a difficult one to reproduce.  i saw it only once
on g.  perhaps the dual processor'ness of g makes it more likely to occur.
anyways, i think that the adding/removing/iterating of the suspended file
transport list needs to be protected by a lock.  any of the 4 file transport
threads could potentially access this.  the ui thread may even hit it in
the case of file transport service shutdown.

Comment 3

[Doug Turner (:dougt)]

moving to 0.9.1

Comment 4

[Doug Turner (:dougt)]

Attachment: Protects mSuspendedTransportList

Comment 5

[Doug Turner (:dougt)]

brendan, darin, can I get a review?  

Comment 6

[Brendan Eich [:brendan]]

The Append and RemoveElement calls do need to be protected, and PRLock seems
like the right tool (with nsAutoLock automation).  But why does Shutdown race
with other threads doing AddSuspendedTransport and/or RemoveSuspendedTransport?
If it can so race, then you need to worry about whether Count() returns a value
greater than the actual count at the point where Shutdown calls GetElementAt,
perhaps by coping with a null supports out parameter.

You'd also need to lock mSuspendedTransportList.Clear() against racing Add and
Remove calls, but at this point, in what sense is the transport service shut
down, if there are racers?

/be

Comment 7

[Doug Turner (:dougt)]

brendan, your right.  Access to the suspended transport list should be denied 
during shutdown.  Another patch coming up.

Comment 8

[Doug Turner (:dougt)]

Attachment: Patch adds lock which products shutdown flag and suspended transport list.

Comment 9

[Doug Turner (:dougt)]

products?  I mean protects.  

Comment 10

[Doug Turner (:dougt)]

I got a review from [e-mail], and r=[e-mail].  brendan,
care to look at this patch again?  I want to get it in for 0.9.1 if it looks
good.

Comment 11

[Brendan Eich [:brendan]]

sr=[e-mail] on the patch, it can only help.  I still would like to
hear how other threads race with shutdown.  Does the main thread just shutdown
at any time, without doing a handshake of some sort with the file transport
thread(s)?

I think this should go in for 0.9.1, but I'll let another driver a= it.

/be

Comment 12

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Comment 13

[Doug Turner (:dougt)]

gagan can you check this in?

Comment 14

[Gagan]

ok checked in.

Comment 15

[benc]

brendan|blizzard: as a "code change" bug report, I have no steps to verify. 
Can you mark this VERIFIED?

Comment 16

[Brendan Eich [:brendan]]

I can verify this, but now I see what I missed before: 1 assigned to
mShuttingDown rather than PR_TRUE?  Doug, say it ain't so!  Please fix with
rs=[e-mail], r=lumpy for all I care -- or fix in your next change to
the same file.

/be

Comment 17

[Darin Fisher]

cc'ing dougt... see brendan's comments.

Comment 18

[Doug Turner (:dougt)]

don't know what the hell I was thinking.  But, my only saving grace is:

http://lxr.mozilla.org/seamonkey/source/nsprpub/pr/include/prtypes.h#412
Next time I am in that file I will fix it.