Socket transport service does not cancel sockets before shutting down.




18 years ago
18 years ago


(Reporter: dougt, Assigned: dougt)


Windows 2000

Firefox Tracking Flags

(Not tracked)



(3 attachments)



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

Comment 1

18 years ago
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 

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

18 years ago
Created attachment 21616 [details] [diff] [review]
xpcom patch

Comment 3

18 years ago
Created attachment 21617 [details] [diff] [review]
socket patch

Comment 4

18 years ago
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

18 years ago
r=waterson for xpcom patch.

Comment 6

18 years ago
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 

Comment 7

18 years ago
Created attachment 21672 [details] [diff] [review]
socket patch 2

Comment 8

18 years ago
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

18 years ago
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

18 years ago
r=darin for the nsSocketTransportService patch.

Comment 11

18 years ago
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

18 years ago

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

18 years ago
fixed checked.  The socket patch was missing a check for null on line 622.  I
added that too.
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 14

18 years ago
verified per engineer's comments
You need to log in before you can comment on or make changes to this bug.