Shutdown the status tray before the system tray delegate.

Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a crash introduced by the above change.

BUG=577413
Committed: https://crrev.com/551152efe645d6263df3e3177f374564b1fe936f
Cr-Commit-Position: refs/heads/master@{#371847}

[jdufault, 2016-01-25 19:43:36]

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

[commit-bot: I haz the power, 2016-01-25 19:46:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1614233002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1614233002/1

[commit-bot: I haz the power, 2016-01-25 20:23:10]

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

[commit-bot: I haz the power, 2016-01-25 20:23:11]

Dry run: This issue passed the CQ dry run.

[jdufault, 2016-01-25 22:54:46]

Description was changed from

==========
IGNORE: trybots for now

BUG=577413
==========

to

==========
Shutdown the status tray before the system tray delegate.

BUG=577413
==========

[jdufault, 2016-01-25 22:55:45]

jdufault@chromium.org changed reviewers:
+ achuith@chromium.org, oshima@chromium.org, stevenjb@chromium.org

[jdufault, 2016-01-25 22:56:16]

jdufault@chromium.org changed reviewers:
- achuith@chromium.org

[jdufault, 2016-01-25 23:02:33]

jdufault@chromium.org changed reviewers:
+ bshe@chromium.org, sadrul@chromium.org

[jdufault, 2016-01-25 23:06:04]

Oshima / Steven, PTAL.

+bshe@ or sadrul@ for keyboard deinit order change. The keyboard crashed if the
shelf was destroyed before the keyboard, because the shelf keyboard observer
starts a layout (see below for a stack trace). I'm not familiar with the
keyboard code, so I'm not sure if this an appropriate fix.

There's more context on the bug (crbug.com/577413) and at
https://codereview.chromium.org/1567103005.

Thanks!


Keyboard crash stack trace:
    base::debug::StackTrace::StackTrace()
    base::debug::(anonymous namespace)::StackDumpSignalHandler()
    <unknown>
    views::Widget::GetWindowBoundsInScreen()
    ash::ShelfLayoutManager::CalculateTargetBounds()
    ash::ShelfLayoutManager::LayoutShelf()
    ash::ShelfLayoutManager::OnWindowResized()
    ash::ShelfLayoutManager::OnKeyboardBoundsChanging()
    keyboard::KeyboardController::NotifyKeyboardBoundsChanging()
    ash::RootWindowController::DeactivateKeyboard()
    ash::RootWindowController::CloseChildWindows()
    ash::WindowTreeHostManager::CloseChildWindows()
    ash::Shell::~Shell()
    ash::Shell::~Shell()
    ash::Shell::DeleteInstance()
    ash::test::AshTestHelper::TearDown()
    ash::test::AshTestBase::TearDown()

[sadrul, 2016-01-25 23:15:26]

ShelfLayoutManager needs to be removed from the observer list before it is dead.
Otherwise, how can you tell that's the only place that triggers the VK
observers?

[stevenjb, 2016-01-25 23:28:17]

https://codereview.chromium.org/1614233002/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1614233002/diff/1/ash/root_window_controller....
ash/root_window_controller.cc:669: // notification to it.
This is problematic. Since (presumably) shelf()->shelf_layout_manager() is still
valid here, it needs to know to ignore this notification when it is shut down.
Artificially inverting this ordering is very confusing.

[jdufault, 2016-01-26 00:55:05]

On 2016/01/25 23:15:26, sadrul wrote:
> ShelfLayoutManager needs to be removed from the observer list before it is
dead.
> Otherwise, how can you tell that's the only place that triggers the VK
> observers?

I've changed the implementation to dismiss the keyboard before closing the
shelf, which avoids this issue entirely.

https://codereview.chromium.org/1614233002/diff/1/ash/root_window_controller.cc
File ash/root_window_controller.cc (right):

https://codereview.chromium.org/1614233002/diff/1/ash/root_window_controller....
ash/root_window_controller.cc:669: // notification to it.
On 2016/01/25 23:28:17, stevenjb wrote:
> This is problematic. Since (presumably) shelf()->shelf_layout_manager() is
still
> valid here, it needs to know to ignore this notification when it is shut down.
> Artificially inverting this ordering is very confusing.
> 

Reverted this change, the keyboard is now deinited before the shelf.

[stevenjb, 2016-01-26 00:59:12]

lgtm

[oshima, 2016-01-26 01:02:18]

can you also update the CL description (that it deactivates keyboard before
closing shelf).

lgtm

[oshima, 2016-01-26 01:04:20]

one q. can we remove null check you added before?

[jdufault, 2016-01-26 01:08:51]

Description was changed from

==========
Shutdown the status tray before the system tray delegate.

BUG=577413
==========

to

==========
Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a
crash introduced by the above change.

BUG=577413
==========

[jdufault, 2016-01-26 01:12:57]

On 2016/01/26 01:04:20, oshima wrote:
> one q. can we remove null check you added before?

Sure. I was planning on doing it in a follow-up CL but it probably makes more
sense to do it here.

[oshima, 2016-01-26 01:16:30]

lgtm. please pay attention to memory bot failure.

[jdufault, 2016-01-27 19:13:40]

The CQ bit was checked by jdufault@chromium.org

[jdufault, 2016-01-27 19:13:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
stevenjb@chromium.org, oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1614233002/#ps60001
(title: "Fix test failures")

[commit-bot: I haz the power, 2016-01-27 19:14:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1614233002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1614233002/60001

[commit-bot: I haz the power, 2016-01-27 19:48:50]

Description was changed from

==========
Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a
crash introduced by the above change.

BUG=577413
==========

to

==========
Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a
crash introduced by the above change.

BUG=577413
==========

[commit-bot: I haz the power, 2016-01-27 19:48:50]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-27 19:49:33]

Description was changed from

==========
Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a
crash introduced by the above change.

BUG=577413
==========

to

==========
Shutdown the status tray before the system tray delegate.

This also deactivates the keyboard before shutting down the shelf, which fixes a
crash introduced by the above change.

BUG=577413

Committed: https://crrev.com/551152efe645d6263df3e3177f374564b1fe936f
Cr-Commit-Position: refs/heads/master@{#371847}
==========

[commit-bot: I haz the power, 2016-01-27 19:49:34]

Patchset 4 (id:??) landed as
https://crrev.com/551152efe645d6263df3e3177f374564b1fe936f
Cr-Commit-Position: refs/heads/master@{#371847}