58657 - IMAP thread doesn't call DestroyThreadEventQueue

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect, P3)

Description

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

The IMAP thread releases its own hold on its monitored event queue as it cleans
up, but it doesn't call nsEventQueueService::DestroyThreadEventQueue in order to
cause the service to also remove the event queue from its internal hash table
and release it.

Comment 1

[Scott MacGregor]

Hmm, I wonder if this is the cause of 52152. Sometimes, when shutting down the
Mac (after running imap), we crash in nsThread....If there is a corelation then
you may have saved me a lot of time dmose!

Comment 2

[Scott MacGregor]

One interesting thing. When I did add the call to DestroyThreadEventQueue just
before we exit the imap thread's run method, I then saw 2 assertions on shutdown
complaining about attempting to process events whose thread has already been
destroyed. So we do have some events left in our queue on shutdown that we may
not be handling.

Comment 3

[Scott MacGregor]

Attachment: call DestroyThreadEventQueue

Comment 4

[David :Bienvenu]

I'd bet it's trying to process the logout response when we try to shutdown the
imap connection. This might be fixed by synchronizing our thread with the main
thread (you know, the thing that Warren's always getting on his hobby horse about).

Comment 5

[OOO until Jun 29 - Dan Mosedale (:dmosedale, :dmose)]

Would it work to call StopAcceptingEvents() and then ProcessPendingEvents()
before the call to DestroyThreadEventQueue()?

Comment 6

[Scott MacGregor]

David, should we really be exiting the imap thread until the logout response is
processed?

Comment 7

[David :Bienvenu]

yes and no. If we did wait, some shutdown memory leaks would get fixed. But on
the other hand, we shouldn't hold shutdown of the app hostage to the imap server
processing the logout response, especially if the server is down or something.
4.x did not wait, but not waiting in 4.x didn't cause memory leaks :-(

So, the right thing to do is probably to wait for "a little while". Most of the
time, we'll get the OK response and everything will be fine.

Comment 8

[Scott MacGregor]

Attachment: new fix to process any pending events before we destroy the thread

Comment 9

[Scott MacGregor]

Well this latest suggestion of Dan's certainly fixed the assertions I was seeing
on shutdown after I started to destroy the thread event queue. I haven't thought
through the ramifications yet to see if this is the right thing to do.

Comment 10

[David :Bienvenu]

I think the fix as you have it is fine, Scott. We won't be waiting for the OK to
come back, but we will process it if it does come back in time, which would be
ideal. I'd really like to see this checked in if it fixes 50529 - I'll try it in
my tree and see.

Comment 11

[Scott MacGregor]

Fix checked in.

Comment 12

[Karen Huang]

Change QA contact to me but adding "verifyme" for the keywords since this bug 
may need developer help for verifying this bug, anybody (except mscott) can help 
for verifying this bug? Thanks.

Comment 13

[David :Bienvenu]

verifying.

Comment 14

[Angela Butler]

Added branch accept to status whiteboard

Comment 15

[Angela Butler]

Please check this in on the branch ASAP. Thanks in advance,
.Angela...

Comment 16

[scottputterman]

it's checked in on the branch

Comment 17

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

adding me and putterman to the cc list.

Comment 18

[Karen Huang]

David, is this bug also fix on the 2001-01-04-17-MTEST build as well?

Comment 19

[Scott MacGregor]

I'm still not sure why we wanted to take this for the branch. Angela, can you
help me out here? To my knowledge this bug didn't plague anyone but a few debug
mac builds? No impact to the end consumer. Why are we taking the risk for the
branch in that case? I wasn't planning on checking this in until I heard for
sure but it looks like putterman checked this in on the branch while I was out
of town.

Comment 20

[lchiang]

per selmer, there was some confusion on our parts. Please back out of this fix
on the branch.  Thanks.  Once done, pls remove [branch accept] from the status
whiteboard.

Comment 21

[Scott MacGregor]

backed out and branch accept removed.