mash: Avoid shelf crashes on display removal.

mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second display.
R=sky@chromium.org

Committed: https://crrev.com/709f84d2f2bac43662b31b9393812c8a711f6da0
Cr-Commit-Position: refs/heads/master@{#416747}

[msw, 2016-09-01 19:40:59]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

BUG=641463
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

Add DisplayObserver::OnDisplayRemoving notification.
(only works for ScreenMus right now...)

Make Mus' RootWindowController a DisplayObserver.
Shutdown the shelf widget in new OnDisplayRemoving.
(shutdown needs to access the display before its removed)

BUG=641463
TEST=
R=
==========

[msw, 2016-09-01 19:41:16]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

Add DisplayObserver::OnDisplayRemoving notification.
(only works for ScreenMus right now...)

Make Mus' RootWindowController a DisplayObserver.
Shutdown the shelf widget in new OnDisplayRemoving.
(shutdown needs to access the display before its removed)

BUG=641463
TEST=
R=
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

Add DisplayObserver::OnDisplayRemoving notification.
(only works for ScreenMus right now...)

Make Mus' RootWindowController a DisplayObserver.
Shutdown the shelf widget in new OnDisplayRemoving.
(shutdown needs to access the display before its removed)

BUG=641463
TEST=
R=sky@chromium.org
==========

[msw, 2016-09-01 19:41:16]

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

[msw, 2016-09-01 19:42:49]

Hey Scott, can you take a quick look? I want to check my direction before trying
to implement and test the new DisplayObserver::OnDisplayRemoving (and
OnDisplayAdding? WDYT?) for other display::Screen implementations.

[msw, 2016-09-01 19:44:41]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-01 19:45:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-01 20:39:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 20:39:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2016-09-01 20:50:27]

When a display is removed at least two things happen:
1. the roots of the display are destroyed, which results in notifying the window
manager of the windows being destroyed.
2. mus notifies observers of the display change by way of
DisplayManagerObserver.

Because ScreenMus and the WindowManager use separate pipes the ordering of 1 and
2 is non-deterministic. This is why I think at least part of the fix is to
change WindowManagerApplication from using ScreenMus and instead use it's own
Screen implementation backed by a DisplayList that is manipulated solely by
WindowManager. By doing this mash only observers changes to the displays via 1,
and there aren't ordering issues.

Once you do that, it should be easy to ensure we have the same shutdown order as
happens in cash. Which I believe is something like
WindowTreeHostManager::DeleteHost.

When the last display is removed I think cash does something special (search for
ScreenForShutdown), we likely need to do that too.

Feel free to stop by and chat about this.

[msw, 2016-09-01 23:37:48]

On 2016/09/01 20:50:27, sky wrote:
> When a display is removed at least two things happen:
> 1. the roots of the display are destroyed, which results in notifying the
window
> manager of the windows being destroyed.
> 2. mus notifies observers of the display change by way of
> DisplayManagerObserver.
> 
> Because ScreenMus and the WindowManager use separate pipes the ordering of 1
and
> 2 is non-deterministic. This is why I think at least part of the fix is to
> change WindowManagerApplication from using ScreenMus and instead use it's own
> Screen implementation backed by a DisplayList that is manipulated solely by
> WindowManager. By doing this mash only observers changes to the displays via
1,
> and there aren't ordering issues.
> 
> Once you do that, it should be easy to ensure we have the same shutdown order
as
> happens in cash. Which I believe is something like
> WindowTreeHostManager::DeleteHost.
> 
> When the last display is removed I think cash does something special (search
for
> ScreenForShutdown), we likely need to do that too.
> 
> Feel free to stop by and chat about this.

Is Patch Set 6 headed in the right direction?
(perhaps excuse views/mus -> ui/display moves)

[sky, 2016-09-01 23:51:17]

https://codereview.chromium.org/2302673002/diff/120001/ash/mus/root_window_co...
File ash/mus/root_window_controller.cc (right):

https://codereview.chromium.org/2302673002/diff/120001/ash/mus/root_window_co...
ash/mus/root_window_controller.cc:139: wm_shelf_->ShutdownShelfWidget();
Can this be moved to WmRootWindowController? I think there is a bunch of stuff
in RootWindowController::CloseChildWindows that should likely move to
WmRootWindowController.

https://codereview.chromium.org/2302673002/diff/120001/ash/mus/window_manager.cc
File ash/mus/window_manager.cc (right):

https://codereview.chromium.org/2302673002/diff/120001/ash/mus/window_manager...
ash/mus/window_manager.cc:188: connector_->ConnectToInterface("mojo:ui",
&display_manager);
The windowmanager shouldn't need to connect to displaymanager because it it is
already told what it needs via WindowManager/WindowTreeClient calls. For
example, when a new display is added OnWmNewDisplay() is called (when mash first
connects to mus, mus calls OnWmNewDisplay for all the existing displays).
Removal can be detected by the root being destroyed (as the code has now in
WindowManager::OnWindowDestroying). We may need to propagate metrics changes
separately, but you shouldn't need to worry about that now.

[msw, 2016-09-06 21:28:47]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

Add DisplayObserver::OnDisplayRemoving notification.
(only works for ScreenMus right now...)

Make Mus' RootWindowController a DisplayObserver.
Shutdown the shelf widget in new OnDisplayRemoving.
(shutdown needs to access the display before its removed)

BUG=641463
TEST=
R=sky@chromium.org
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

[msw, 2016-09-06 21:31:11]

Hey Scott, please take another look; thanks! I can fix the crash much more
simply now; possibly because of your codereview.chromium.org/2301353003.

[msw, 2016-09-06 21:31:16]

The CQ bit was checked by msw@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 21:32:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msw, 2016-09-06 21:32:44]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

[sky, 2016-09-06 22:17:02]

LGTM

[commit-bot: I haz the power, 2016-09-06 22:17:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 22:17:22]

Dry run: This issue passed the CQ dry run.

[msw, 2016-09-06 22:17:54]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2016-09-06 22:18:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:00:05]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

[commit-bot: I haz the power, 2016-09-06 23:00:06]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-09-06 23:02:58]

Description was changed from

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org
==========

to

==========
mash: Avoid shelf crashes on display removal.

Make ShelfWidget CloseNow in Shutdown.
Call ShutdownShelfWidget() in the WmShelfMus dtor.
Check for a valid shelf window in UpdateVisibilityState.

BUG=641463
TEST='chrome --mash --multi-display' doesn't crash when closing the second
display.
R=sky@chromium.org

Committed: https://crrev.com/709f84d2f2bac43662b31b9393812c8a711f6da0
Cr-Commit-Position: refs/heads/master@{#416747}
==========

[commit-bot: I haz the power, 2016-09-06 23:02:59]

Patchset 10 (id:??) landed as
https://crrev.com/709f84d2f2bac43662b31b9393812c8a711f6da0
Cr-Commit-Position: refs/heads/master@{#416747}