mojo: terminate apps if the shell goes away

mojo: terminate apps if the shell goes away

Each application <> shell connection is represented by a ShellImpl instance
on the shell side. This CL makes the Application watch its ShellPtr for pipe
errors so it can Quit() itself if the shell goes away (shell loop destroyed).

mojo_shell_tests starts using a new method to terminate all shell connections
in this CL, and waits afterward until KeepAlive quits the loop signifying that
all apps are gone. This is done prior to destroying the shell MessageLoop so
that the test ensures apps have all died before the next test. In the future
it would be a test failure if this wasn't a no-op, but we have things in the
shell that don't quit themselves right now.

The shell itself won't wait around for apps to die (as of this CL), but
~MessageLoop will still send the message to Application sides that will quit.

** NOTE ** This does not require an app build target to explicitly add magic
*.cc files to their sources.  It requires selecting an appropriate
mojo_application_{chromium, standalone} library akin to mojo_environment_*.
We can possibly combine these two in the future.

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

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

[tim (not reviewing), 2014-07-20 22:12:37]

From a few looks over the ServiceFactory CL, I don't think much of this
conflicts, so I think it can be reviewed. (James as FYI, feel free to review).

Note, this is the base for https://codereview.chromium.org/397733003/

[darin (slow to review), 2014-07-22 03:58:55]

A few comments:

1- I think it would be nice if we could encapsulate the difference between
MessageLoop and RunLoop through mojo_main_{chromium,standalone}.cc somehow, so
you don't have to keep that selection in sync with the
application_impl_{chromium,standalone}.cc selection. One approach would be to
make ApplicationImpl support subclassing or to pass a mojo::Callback<void()>
callback to it that gets run at exit time.

2- Just a nit, but I think the description of the CL should say "apps should
gracefully terminate when the shell goes away." Forceful termination seems like
something the system would do if an app fails to respond to a request to
terminate.

3- Are we sure that having the mojo_shell close the Shell connection to the app
is the best way to tell the app to terminate? It certainly works, but I can also
imagine that we may want a more explicit method on Application. Actually, we may
want more of a Suspend / Resume model, and then just force kill processes when
we are done with them. That said, it seems clean shutdown is important if we are
going to support loading multiple apps into the same process. Hmm...

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

On 2014/07/22 03:58:55, darin wrote:
> A few comments:
> 
> 1- I think it would be nice if we could encapsulate the difference between
> MessageLoop and RunLoop through mojo_main_{chromium,standalone}.cc somehow, so
> you don't have to keep that selection in sync with the
> application_impl_{chromium,standalone}.cc selection. One approach would be to
> make ApplicationImpl support subclassing or to pass a mojo::Callback<void()>
> callback to it that gets run at exit time.

Sorry, to clarify, my point is that we could do away with application_impl_*
if we made ApplicationImpl take a mojo::Callback or function pointer that gets
run when it is time to quit.

[tim (not reviewing), 2014-07-22 17:47:07]

Thanks for the review. In general I agree with much of this; This patch is part
of a work in progress detailed a bit more in my shutdown doc.

On 2014/07/22 03:58:55, darin wrote:
> A few comments:
> 
> 1- I think it would be nice if we could encapsulate the difference between
> MessageLoop and RunLoop through mojo_main_{chromium,standalone}.cc somehow, so
> you don't have to keep that selection in sync with the
> application_impl_{chromium,standalone}.cc selection. One approach would be to
> make ApplicationImpl support subclassing or to pass a mojo::Callback<void()>
> callback to it that gets run at exit time.

A few thoughts here: currently mojo_main_{c,s} is *optionally* linked in to
provide a factory-issued "main" function.  Some apps, chromium based or
standalone, may still write their own mains (network) and in both worlds we need
an applicationimpl. This is one reason I left it separate.  In fact, in an
earlier version of the patch I had Terminate as pure virtual on ApplicationImpl
with the appropriate concrete implementations defined within mojo_main_{c,s}
files.  But I didn't feel like wanting to write your own main should force you
to write your own ApplicationImpl, or conversely, give you a way to opt-out of
being terminated.  Terminate() should be thought of as the lowest possible level
kill switch that is built in to the shell that isn't overrideable by the app.
For these reasons, I didn't really like subclassing.

The callback approach has some of the same downsides -- we can do the right
thing for mojo_main_{c,s}, but if you write your own main you would have the
option of disobeying the order to terminate by providing a no-op callback.  At
least, that's how I saw it... unless your suggestion was different?

> 
> 2- Just a nit, but I think the description of the CL should say "apps should
> gracefully terminate when the shell goes away." Forceful termination seems
like
> something the system would do if an app fails to respond to a request to
> terminate.
> 
> 3- Are we sure that having the mojo_shell close the Shell connection to the
app
> is the best way to tell the app to terminate? It certainly works, but I can
also
> imagine that we may want a more explicit method on Application. Actually, we
may
> want more of a Suspend / Resume model, and then just force kill processes when
> we are done with them. That said, it seems clean shutdown is important if we
are
> going to support loading multiple apps into the same process. Hmm...

I totally agree with these concerns. There are differences between graceful
termination and forceful termination, and we may want an explicit Shell<>App
contract -- I had similar thoughts when scribbling together the Quitting Mojo
Apps doc:
https://docs.google.com/a/chromium.org/document/d/1tKZw3cnnGEuoOD1gYLmPQtqYHN....
I'm working towards that in theory, but starting at a more basic level. See
ApplicationDelegate::Shutdown. 

Adding some treatment of the suspend / resume concept there is totally something
I'd like to get to, but my aim here was to start with the bare necessities:
whether an app like network service chooses to gracefully terminate or not may
or may not be a bug, but the shell needs a way to kill it if for no other reason
than to support testing, to ensure no state leaks across runs of the shell.

[darin (slow to review), 2014-07-24 17:55:40]

LGTM

[tim (not reviewing), 2014-07-24 17:57:51]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-24 17:59:36]

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

[tim (not reviewing), 2014-07-24 19:01:25]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-24 19:04:02]

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

[tim (not reviewing), 2014-07-24 19:47:36]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-07-24 19:48:37]

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

[jamesr, 2014-07-24 19:52:44]

https://codereview.chromium.org/394903005/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_impl_chromium.cc (right):

https://codereview.chromium.org/394903005/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_impl_chromium.cc:11:
base::MessageLoop::current()->Quit();
hmm, in core_window_manager_unittests this blows up with the following stack:

[FATAL:message_loop.cc(314)] Check failed: false. Must be inside Run to call
Quit
#0 0x7f69178bbcee base::debug::StackTrace::StackTrace()
#1 0x7f691794f2e5 logging::LogMessage::~LogMessage()
#2 0x7f6917974bfa base::MessageLoop::QuitWhenIdle()
#3 0x00000043f2c5 base::MessageLoop::Quit()
#4 0x0000006cf1b9 mojo::ApplicationImpl::Terminate()
#5 0x00000068b902 mojo::ApplicationImpl::OnShellError()
#6 0x00000068ae09 mojo::ApplicationImpl::ShellPtrWatcher::OnConnectionError()
#7 0x00000066bee4 mojo::internal::Connector::NotifyError()
#8 0x00000066c3fa mojo::internal::Connector::OnHandleReady()
#9 0x00000066c323 mojo::internal::Connector::CallOnHandleReady()
#10 0x7f69189bda7b mojo::internal::(anonymous namespace)::OnHandleReady()
#11 0x7f69189bde97 base::internal::RunnableAdapter<>::Run()
#12 0x7f69189bddd4 base::internal::InvokeHelper<>::MakeItSo()
#13 0x7f69189bdd2f base::internal::Invoker<>::Run()
#14 0x7f69184f8486 base::Callback<>::Run()
#15 0x7f69184f817a mojo::common::HandleWatcher::State::OnHandleReady()
#16 0x7f69184f82fa
mojo::common::HandleWatcher::State::WillDestroyCurrentMessageLoop()
#17 0x7f6917973eb8 base::MessageLoop::~MessageLoop()
#18 0x00000042135b mojo::WindowManagerApiTest::~WindowManagerApiTest()
#19 0x0000004192e5
mojo::WindowManagerApiTest_FocusAndActivateWindow_Test::~WindowManagerApiTest_FocusAndActivateWindow_Test()
#20 0x000000419309
mojo::WindowManagerApiTest_FocusAndActivateWindow_Test::~WindowManagerApiTest_FocusAndActivateWindow_Test()
#21 0x00000046a06e testing::Test::DeleteSelf_()
#22 0x00000047edd3 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#23 0x000000469b8e testing::internal::HandleExceptionsInMethodIfSupported<>()
#24 0x00000045fd63 testing::TestInfo::Run()
#25 0x00000046030a testing::TestCase::Run()
#26 0x000000464918 testing::internal::UnitTestImpl::RunAllTests()
#27 0x000000476263 testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#28 0x00000046bfee testing::internal::HandleExceptionsInMethodIfSupported<>()
#29 0x0000004645a4 testing::UnitTest::Run()
#30 0x000000435521 RUN_ALL_TESTS()
#31 0x000000434547 base::TestSuite::Run()
#32 0x000000428822 base::internal::RunnableAdapter<>::Run()
#33 0x00000042878f base::internal::InvokeHelper<>::MakeItSo()
#34 0x00000042873a base::internal::Invoker<>::Run()
#35 0x00000042c75e base::Callback<>::Run()
#36 0x000000428d20 base::(anonymous namespace)::LaunchUnitTestsInternal()
#37 0x000000428a17 base::LaunchUnitTests()
#38 0x00000042844c main
#39 0x7f69150ce76d __libc_start_main
#40 0x000000417199 <unknown>

In other words, we're destroying the MessageLoop which signals the error handler
on the pipe to the shell which then attempts to terminate.  Is the shutdown
order wrong here?  I think the order is supposed to be:

1.) ~MessageLoop to trigger connection errors on all outstanding pipes and run
their teardown code with the application still alive (note: order here is not
well defined)
2.) ~ApplicationDelegate to tear down the application itself
3.) ~Thread to run TLS destructors with library still loaded
4.) unload library

If one of the error handlers tries to quit the message loop, however, it'll blow
up in step 1.

Maybe this test harness is doing something wrong?

[tim (not reviewing), 2014-07-24 20:31:25]

The CQ bit was unchecked by tim@chromium.org

[tim (not reviewing), 2014-07-28 16:43:31]

Please take another look, this is now based on
https://codereview.chromium.org/420143003/. 
https://docs.google.com/a/chromium.org/document/d/1tKZw3cnnGEuoOD1gYLmPQtqYHN...
has some context.

[darin (slow to review), 2014-07-28 16:49:59]

https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
File mojo/public/interfaces/service_provider/service_provider.mojom (right):

https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
mojo/public/interfaces/service_provider/service_provider.mojom:30: Terminate();
I'm not sure this is the API we want for application lifetime. I'm not sure we
want any explicit "you should shutdown" event. We may be better served only
providing "suspend/resume" events. If suspended, you might be killed. However, I
guess the issue is that we need some kind of explicit "now you are killed" event
to handle the case of single process mode or multiple apps loaded into the same
process? Hmm...

[tim (not reviewing), 2014-07-28 16:55:17]

https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
File mojo/public/interfaces/service_provider/service_provider.mojom (right):

https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
mojo/public/interfaces/service_provider/service_provider.mojom:30: Terminate();
On 2014/07/28 16:49:58, darin wrote:
> I'm not sure this is the API we want for application lifetime. I'm not sure we
> want any explicit "you should shutdown" event. We may be better served only
> providing "suspend/resume" events. If suspended, you might be killed. However,
I
> guess the issue is that we need some kind of explicit "now you are killed"
event
> to handle the case of single process mode or multiple apps loaded into the
same
> process? Hmm...

If the primordial app exits and we want to quit the shell, we'll want to kill
other apps...

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

On 2014/07/28 16:55:17, timsteele wrote:
>
https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
> File mojo/public/interfaces/service_provider/service_provider.mojom (right):
> 
>
https://codereview.chromium.org/394903005/diff/120001/mojo/public/interfaces/...
> mojo/public/interfaces/service_provider/service_provider.mojom:30:
Terminate();
> On 2014/07/28 16:49:58, darin wrote:
> > I'm not sure this is the API we want for application lifetime. I'm not sure
we
> > want any explicit "you should shutdown" event. We may be better served only
> > providing "suspend/resume" events. If suspended, you might be killed.
However,
> I
> > guess the issue is that we need some kind of explicit "now you are killed"
> event
> > to handle the case of single process mode or multiple apps loaded into the
> same
> > process? Hmm...
> 
> If the primordial app exits and we want to quit the shell, we'll want to kill
> other apps...

Note that I'm not saying we won't want something more sophisticated eventually
(I touched on this in the doc), but I was starting with the basics so I can make
incremental progress and get some tests passing.  If you strongly prefer hiding
this from the mojom (as in patch set 4) I can work on going back, but it's a bit
trickier than what I had before (or this patch).

[tim (not reviewing), 2014-07-31 18:08:12]

OK, I feel this is ready for another look. It is based on
https://codereview.chromium.org/437493002/, where I added
BackgroundShellServiceLoader to replace BackgroundServiceLoader, so that shell
bundled apps (important for android for the near term) can still Quit() their
MessageLoops just like normal apps.

[tim (not reviewing), 2014-08-05 15:44:32]

On 2014/07/31 18:08:12, timsteele wrote:
> OK, I feel this is ready for another look. It is based on
> https://codereview.chromium.org/437493002/, where I added
> BackgroundShellServiceLoader to replace BackgroundServiceLoader, so that shell
> bundled apps (important for android for the near term) can still Quit() their
> MessageLoops just like normal apps.

ping if anyone *isn't* on vacation :)

[Aaron Boodman, 2014-08-05 16:59:05]

I'll take a look in an hour if somebody hasn't beaten me to it.

[Aaron Boodman, 2014-08-05 18:30:09]

Sorry, actually -- after lunch.


On Tue, Aug 5, 2014 at 9:59 AM, <aa@chromium.org> wrote:

> I'll take a look in an hour if somebody hasn't beaten me to it.
>
> https://codereview.chromium.org/394903005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-08-05 20:00:21]

LGTM, I figure if the other guys still want you to change something you can do
it async.

[tim (not reviewing), 2014-08-05 20:13:35]

On 2014/08/05 20:00:21, Aaron Boodman wrote:
> LGTM, I figure if the other guys still want you to change something you can do
> it async.

Deal. Thanks so much for taking a look!

[tim (not reviewing), 2014-08-05 20:30:20]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-05 20:32:28]

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

[tim (not reviewing), 2014-08-05 23:53:24]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-05 23:56:32]

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

[commit-bot: I haz the power, 2014-08-06 03:50:12]

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

[commit-bot: I haz the power, 2014-08-06 05:35:26]

Change committed as 287680

[tim (not reviewing), 2014-08-06 19:10:54]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-06 19:12:45]

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

[tim (not reviewing), 2014-08-06 19:16:51]

The CQ bit was unchecked by tim@chromium.org

[tim (not reviewing), 2014-08-06 19:17:54]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-08-06 19:19:56]

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

[commit-bot: I haz the power, 2014-08-06 23:31:09]

Change committed as 287894