mojo: make NetworkServiceImpl clean up after itself

mojo: make NetworkServiceImpl clean up after itself

Each URLLoaderImpl created by NetworkServiceImpl can allocate a net::URLRequest.
When shutting down the app, destruction of the NetworkContext and the
URLRequestContext within it expects that all net::URLRequest resources have been
freed by now (it CHECKs). NetworkService is an app like any other and can't make
any assumptions about when it is being destroyed relative to other apps in the
system that may have been using it, so the safest thing to do is to have it
gracefully tear-down in-progress URLLoaders.

Note: the only reason we're not seeing the CHECK happen is because things
shut down (the process terminates) before we even get to ~URLRequestContext.
I'll be repairing shutdown in an upcoming CL.

BUG=394477
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284777

[tim (not reviewing), 2014-07-18 17:33:07]

Darin, ptal?  I know it's not the prettiest thing, but the current code is being
a little too loose for URLRequestContext's liking.  Ensuring that every other
customer of NetworkService in the system has cleaned up their URLLoaderPtrs by
the time a NetworkService is being destroyed isn't really feasible now and I
suspect it will only become less so in the future, when apps might be killed for
a variety of reasons.  So I thought it made sense to have NSI be a good citizen.

If there's another way you prefer, let me know.

[darin (slow to review), 2014-07-18 18:20:00]

Hmm, another approach is to make the Delegate in main.cc (or at least the
NetworkContext) outlive the MessageLoopForIO. When the message loop is
destroyed, it will interrupt any HandleWatcher, which will cause all of the
Connector objects to observe an error on the pipe. That should trigger the
destruction of all URLLoaderImpls.

[tim (not reviewing), 2014-07-18 20:06:41]

On 2014/07/18 18:20:00, darin wrote:
> Hmm, another approach is to make the Delegate in main.cc (or at least the
> NetworkContext) outlive the MessageLoopForIO. When the message loop is
> destroyed, it will interrupt any HandleWatcher, which will cause all of the
> Connector objects to observe an error on the pipe. That should trigger the
> destruction of all URLLoaderImpls.

Ah, well that certainly cleans things up, tiny patch now.
I added a comment addressing the subtlety.

[tim (not reviewing), 2014-07-18 22:11:07]

On 2014/07/18 20:06:41, timsteele wrote:
> On 2014/07/18 18:20:00, darin wrote:
> > Hmm, another approach is to make the Delegate in main.cc (or at least the
> > NetworkContext) outlive the MessageLoopForIO. When the message loop is
> > destroyed, it will interrupt any HandleWatcher, which will cause all of the
> > Connector objects to observe an error on the pipe. That should trigger the
> > destruction of all URLLoaderImpls.
> 
> Ah, well that certainly cleans things up, tiny patch now.
> I added a comment addressing the subtlety.

Hm. It turns out with or without this CL, in a static build, if you actually
keep the process around long enough to let InProcessDynamicServiceRunner
dlclose() libmojo_network_service, we segfault:

#1   __nptl_deallocate_tsd () at pthread_create.c:156
#2  start_thread (arg=0x7fffedf5f700) at pthread_create.c:315
#3  clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112

Not sure why yet (although crbug.com/25245 is interesting). As I said, it
happens without this CL (so I think it still makes sense to land).

[tim (not reviewing), 2014-07-19 01:05:39]

On 2014/07/18 22:11:07, timsteele wrote:
> On 2014/07/18 20:06:41, timsteele wrote:
> > On 2014/07/18 18:20:00, darin wrote:
> > > Hmm, another approach is to make the Delegate in main.cc (or at least the
> > > NetworkContext) outlive the MessageLoopForIO. When the message loop is
> > > destroyed, it will interrupt any HandleWatcher, which will cause all of
the
> > > Connector objects to observe an error on the pipe. That should trigger the
> > > destruction of all URLLoaderImpls.
> > 
> > Ah, well that certainly cleans things up, tiny patch now.
> > I added a comment addressing the subtlety.
> 
> Hm. It turns out with or without this CL, in a static build, if you actually
> keep the process around long enough to let InProcessDynamicServiceRunner
> dlclose() libmojo_network_service, we segfault:
> 
> #1   __nptl_deallocate_tsd () at pthread_create.c:156
> #2  start_thread (arg=0x7fffedf5f700) at pthread_create.c:315
> #3  clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:112
> 
> Not sure why yet (although crbug.com/25245 is interesting). As I said, it
> happens without this CL (so I think it still makes sense to land).

I guess this isn't so surprising since we know static builds of chromium-based
apps aren't supported. D'oh. 

What's happening here is
* trying to load a service via NetworkLoader results in loading
libmojo_network_service.so with its own copy of base
* binding the shell_handle in the network service app spins up a HandleWatcher
thread which registers some TLS data
* the destructor for that data points to a function in copy 2 of base::
* network service finishes up
* dlclose() libmojo_network_service (and copy 2 of base)
* thread exits, kaboom

I guess one interesting thing is that everything else actually "works" for the
time being. If we were to treat the network lib like a bad plugin and didn't
bother dlclosing, we could run the shell_tests in both builds. This isn't a
viable long term plan obviously, but since network service is actually part of
the shell it'd be nice to test it.

Anyway, none of this is so relevant to this CL, more for
https://codereview.chromium.org/397733003/ (as you hinted at before).

So let me know if this patch looks good otherwise!

[darin (slow to review), 2014-07-19 03:54:44]

Upon reflection, I wonder if it is really safe that the NetworkContext is OK
being destroyed after the MessageLoop is destroyed. Also, this "fix" is not an
option for folks who link against mojo_main_{chromium,standalone}.cc.

Another solution would have been to make each URLLoaderImpl take an owning
reference to the NetworkContext. We could make it ref-counted. Then, it wouldn't
matter if you dropped the Delegate's reference to the NetworkContext prior to
the MessageLoop being destroyed. Hmm, I know we generally try to avoid reference
counting, but this kind of feels like a good fit for reference counting. N guys
need the NetworkContext to outlive them. The reference count is literally N.

https://codereview.chromium.org/407593002/diff/40001/mojo/services/network/ma...
File mojo/services/network/main.cc (right):

https://codereview.chromium.org/407593002/diff/40001/mojo/services/network/ma...
mojo/services/network/main.cc:75: Delegate delegate;
I might call out the scoping a bit more explicitly for readability sake:

  Delegate delegate;
  {
    // ...
    base::MessageLoopForIO loop;
    ...
    loop.Run();
  }

[tim (not reviewing), 2014-07-19 22:55:52]

True about this not being an option for the standard SDK MojoMains. Similarly I
originally felt this was NetworkServiceImpls fault... 

We could do the ref-counting, but is that really preferable to what I had in the
original patch? I like the deterministic nature of iterating over the
URLLoaderImpls we still have out there at shutdown and cleaning them up.

In general I prefer being able to fully control what happens at shutdown and
reserve ref counting for cases where it isn't easy / possible to do that.  That
said, the only reason we can do this now is because it's relatively easy to get
a hold of *all* the NetworkServiceImpls that have been created thanks to the
tracking done by ServiceConnector.  I think James' CL may make that part a bit
trickier. Your ref-counting proposal is probably robust enough to withstand
that.. so maybe I will give it a shot.

[tim (not reviewing), 2014-07-20 21:48:34]

On 2014/07/19 22:55:52, timsteele wrote:
> True about this not being an option for the standard SDK MojoMains. Similarly
I
> originally felt this was NetworkServiceImpls fault... 
> 
> We could do the ref-counting, but is that really preferable to what I had in
the
> original patch? I like the deterministic nature of iterating over the
> URLLoaderImpls we still have out there at shutdown and cleaning them up.
> 
> In general I prefer being able to fully control what happens at shutdown and
> reserve ref counting for cases where it isn't easy / possible to do that. 
That
> said, the only reason we can do this now is because it's relatively easy to
get
> a hold of *all* the NetworkServiceImpls that have been created thanks to the
> tracking done by ServiceConnector.  I think James' CL may make that part a bit
> trickier. Your ref-counting proposal is probably robust enough to withstand
> that.. so maybe I will give it a shot.

Actually, thinking about this some more I'm still leaning towards the approach I
took in patch set 1.  If the context is ref counted, then ownership is logically
bound to the pipes connecting the URLLoaderImpls to the InterfacePtrs off in
some foreign app. NetworkService is manually vending those Impls, meaning they
aren't tracked by the shell and the only reference to them is from that foreign
app. That means NetworkService could terminate (thread exits, lib unloaded)
without cleaning up the resources it allocated, and the URLRequestContext memory
might outlive the library entirely, which is bad.

I'm hoping this will be compatible with James' CL because we're still going to
connect the NetworkServiceImpl via ServiceManager and so they'll be tracked.

WDYT?

[darin (slow to review), 2014-07-22 04:06:12]

LGTM for the current patch. I would just add some scoping brackets to make the
lifetime stand out more.

[tim (not reviewing), 2014-07-22 18:02:38]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-22 18:03:24]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tim@chromium.org/407593002/60001

[commit-bot: I haz the power, 2014-07-22 21:23:29]

Change committed as 284777