Closed Bug 1238910 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1152046, 1158189
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?
+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..
dragana are you planning a patch for this? I think it would be good
(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.
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
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.
Attached patch bug_1238910_v1.patch (obsolete) — Splinter Review
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.
Attachment #8707025 - Flags: feedback?(mcmanus)
Attachment #8707025 - Flags: feedback?(jduell.mcbugs)
(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.
Attached patch bug_1238910_v1.patch (obsolete) — Splinter Review
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.
Attachment #8707025 - Attachment is obsolete: true
Attachment #8707025 - Flags: feedback?(mcmanus)
Attachment #8707025 - Flags: feedback?(jduell.mcbugs)
Attachment #8707329 - Flags: review?(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?
Attachment #8707329 - Flags: review?(mcmanus) → review+
(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.
Attachment #8707329 - Attachment is obsolete: true
Attachment #8707842 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a4ae2c475ef7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Depends on: 1242755
You need to log in before you can comment on or make changes to this bug.