mojo: kill app_thread before unloading main app library and add tests

mojo: kill app_thread before unloading app libraries and add tests

* Depends on https://codereview.chromium.org/394903005/ which makes the shell kill app threads on exit.
* Adds tests that verify basic functionality and termination of NetworkService by the shell.

BUG=378155, 394477
R=darin@chromium.org

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

[darin (slow to review), 2014-07-15 22:15:15]

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
File mojo/shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
mojo/shell/shell_test_base_unittest.cc:140:
service.Bind(ConnectToServiceViaNetwork(
does this rely on a web server running or is it not really connecting via the
network?  (dynamic_service_loader.cc avoids using the network service for
loading modules off the disk.)

[tim (not reviewing), 2014-07-15 22:23:56]

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
File mojo/shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
mojo/shell/shell_test_base_unittest.cc:140:
service.Bind(ConnectToServiceViaNetwork(
On 2014/07/15 22:15:15, darin wrote:
> does this rely on a web server running or is it not really connecting via the
> network?  (dynamic_service_loader.cc avoids using the network service for
> loading modules off the disk.)

ConnectToServiceViaNetwork configures the MojoURLResolver's origin to be that of
an (embedded) http server serving files from the output directory. So the URL
that the DynamicServiceLoader sees winds up being (e.g)
http://127.0.0.1:8011/libmojo_test_app.so, so it does route this to the
NetworkService and to the EmbeddedTestServer instance.  Seems to work!

[darin (slow to review), 2014-07-15 22:44:23]

On 2014/07/15 22:23:56, timsteele wrote:
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> File mojo/shell/shell_test_base_unittest.cc (right):
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> mojo/shell/shell_test_base_unittest.cc:140:
> service.Bind(ConnectToServiceViaNetwork(
> On 2014/07/15 22:15:15, darin wrote:
> > does this rely on a web server running or is it not really connecting via
the
> > network?  (dynamic_service_loader.cc avoids using the network service for
> > loading modules off the disk.)
> 
> ConnectToServiceViaNetwork configures the MojoURLResolver's origin to be that
of
> an (embedded) http server serving files from the output directory. So the URL
> that the DynamicServiceLoader sees winds up being (e.g)
> http://127.0.0.1:8011/libmojo_test_app.so, so it does route this to the
> NetworkService and to the EmbeddedTestServer instance.  Seems to work!

I see. Maybe this only works when you use the component="shared_library" build?
If shell_test_base.o is linked against a different copy of libnet.a than
libmojo_network_service.so, then this won't work, right?

[viettrungluu, 2014-07-15 22:44:54]

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.cc
File mojo/shell/mojo_url_resolver.cc (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
mojo/shell/mojo_url_resolver.cc:41: void MojoURLResolver::SetOrigin(const
std::string& origin) {
I wonder if "origin" is not the appropriate term. Maybe "BaseURL" would be
better.

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.h
File mojo/shell/mojo_url_resolver.h (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
mojo/shell/mojo_url_resolver.h:27: void SetOrigin(const std::string& origin);
I wonder if this shouldn't take a GURL as an argument.

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base.cc
File mojo/shell/shell_test_base.cc (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base.c...
mojo/shell/shell_test_base.cc:28: test_server_.reset(new
net::test_server::EmbeddedTestServer());
Do we really want to do this for all tests, even ones which don't hit the
network?

[darin (slow to review), 2014-07-15 22:46:35]

On 2014/07/15 22:44:23, darin wrote:
> On 2014/07/15 22:23:56, timsteele wrote:
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > File mojo/shell/shell_test_base_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > mojo/shell/shell_test_base_unittest.cc:140:
> > service.Bind(ConnectToServiceViaNetwork(
> > On 2014/07/15 22:15:15, darin wrote:
> > > does this rely on a web server running or is it not really connecting via
> the
> > > network?  (dynamic_service_loader.cc avoids using the network service for
> > > loading modules off the disk.)
> > 
> > ConnectToServiceViaNetwork configures the MojoURLResolver's origin to be
that
> of
> > an (embedded) http server serving files from the output directory. So the
URL
> > that the DynamicServiceLoader sees winds up being (e.g)
> > http://127.0.0.1:8011/libmojo_test_app.so, so it does route this to the
> > NetworkService and to the EmbeddedTestServer instance.  Seems to work!
> 
> I see. Maybe this only works when you use the component="shared_library"
build?
> If shell_test_base.o is linked against a different copy of libnet.a than
> libmojo_network_service.so, then this won't work, right?

Or, maybe not. I see this really does open up a listening socket and implement
a basic web server. Wow, okay... seems good then!

[darin (slow to review), 2014-07-15 22:50:13]

On 2014/07/15 22:44:54, viettrungluu wrote:
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.cc
> File mojo/shell/mojo_url_resolver.cc (right):
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
> mojo/shell/mojo_url_resolver.cc:41: void MojoURLResolver::SetOrigin(const
> std::string& origin) {
> I wonder if "origin" is not the appropriate term. Maybe "BaseURL" would be
> better.
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.h
> File mojo/shell/mojo_url_resolver.h (right):
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
> mojo/shell/mojo_url_resolver.h:27: void SetOrigin(const std::string& origin);
> I wonder if this shouldn't take a GURL as an argument.

Yes and yes. It should be SetBaseURL not SetOrigin.


> 
> https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base.cc
> File mojo/shell/shell_test_base.cc (right):
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base.c...
> mojo/shell/shell_test_base.cc:28: test_server_.reset(new
> net::test_server::EmbeddedTestServer());
> Do we really want to do this for all tests, even ones which don't hit the
> network?

Maybe it is not that expensive to setup given that it runs in-process.

[tim (not reviewing), 2014-07-15 23:01:29]

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.h
File mojo/shell/mojo_url_resolver.h (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
mojo/shell/mojo_url_resolver.h:27: void SetOrigin(const std::string& origin);
On 2014/07/15 22:44:53, viettrungluu wrote:
> I wonder if this shouldn't take a GURL as an argument.

Heh, do either of you know a handy way to force a trailing slash on a GURL
(aside from GURL::Replacements)? I'd like to have the base_url_ behavior be
consistent when it is set so we don't have to think about it later in Resolve
(it's also important because gurl.Resolve truncates to the last slash for file
URLs). I kept it to a string because this was trivial, but I agree a GURL would
be nice.

[darin (slow to review), 2014-07-15 23:15:03]

On 2014/07/15 23:01:29, timsteele wrote:
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.h
> File mojo/shell/mojo_url_resolver.h (right):
> 
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
> mojo/shell/mojo_url_resolver.h:27: void SetOrigin(const std::string& origin);
> On 2014/07/15 22:44:53, viettrungluu wrote:
> > I wonder if this shouldn't take a GURL as an argument.
> 
> Heh, do either of you know a handy way to force a trailing slash on a GURL
> (aside from GURL::Replacements)? I'd like to have the base_url_ behavior be
> consistent when it is set so we don't have to think about it later in Resolve
> (it's also important because gurl.Resolve truncates to the last slash for file
> URLs). I kept it to a string because this was trivial, but I agree a GURL
would
> be nice.

That's a good brettw@ question.

[tim (not reviewing), 2014-07-16 00:11:30]

On 2014/07/15 22:46:35, darin wrote:
> On 2014/07/15 22:44:23, darin wrote:
> > On 2014/07/15 22:23:56, timsteele wrote:
> > >
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > > File mojo/shell/shell_test_base_unittest.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > > mojo/shell/shell_test_base_unittest.cc:140:
> > > service.Bind(ConnectToServiceViaNetwork(
> > > On 2014/07/15 22:15:15, darin wrote:
> > > > does this rely on a web server running or is it not really connecting
via
> > the
> > > > network?  (dynamic_service_loader.cc avoids using the network service
for
> > > > loading modules off the disk.)
> > > 
> > > ConnectToServiceViaNetwork configures the MojoURLResolver's origin to be
> that
> > of
> > > an (embedded) http server serving files from the output directory. So the
> URL
> > > that the DynamicServiceLoader sees winds up being (e.g)
> > > http://127.0.0.1:8011/libmojo_test_app.so, so it does route this to the
> > > NetworkService and to the EmbeddedTestServer instance.  Seems to work!
> > 
> > I see. Maybe this only works when you use the component="shared_library"
> build?
> > If shell_test_base.o is linked against a different copy of libnet.a than
> > libmojo_network_service.so, then this won't work, right?
> 
> Or, maybe not. I see this really does open up a listening socket and implement
> a basic web server. Wow, okay... seems good then!

I think I am running afoul of crbug.com/392685 though? When we use the
NetworkService to load the app, the DynamicServiceLoader holds an InterfacePtr
to a NetworkService. I thought this was OK since we'll reset the shell Context
between tests, and I assumed that would kill off whatever the ServiceLoader was
holding on to?

But it appears the MojoMain for network service never terminates even though
test fixture is destroyed.... So the next test winds up re-entering
network/main.cc's MojoMain and allocating another AtExitManager (etc) and bad
things happen.  The tests run fine when run one-by-one.

I'm a bit puzzled, it's not obvious to me what needs to happen to clear that
between tests... looking.

[tim (not reviewing), 2014-07-16 00:54:18]

On 2014/07/16 00:11:30, timsteele wrote:
> On 2014/07/15 22:46:35, darin wrote:
> > On 2014/07/15 22:44:23, darin wrote:
> > > On 2014/07/15 22:23:56, timsteele wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > > > File mojo/shell/shell_test_base_unittest.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/397733003/diff/1/mojo/shell/shell_test_base_u...
> > > > mojo/shell/shell_test_base_unittest.cc:140:
> > > > service.Bind(ConnectToServiceViaNetwork(
> > > > On 2014/07/15 22:15:15, darin wrote:
> > > > > does this rely on a web server running or is it not really connecting
> via
> > > the
> > > > > network?  (dynamic_service_loader.cc avoids using the network service
> for
> > > > > loading modules off the disk.)
> > > > 
> > > > ConnectToServiceViaNetwork configures the MojoURLResolver's origin to be
> > that
> > > of
> > > > an (embedded) http server serving files from the output directory. So
the
> > URL
> > > > that the DynamicServiceLoader sees winds up being (e.g)
> > > > http://127.0.0.1:8011/libmojo_test_app.so, so it does route this to the
> > > > NetworkService and to the EmbeddedTestServer instance.  Seems to work!
> > > 
> > > I see. Maybe this only works when you use the component="shared_library"
> > build?
> > > If shell_test_base.o is linked against a different copy of libnet.a than
> > > libmojo_network_service.so, then this won't work, right?
> > 
> > Or, maybe not. I see this really does open up a listening socket and
implement
> > a basic web server. Wow, okay... seems good then!
> 
> I think I am running afoul of crbug.com/392685 though? When we use the
> NetworkService to load the app, the DynamicServiceLoader holds an InterfacePtr
> to a NetworkService. I thought this was OK since we'll reset the shell Context
> between tests, and I assumed that would kill off whatever the ServiceLoader
was
> holding on to?
> 
> But it appears the MojoMain for network service never terminates even though
> test fixture is destroyed.... So the next test winds up re-entering
> network/main.cc's MojoMain and allocating another AtExitManager (etc) and bad
> things happen.  The tests run fine when run one-by-one.
> 
> I'm a bit puzzled, it's not obvious to me what needs to happen to clear that
> between tests... looking.

Hm, it isn't 392685 per se.  The problem is just that nothing stops the MojoMain
MessageLoop in NetworkService's case.
I'm not sure what should terminate it either. The shell going away
(~DynamicServiceLoader) might make sense as a trigger, but plumbing it properly
is tricky. Perhaps Application (the mojom declaration) could have a method to
handle system termination implemented by ApplicationImpl to quit the message
loop? 

Is there prior thinking here?

[tim (not reviewing), 2014-07-16 00:54:47]

+davemoore, please see my last comment.

[DaveMoore, 2014-07-16 18:13:47]

On 2014/07/16 00:54:47, timsteele wrote:
> +davemoore, please see my last comment.

We've given no thought to having a formal way to quit an app...we need to.

[tim (not reviewing), 2014-07-19 02:11:37]

I addressed comments and built this on top of a prototype CL for app termination
on shell exit (https://codereview.chromium.org/394903005/). 

I also added an egregious hack to get this running in both static and shared lib
builds. I'm not that proud of it but I did want to get to a point where "it
worked", so I could take a step back and look at it... 

James' changes will likely impact things so reviewing isn't too crucial until
that lands, but feel free to give feedback.

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.cc
File mojo/shell/mojo_url_resolver.cc (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
mojo/shell/mojo_url_resolver.cc:41: void MojoURLResolver::SetOrigin(const
std::string& origin) {
On 2014/07/15 22:44:53, viettrungluu wrote:
> I wonder if "origin" is not the appropriate term. Maybe "BaseURL" would be
> better.

Done.

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver.h
File mojo/shell/mojo_url_resolver.h (right):

https://codereview.chromium.org/397733003/diff/1/mojo/shell/mojo_url_resolver...
mojo/shell/mojo_url_resolver.h:27: void SetOrigin(const std::string& origin);
On 2014/07/15 22:44:53, viettrungluu wrote:
> I wonder if this shouldn't take a GURL as an argument.

Done.

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/shell_test_ba...
File mojo/shell/shell_test_base_unittest.cc (right):

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/shell_test_ba...
mojo/shell/shell_test_base_unittest.cc:154: shell_context()->Shutdown();
Considering moving these two lines to ShellTestBase::TearDown in an
if(connect_to_service_via_network_was_called_)... although it's nice to see it
here as part of the test..

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

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:95: // TODO(tim): Remove this
ugly hack.
nit: can you explain what this ugly hack is achieving? it is not obvious.

[tim (not reviewing), 2014-07-19 21:59:43]

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/in_process_dy...
File mojo/shell/in_process_dynamic_service_runner.cc (right):

https://codereview.chromium.org/397733003/diff/20001/mojo/shell/in_process_dy...
mojo/shell/in_process_dynamic_service_runner.cc:95: // TODO(tim): Remove this
ugly hack.
On 2014/07/19 21:18:00, darin wrote:
> nit: can you explain what this ugly hack is achieving? it is not obvious.

Sorry about that. What was happening was:

What's happening here is
* chromium-based apps like network_service have their own copy of base
* binding the shell_handle in MojoMains spins up a HandleWatcher
thread which registers some TLS data
* the destructor for that data points to a function in copy 2 of base::
* the app finishes up
* dlclose() the DSO (bye bye copy 2 of base)
* thread exits, kaboom

For shared library builds this was actually working because the pointer to the
static base:: function is still mapped. For static builds, it's gone.  If we do
*not* Reset the library here and let the thread terminate first, all is good.

I called it an "ugly hack" because of the #ifdef COMPONENT_BUILD... but as
discussed offline perhaps this is what we should do in all cases. I can see if I
can clean things up with that in mind.

[tim (not reviewing), 2014-07-19 22:40:10]

removed "hack".

[qsr, 2014-07-21 08:09:20]

https://codereview.chromium.org/397733003/diff/60001/mojo/shell/context.cc
File mojo/shell/context.cc (right):

https://codereview.chromium.org/397733003/diff/60001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:164: GURL("mojo:profile_service"));
This is probably outdated, the profile_service does not exist anymore.

[tim (not reviewing), 2014-07-21 16:47:01]

https://codereview.chromium.org/397733003/diff/60001/mojo/shell/context.cc
File mojo/shell/context.cc (right):

https://codereview.chromium.org/397733003/diff/60001/mojo/shell/context.cc#ne...
mojo/shell/context.cc:164: GURL("mojo:profile_service"));
On 2014/07/21 08:09:20, qsr wrote:
> This is probably outdated, the profile_service does not exist anymore.

Oops, bad merge. Thanks!

[darin (slow to review), 2014-07-22 04:10:00]

LGTM

[tim (not reviewing), 2014-07-24 16:52:38]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-24 16:53:52]

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

[tim (not reviewing), 2014-07-24 18:44:43]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-24 18:46:21]

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

[commit-bot: I haz the power, 2014-07-24 22:33:11]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

[tim (not reviewing), 2014-07-24 22:37:31]

On 2014/07/24 22:33:11, I haz the power (commit-bot) wrote:
> FYI, CQ is re-trying this CL (attempt #1).
> The failing builders are:
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
>   linux_chromium_rel_swarming on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)

gn_unittests is a known issue: https://codereview.chromium.org/412263004/
everything else looks green, so may dcommit.

[tim (not reviewing), 2014-07-24 22:38:56]

Committed patchset #6 manually as r285396 (presubmit successful).