1238910 - Rethink/rework neckoxpcom-shutdown/profile-change-net-teardown

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dragana Damjanovic [:dragana]]

So when we are shutting down nsHttpConnectionMng and nsSocketTransportService it would be good to stop opening new connection.

Loooking at:
https://wiki.mozilla.org/XPCOM_Shutdown

On profile-change-net-teardown we shutdown nsHttpConnectionMng and then nsSocketTransportService, but already at this moment it would be good not to do PR_Connect for example.

We do have checks for gIOService->IsOffline() but this is true too late, only on xpcom-shutdown notification when nsHttpConnectionMng and nsSocketTransportService are already shutdown.

This could help with bug 1158189 and 1152046.

Comment 1

[Dragana Damjanovic [:dragana]]

So this is a bit stupid that profile-change-net-teardown is the first notification and we should tear down all network activities in order: tell nsIOService that we are shutting down, shut nsHttpConnectionMng and then shut nsSocketTransportService(which is done by nsIOService)

I thought of moving nsHttpHandler shutdown to be done by nsIOService, but there are one on child process a well :)

Probably a stupid question, but do we need nsConnectionMgr on the child process?

Comment 2

[Patrick McManus [:mcmanus]]

+1 on the overall thing

> Probably a stupid question, but do we need nsConnectionMgr on the child
> process?

basically no - and I believe I added an assert somewhere that we don't do real networking on the child - but I think jason left it in for a reason..

Comment 3

[Patrick McManus [:mcmanus]]

dragana are you planning a patch for this? I think it would be good

Comment 4

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #3)
> dragana are you planning a patch for this? I think it would be good

Yes. My idea is to not initialize nsConnectionMgr on child process and to have it only on parent, of course if we do not need it on the child, so then I can move closing of nsConnectionMgr to the nsIOService. And solution for this problem would look nicer.

If this does not work I will need to signal nsIOService from nsHttpHanndler that we are going to tear down networking - not so nice solution.

Comment 5

[Jason Duell]

The only reason we have a socket transport thread on the child is because we need it to run xpcshell tests (and maybe mochitests?) that use httpd.js.  I know it has something to do with how we inject server-side handlers into httpd.js, but I don't know whether that means we need nsConnectionMgr or not.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1238910_v1.patch

Jason, do we need nsConnectionMgr on the child process?

If we need it I will make different solution, if we do not need it you can review.

Comment 7

[Dragana Damjanovic [:dragana]]

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #5)
> The only reason we have a socket transport thread on the child is because we
> need it to run xpcshell tests (and maybe mochitests?) that use httpd.js.  I
> know it has something to do with how we inject server-side handlers into
> httpd.js, but I don't know whether that means we need nsConnectionMgr or not.

I somehow missed your comment. I will do a try run.

Comment 8

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bbef94c1181

Comment 9

[Dragana Damjanovic [:dragana]]

Attachment: bug_1238910_v1.patch

I have only change commit message here (the patch is the same as the one pushed to try server).

So try server have not complained about not starting nsConnectionMgr on child process, so I am asking for a review.

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8707329 [details] [diff] [review]
bug_1238910_v1.patch

Review of attachment 8707329 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsSocketTransport2.cpp
@@ -1221,5 @@
>      bool isLocal;
>      IsLocal(&isLocal);
>  
> -    if (gIOService->IsShutdown()) {
> -        return NS_ERROR_ABORT;

why the change from abort to offline here?

Comment 11

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #10)
> Comment on attachment 8707329 [details] [diff] [review]
> bug_1238910_v1.patch
> 
> Review of attachment 8707329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/nsSocketTransport2.cpp
> @@ -1221,5 @@
> >      bool isLocal;
> >      IsLocal(&isLocal);
> >  
> > -    if (gIOService->IsShutdown()) {
> > -        return NS_ERROR_ABORT;
> 
> why the change from abort to offline here?

That was my mistake: I thought that profile-change-net-teardown is fired not only during shutdown but in some other cases too. So I did not want to kill local connections for "other" cases. I checked: it is fired only on shutdown so I will change this to ABORT all.

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: bug_1238910_v2.patch

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ae2c475ef7

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a4ae2c475ef7