63565 - Socket transport service does not cancel sockets before shutting down.

Status: VERIFIED FIXED

Description

[Doug Turner (:dougt)]

When the socket transport thread goes away, it should cancel all active 
transports.  If it does not, they will leak.

Comment 1

[Doug Turner (:dougt)]

Two problem cause this.  

First, any events that are create and posted during xpcom shutdown, specifically 
during xpcom service shutdown, are lost.  This is terrible for necko since we 
create async messages which AddRef various nsIChannel's.  If these messages are 
not handled, we leak references as well as we do not complete any pending 
transactions.

The second problem is more obvious.  First we need to cancel any transport that 
is in our active transport list durning shutdown.  In the ~() of the service, I 
also release anything that is still in the active list.

We also have to make sure that the the socket transport posting thread does not 
go away by holding a reference to the event queue service.  Without holding this 
reference, I see the event chain get whacked.


One other unrelated changes was to create the string bundle via a thread proxy.  
This was so that if the transport service was ever created on a non-ui thread, 
the string bundle stuff wouldn't bitch.

patches coming soon.

Comment 2

[Doug Turner (:dougt)]

Created attachment 21616 [details] [diff] [review]
xpcom patch

Comment 3

[Doug Turner (:dougt)]

Created attachment 21617 [details] [diff] [review]
socket patch

Comment 4

[Doug Turner (:dougt)]

Darin, could you review the socket patch.  (I didn't mess around with the 
ordering of suspend and resume.  I will leave that for another day/bug)

Chris-da-man, could you review that I did not bodyslam xpcom too hard.

Comment 5

[Chris Waterson]

r=waterson for xpcom patch.

Comment 6

[Doug Turner (:dougt)]

The socket patch is not entirely correct.  I should be releasing the transports 
during the Shutdown method in the service, not the destructor.  If I do not 
release them in the Shutdown, we will get cycles.  Darin, please review the next 
patch.  

Comment 7

[Doug Turner (:dougt)]

Created attachment 21672 [details] [diff] [review]
socket patch 2

Comment 8

[rpotts (gone)]

hey doug,

I think that the patch looks good...

The only part that I don't understand is why you need to hold onto 
mEventQueueService ? It looks like this is the eventQ of the UI thread...  What 
is the point of defering the destruction of the UI eventQ until after the 
SocketTransportService has been destroyed?

-- rick

Comment 9

[Doug Turner (:dougt)]

Hey rick, thanks for your comments.  

the point is that during the destruction of the socket transport service, we may 
destory (via cancel()) active transports.  (FTP caches control connections.  The 
logic if very simple now.  Hold onto the connection forever.  This will 
change, but for now it will do...  Also, note that when http pipelining is 
enabled (and works :-), they may cache transport in a similar fashion.)  Now, 
when we Shutdown() the STS, any remaining transport we will cancel.  They may 
fire async notification on to the UI thread.  If we do not hold onto the event 
service during the the destruction of the sts, the UI event queue will be marked 
as not-accepting.  (hence the assert without caching the event queue service).  
The notifications holds a references on the transport plus the event could not 
be posted successfully, so we will leak.

Comment 10

[Darin Fisher]

r=darin for the nsSocketTransportService patch.

Comment 11

[Scott MacGregor]

Doug your socket changes look good to me. Just for kicks, have you tried them
out with imap? Like ftp, we also do a lot of connection caching. Just want to
make sure this doesn't rock the imap boat any.

pending that, sr=mscott

Comment 12

[Doug Turner (:dougt)]

thanks. 

no problems with imap that I could see.  I sent and received mail without a
problem.  I would only expect shutdown problems (e.g. crash on shutdown), but
with the last socket patch, I have not seen any.  

Comment 13

[Doug Turner (:dougt)]

fixed checked.  The socket patch was missing a check for null on line 622.  I
added that too.

Comment 14

[Tom Everingham]

verified per engineer's comments