Speculative fix for crash in ash::Shelf::SetAlignment()

When disconnecting an external monitor the display manager shuts down
the associated RootWindowController, but defers deletion of the
RootWindowController via a posted task. This means an ash::Shelf
can exist without a ShelfWidget.

A similar code path exists for entering tablet mode, which turns on
display mirroring, and shuts down the RootWindowController for the
external display.

If there's a pending task that changes session state (for example,
the user's sign-in completes), then SessionController observers can
fire and attempt to update the alignment of the shelf.

Add null checks to ash::Shelf for shelf_widget_. I think we used to
have these many years ago, and I think I removed them in 2016 when
when cleaning up ash::Shelf. Sadly, they are still needed.

Bug: 937495, 738011
Test: ash_unittests
Change-Id: I794c610ba1f56483dae49f49e6d955ccf15ce10d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1542572Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#645321}

ash/display/window_tree_host_manager.cc

@@ -548,6 +548,7 @@ void WindowTreeHostManager::DeleteHost(AshWindowTreeHost* host_to_delete) {
   controller->Shutdown();
   if (primary_tree_host_for_replace_ == host_to_delete)
     primary_tree_host_for_replace_ = nullptr;
+  // NOTE: ShelfWidget is gone, but Shelf still exists until this task runs.
   base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, controller);
 }
 

ash/shelf/shelf.cc

@@ -143,13 +143,12 @@ bool Shelf::IsVisible() const {
 }
 
 aura::Window* Shelf::GetWindow() {
-  return shelf_widget_->GetNativeWindow();
+  return shelf_widget_ ? shelf_widget_->GetNativeWindow() : nullptr;
 }
 
 void Shelf::SetAlignment(ShelfAlignment alignment) {
-  // Checks added for http://crbug.com/738011.
-  CHECK(shelf_widget_);
-  CHECK(shelf_layout_manager_);
+  if (!shelf_widget_)
+    return;
 
   if (alignment_ == alignment)
     return;
@@ -218,7 +217,8 @@ void Shelf::UpdateAutoHideState() {
 }
 
 ShelfBackgroundType Shelf::GetBackgroundType() const {
-  return shelf_widget_->GetBackgroundType();
+  return shelf_widget_ ? shelf_widget_->GetBackgroundType()
+                       : SHELF_BACKGROUND_DEFAULT;
 }
 
 void Shelf::UpdateVisibilityState() {
@@ -280,7 +280,7 @@ void Shelf::NotifyShelfIconPositionsChanged() {
 }
 
 StatusAreaWidget* Shelf::GetStatusAreaWidget() const {
-  return shelf_widget_->status_area_widget();
+  return shelf_widget_ ? shelf_widget_->status_area_widget() : nullptr;
 }
 
 TrayBackgroundView* Shelf::GetSystemTrayAnchorView() const {

ash/shelf/shelf.h

@@ -191,6 +191,8 @@ class ASH_EXPORT Shelf : public ShelfLayoutManagerObserver {
   // ShelfWidget and lifetimes are managed by the container windows themselves.
   ShelfLayoutManager* shelf_layout_manager_ = nullptr;
 
+  // Null during display teardown, see WindowTreeHostManager::DeleteHost() and
+  // RootWindowController::CloseAllChildWindows().
   std::unique_ptr<ShelfWidget> shelf_widget_;
 
   // These initial values hide the shelf until user preferences are available.

ash/shelf/shelf_locking_manager.h

@@ -36,7 +36,7 @@ class ASH_EXPORT ShelfLockingManager : public SessionObserver,
   // Update the shelf state for session and screen lock changes.
   void UpdateLockedState();
 
-  Shelf* shelf_;
+  Shelf* const shelf_;
   bool session_locked_ = false;
   bool screen_locked_ = false;
   ShelfAlignment stored_alignment_;

ash/shelf/shelf_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "ash/public/cpp/shelf_model.h"
 #include "ash/root_window_controller.h"
+#include "ash/session/session_controller.h"
 #include "ash/session/test_session_controller_client.h"
 #include "ash/shelf/overflow_button.h"
 #include "ash/shelf/shelf.h"
@@ -17,6 +18,7 @@
 #include "ash/shell.h"
 #include "ash/test/ash_test_base.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/session_manager/session_manager_types.h"
 #include "mojo/public/cpp/bindings/associated_binding.h"
 
@@ -145,5 +147,31 @@ TEST_F(ShelfTest, ShelfHiddenOnScreenOnSecondaryDisplay) {
   }
 }
 
+using NoSessionShelfTest = NoSessionAshTestBase;
+
+// Regression test for crash in Shelf::SetAlignment(). https://crbug.com/937495
+TEST_F(NoSessionShelfTest, SetAlignmentDuringDisplayDisconnect) {
+  UpdateDisplay("1024x768,800x600");
+  base::RunLoop().RunUntilIdle();
+
+  // The task indirectly triggers Shelf::SetAlignment() via a SessionObserver.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          [](TestSessionControllerClient* session) {
+            session->SetSessionState(session_manager::SessionState::ACTIVE);
+          },
+          GetSessionControllerClient()));
+
+  // Remove the secondary display.
+  UpdateDisplay("1280x1024");
+
+  // The session activation task runs before the RootWindowController and the
+  // Shelf are deleted.
+  base::RunLoop().RunUntilIdle();
+
+  // No crash.
+}
+
 }  // namespace
 }  // namespace ash