72320 - abort on exit (sending "logout" to a closed IMAP socket?)

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

sometimes I abort on exit.

it looks like we are trying to send "logout" on a closed socket.

#0  0x402e14e1 in __kill () from /lib/libc.so.6
#1  0x402551eb in raise (sig=6) at signals.c:64
#2  0x402e2868 in abort () at ../sysdeps/generic/abort.c:88
#3  0x40221945 in PR_Assert ()
   from /builds/seth/seamonkey/mozilla/dist/bin/libnspr4.so
#4  0x4021f0cf in _PR_InvalidInt ()
   from /builds/seth/seamonkey/mozilla/dist/bin/libnspr4.so
#5  0x4021f285 in PR_Write ()
   from /builds/seth/seamonkey/mozilla/dist/bin/libnspr4.so
#6  0x40adf9a5 in nsSocketBOS::Write (this=0x8956670,
    aBuf=0x83e24b0 "8 logout\r\n", aCount=10, aBytesWritten=0xbffff304)
    at nsSocketTransport.cpp:1922
#7  0x41bd70bd in nsImapProtocol::TellThreadToDie (this=0x895f748,
    isSaveToClose=1) at nsImapProtocol.cpp:837
#8  0x41ba5b08 in nsImapIncomingServer::CloseCachedConnections (this=0x880c538)
    at nsImapIncomingServer.cpp:899
#9  0x415284be in nsMsgAccountManager::closeCachedConnections (aKey=0x880c908,
    aData=0x880c538, closure=0x0) at nsMsgAccountManager.cpp:1044
#10 0x400aca83 in _hashEnumerate (he=0x880c920, i=0, arg=0xbffff3f4)
    at nsHashtable.cpp:193
#11 0x4020c5b0 in PL_HashTableEnumerateEntries ()
   from /builds/seth/seamonkey/mozilla/dist/bin/libplds4.so
#12 0x400acf62 in nsHashtable::Enumerate (this=0x82a8e28,
    aEnumFunc=0x415284a0 <nsMsgAccountManager::closeCachedConnections(nsHashKey
*, void *, void *)>, closure=0x0) at nsHashtable.cpp:359
#13 0x4152aad4 in nsMsgAccountManager::CloseCachedConnections (this=0x82a8dd0)
    at nsMsgAccountManager.cpp:1398
#14 0x41523642 in nsMsgAccountManager::Shutdown (this=0x82a8dd0)
    at nsMsgAccountManager.cpp:191
#15 0x41523793 in nsMsgAccountManager::Observe (this=0x82a8dd0,
    aSubject=0x8065340, aTopic=0xbffff870, someData=0x0)
    at nsMsgAccountManager.cpp:219
#16 0x400b0339 in nsObserverService::Notify (this=0x80a8350,
    aSubject=0x8065340, aTopic=0xbffff870, someData=0x0)
    at nsObserverService.cpp:236
#17 0x400a63dc in NS_ShutdownXPCOM (servMgr=0x0) at nsXPComInit.cpp:446
#18 0x8052f81 in main (argc=2, argv=0xbffff994) at nsAppRunner.cpp:1306

Comment 1

[David :Bienvenu]

This is due to necko changes - I'll have to add something like the following to
the imap code.

    if (m_transport)
    {
      nsCOMPtr <nsISocketTransport> socket = do_QueryInterface(m_transport);
      if (socket)
        socket->SetReuseConnection(PR_TRUE);
    }

Comment 2

[David :Bienvenu]

*** Bug 72963 has been marked as a duplicate of this bug. ***

Comment 3

[Darin Fisher]

bienvenu: i can attach a patch to add the null checks, but i suspect that that
might only mask the problem.  can someone in mailnews investigate this.

Comment 4

[David :Bienvenu]

please attach the patch - I don't think any other problem is being masked,
except for the problem where we receive an error on send from the smtp server
and reading that data causes us to close the connection (I don't have the bug #
handy, but you're cc'ed on it). This problem is *not* that problem.

Comment 5

[Darin Fisher]

Attachment: Addes NULL checks to protect from crash.

Comment 6

[David :Bienvenu]

I'll try this out on windows - do you want to try it on Linux, Seth?

Comment 7

[(not reading, please use [e-mail] instead)]

I'll test on linux.

Comment 8

[David :Bienvenu]

I have not seen any problems with this patch. Checking it in would make the
product *much* more useable for some people. sr=bienvenu for it.

Comment 9

[Scott MacGregor]

*** Bug 73198 has been marked as a duplicate of this bug. ***

Comment 10

[David :Bienvenu]

the patch does not fix the problem. the socket is not null; it's invalid. All
it's methods are _pr_faulty_methods. I believe we need to check if the socket is
valid or not. Or figure out why the transport is holding onto an invalid socket.

Comment 11

[Darin Fisher]

sounds like the socket is being closed, but not cleared in the blocking
output stream.  working on a patch...

Comment 12

[Darin Fisher]

Attachment: The real deal

Comment 13

[Darin Fisher]

dougt, mscott, rpotts:  i've written up a patch for the socket transport that
hopefully fixes the "ownership/lifetime problems" we've been noticing.

each request/stream opened on the socket transport now gets an owning reference
to the socket itself.  when each of the request/stream's goes away, it releases
its reference to the socket.  by keeping a reference count like this, it is
trivial to determine when the socket should be closed.

clients of a particular socket transport can still call SetReuseConnection to
keep the socket transport alive.  this corresponds to incrementing the socket
reference count.

i've spent some time testing this on my linux build.  i will have to do more
testing of course to avoid regressions (my fear is that people might open a
request and then try to open another request inside the onStopRequest of the
first... and since onStopRequest is proxied, the first request could go away
before the second is issued, resulting in the socket being closed prematurely.
i think that this should only be a problem for non-keep-alive HTTP... so i am
working on a patch for HTTP that will toggle SetReuseConnection for the 
required duration to avoid this race condition.  are there other protocols
that might need to be concerned about this?  mailnews and ftp seem okay to me.)

Comment 14

[Scott MacGregor]

Darin your patch looks good to me although I haven't yet tried it to see if it
fixes the problem. I don't think you need to worry about other protocols opening
another request on the same transport inside of an OnStopRequest. well at least
I can vouch that you are safe from that behavior in mailnews. http is the only
one I can think of anyway. r/sr=mscott.

Comment 15

[Darin Fisher]

-> myself

Comment 16

[(not reading, please use [e-mail] instead)]

r=sspitzer

fixes the crasher with imap.

now I can debug, lose my connection to the server, and I don't abort.

Comment 17

[(not reading, please use [e-mail] instead)]

*** Bug 72317 has been marked as a duplicate of this bug. ***

Comment 18

[Doug Turner (:dougt)]

sr=dougt

Comment 19

[Darin Fisher]

reopening b/c i haven't checked in the fix yet.  i'll check it in once the 
tree goes green... thanks everyone for reviewing this patch!

Comment 20

[Darin Fisher]

fix checked in

Comment 21

[esther]

Using build on 4-3 on winme, mac and linux exiting after being logged into my 
POP and/or just my IMAP account does not abort or crash. Note: we have newer 
bugs for exiting crashing (like 74770) but it's not the same scenario.  
Verified.