286807 - Connections never reclaimed if active while going offline

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

When going offline, the socket transport service is shutdown (by the io service):
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsIOService.cpp#526

this sets mInitialized to PR_FALSE very early:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsSocketTransportService2.cpp#436

Note that this happens before any in-progress sockets get notified of the
shutdown. Now, when they do get notified, in the HTTP case, they eventually try
to reclaim their connection (by virtue of the destructor of nsConnectionHandle).
This happens by posting an event:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp#265

Now, this PostEvent call basically only forwards to the sockettransportservice:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp#161

But alas! The STS fails PostEvent calls when mInitialized is PR_FALSE:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsSocketTransportService2.cpp#123

Now, ReclaimConnection basically does nothing if PostEvent fails.
This means: The connection will never be removed from the active queue! In the
end, this means that the number of dead connections may prevent any further
connections to a certain host.


Note: Shutdown has a similar problem. It races with the IO Service on who gets
the net-teardown topic first:
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsIOService.cpp#678
vs:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpHandler.cpp#1602
which calls:
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/http/src/nsHttpConnectionMgr.cpp#133

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

I now verified that PostEvent does indeed fail for the ReclaimConnection event
when going offline with a transfer active.

In addition, while offline, PostEvent keeps failing for
nsHttpConnectionMgr::OnMsgPruneDeadConnections.

And when shutting down while offline, posting the event for
nsHttpConnectionMgr::OnMsgShutdown fails as well.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

from irc:
<darin> biesi: we should probably have it so that the IO service listens for all
external events and delegates to internal components at the appropriate time
<biesi> darin, it seems to do that for some services
<biesi> DNS, Socket
<darin> biesi: right... and then we need internal interfaces or something for
those Init and Shutdown methods
<biesi> can we make the io service own nsSocketTransportService* etc, and have
GetService return those members?
<timeless> a magical factory could make that happen

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

forgot to paste this:
<darin> biesi: perhaps we should allow PostEvent from the socket transport
thread when we are in shutdown mode
<biesi> darin, maybe the connmgr should call its OnMsg functions directly if on
the sts thread?
<darin> biesi: but that might end up recursing things in a weird way
<darin> biesi: but sure.. we could probably rearrange thigns to make that possible

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: possible patch

how about this? Allows socket detach handlers to post events and processes
them.

Comment 5

[Darin Fisher]

It'll take me a little while to study this patch and its implications.

Comment 6

[Darin Fisher]

Comment on attachment 183313 [details] [diff] [review]
possible patch

>Index: base/src/nsSocketTransportService2.cpp

>+    // Final pass over the event queue
>+    // this makes sure that events posted by socket detach handlers get
>+    // processed
>+    ServiceEventQ();

nit: fix this comment to wrap nicely :)  (vim's gqap is your friend!)


>Index: base/src/nsSocketTransportService2.h

>     //-------------------------------------------------------------------------
>+    // misc (socket thread only)
>+    //-------------------------------------------------------------------------
>+    // pref to indicate whether we are currently in the process of shutting
>+    // down
>+    PRBool      mShuttingDown;

nit: replace "pref to indicate" with "indicates" and put entire comment on a 
single line.  only mAutodialEnabled is a pref ;-)


Otherwise, this patch looks good to me.  I would perhaps add a NS_WARNING if
the PostEvent is rejected when run on the socket thread.  Those should always
be errors in programming logic.

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2, including NS_WARN_IF_FALSE

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 185660 [details] [diff] [review]
patch v2, including NS_WARN_IF_FALSE

fixes a bug where users may not be able to connect to certain sites anymore
after going offline and online again

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in netwerk/base/src/nsSocketTransportService2.cpp;
/cvsroot/mozilla/netwerk/base/src/nsSocketTransportService2.cpp,v  <-- 
nsSocketTransportService2.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in netwerk/base/src/nsSocketTransportService2.h;
/cvsroot/mozilla/netwerk/base/src/nsSocketTransportService2.h,v  <-- 
nsSocketTransportService2.h
new revision: 1.15; previous revision: 1.14
done