Bring skydebugger closer to clean-shutdown

Bring skydebugger closer to clean-shutdown

This teaches the SkyDebugger prompt how to tell
the sky debugger (server) to shut down instead of
just calling exit(0).

This also teaches the WindowManagerApp (server) how
to tear down all of its connections itself instead
of depending on the pipes to do so (which would
crash when youd delete the WindowManagerApp as the
pipes could outlive it with WindowManagerImpl objects
containing raw pointers back to the WindowManagerApp).

Shutdown is not yet clean.  It errors out trying to
talk to the X11 server, but it's closer to clean
than it was prior to this change.  I may add back
and exit(0) to side-step shutdown until it can be
made fully clean.

R=jamesr@chromium.org, sky@chromium.org
BUG=430291, 430242
Committed: https://chromium.googlesource.com/external/mojo/+/3f1157b76bb4ed7a42344bb236ab2314d261d973

[eseidel, 2014-11-05 18:27:08]

https://codereview.chromium.org/695183003/diff/1/mojo/cc/output_surface_mojo.cc
File mojo/cc/output_surface_mojo.cc (right):

https://codereview.chromium.org/695183003/diff/1/mojo/cc/output_surface_mojo....
mojo/cc/output_surface_mojo.cc:29:
surface_->DestroySurface(SurfaceId::From(surface_id_));
My understanding is that this is necessary based on reading SurfaceService code
(which does not know how to destroy surfaces when you close its pipe!).  We
don't actually hit this code yet because the process errors out earlier in
shutdown than this.

[sky, 2014-11-05 18:30:51]

sky@chromium.org changed reviewers:
+ sky@chromium.org

[sky, 2014-11-05 18:30:52]

https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
File mojo/services/view_manager/display_manager.cc (right):

https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
mojo/services/view_manager/display_manager.cc:99: if (!surface_id_.is_null())
Since we're shutting down is this strictly necessary? Won't the surface service
correctly clean up if the pipe is abruptly closed?

[eseidel, 2014-11-05 18:35:27]

On 2014/11/05 18:30:52, sky wrote:
>
https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
> File mojo/services/view_manager/display_manager.cc (right):
> 
>
https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
> mojo/services/view_manager/display_manager.cc:99: if (!surface_id_.is_null())
> Since we're shutting down is this strictly necessary? Won't the surface
service
> correctly clean up if the pipe is abruptly closed?

I don't see any code to hold onto the SurfacesImpls and delete them.

The SurfacesFactory explicitly logs in its destructor if Destroy was not called:
https://github.com/domokit/mojo/blob/master/cc/surfaces/surface_factory.cc#L19

Do you expect SurfacesImpl to call DestoryAll() before letting the
SurfacesFactory destruct?

[jamesr, 2014-11-05 18:42:50]

On 2014/11/05 18:35:27, eseidel wrote:
> On 2014/11/05 18:30:52, sky wrote:
> >
>
https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
> > File mojo/services/view_manager/display_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/695183003/diff/1/mojo/services/view_manager/d...
> > mojo/services/view_manager/display_manager.cc:99: if
(!surface_id_.is_null())
> > Since we're shutting down is this strictly necessary? Won't the surface
> service
> > correctly clean up if the pipe is abruptly closed?
> 
> I don't see any code to hold onto the SurfacesImpls and delete them.
> 
> The SurfacesFactory explicitly logs in its destructor if Destroy was not
called:
> https://github.com/domokit/mojo/blob/master/cc/surfaces/surface_factory.cc#L19
> 
> Do you expect SurfacesImpl to call DestoryAll() before letting the
> SurfacesFactory destruct?

It should, yes: https://code.google.com/p/chromium/issues/detail?id=430242#c5

[eseidel, 2014-11-05 18:46:57]

ptal

[jamesr, 2014-11-05 19:02:24]

https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
File mojo/services/surfaces/surfaces_impl.cc (right):

https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
mojo/services/surfaces/surfaces_impl.cc:28: // Destory any outstanding surfaces
when the connection goes down for any
s/Destory/Destroy/

https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
mojo/services/surfaces/surfaces_impl.cc:29: // reason to avoid ~SurfaceFactory()
logging about us failing to do so.
this comment is misleading. we aren't destroying to avoid a log message, we're
destroying because when the connection is gone all surfaces we might want to
manage should be destroyed.

we might need to refine this in the future

https://codereview.chromium.org/695183003/diff/40001/mojo/services/window_man...
File mojo/services/window_manager/window_manager_app.cc (right):

https://codereview.chromium.org/695183003/diff/40001/mojo/services/window_man...
mojo/services/window_manager/window_manager_app.cc:98: // Necessary since we
used WeakBindToPipe and are not automatically deleted.
then why weakly bind? the definition of strong binding is delete-on-error

[eseidel, 2014-11-05 19:21:05]

On 2014/11/05 19:02:24, jamesr wrote:
>
https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
> File mojo/services/surfaces/surfaces_impl.cc (right):
> 
>
https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
> mojo/services/surfaces/surfaces_impl.cc:28: // Destory any outstanding
surfaces
> when the connection goes down for any
> s/Destory/Destroy/
> 
>
https://codereview.chromium.org/695183003/diff/40001/mojo/services/surfaces/s...
> mojo/services/surfaces/surfaces_impl.cc:29: // reason to avoid
~SurfaceFactory()
> logging about us failing to do so.
> this comment is misleading. we aren't destroying to avoid a log message, we're
> destroying because when the connection is gone all surfaces we might want to
> manage should be destroyed.

I guess.  ~SurfaceFactory() calls DestoryAll(). :)  It just does so after
logging.  So we're really just avoiding the log.  I can remove the comment if
you like.
https://github.com/domokit/mojo/blob/master/cc/surfaces/surface_factory.cc#L19

> we might need to refine this in the future
> 
>
https://codereview.chromium.org/695183003/diff/40001/mojo/services/window_man...
> File mojo/services/window_manager/window_manager_app.cc (right):
> 
>
https://codereview.chromium.org/695183003/diff/40001/mojo/services/window_man...
> mojo/services/window_manager/window_manager_app.cc:98: // Necessary since we
> used WeakBindToPipe and are not automatically deleted.
> then why weakly bind? the definition of strong binding is delete-on-error

I was told to do this to avoid double-free.  Ask sky or davemoore.

[sky, 2014-11-05 20:02:21]

vm/wm LGTM

[jamesr, 2014-11-05 21:08:03]

lgtm

i don't think we have the pipe binding quite right yet, but that isn't this
code's problem.

[eseidel, 2014-11-05 21:09:15]

Committed patchset #6 (id:100001) manually as
3f1157b76bb4ed7a42344bb236ab2314d261d973 (presubmit successful).