Fixes shutdown race in mash

Prior to this change it was possible for the AcceleratorRegistrarImpl
to outlive the WindowManager and then attempt to use it. This patch
ensures any AcceleratorRegistrarImpl are destroyed when the
WindowManager loses its connection.

BUG=none
TEST=none
R=ben@chromium.org

Review-Url: https://codereview.chromium.org/2105653003
Cr-Commit-Position: refs/heads/master@{#402670}

ash/mus/BUILD.gn

@@ -54,7 +54,6 @@ source_set("lib") {
     "property_util.h",
     "root_window_controller.cc",
     "root_window_controller.h",
-    "root_windows_observer.h",
     "screenlock_layout.cc",
     "screenlock_layout.h",
     "shadow.cc",
@@ -74,6 +73,7 @@ source_set("lib") {
     "window_manager.h",
     "window_manager_application.cc",
     "window_manager_application.h",
+    "window_manager_observer.h",
   ]
 
   deps = [

ash/mus/accelerator_registrar_impl.cc

@@ -35,6 +35,7 @@ AcceleratorRegistrarImpl::AcceleratorRegistrarImpl(
       binding_(this, std::move(request)),
       accelerator_namespace_(accelerator_namespace & 0xffff),
       destroy_callback_(destroy_callback) {
+  window_manager_->AddObserver(this);
   binding_.set_connection_error_handler(base::Bind(
       &AcceleratorRegistrarImpl::OnBindingGone, base::Unretained(this)));
 }
@@ -57,6 +58,7 @@ void AcceleratorRegistrarImpl::ProcessAccelerator(uint32_t accelerator_id,
 }
 
 AcceleratorRegistrarImpl::~AcceleratorRegistrarImpl() {
+  window_manager_->RemoveObserver(this);
   RemoveAllAccelerators();
   destroy_callback_.Run(this);
 }
@@ -132,5 +134,15 @@ void AcceleratorRegistrarImpl::RemoveAccelerator(uint32_t accelerator_id) {
     delete this;
 }
 
+void AcceleratorRegistrarImpl::OnAccelerator(uint32_t id,
+                                             const ui::Event& event) {
+  if (OwnsAccelerator(id))
+    ProcessAccelerator(id, event);
+}
+
+void AcceleratorRegistrarImpl::OnWindowTreeClientDestroyed() {
+  delete this;
+}
+
 }  // namespace mus
 }  // namespace ash

ash/mus/accelerator_registrar_impl.h

@@ -9,6 +9,7 @@
 
 #include <map>
 
+#include "ash/mus/window_manager_observer.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "components/mus/public/interfaces/accelerator_registrar.mojom.h"
@@ -24,7 +25,8 @@ class WindowManager;
 // connection. This manages its own lifetime, and destroys itself when the
 // AcceleratorRegistrar and all its AcceleratorHandlers are disconnected. Upon
 // destruction, it calls the DestroyCallback.
-class AcceleratorRegistrarImpl : public ::mus::mojom::AcceleratorRegistrar {
+class AcceleratorRegistrarImpl : public ::mus::mojom::AcceleratorRegistrar,
+                                 public WindowManagerObserver {
  public:
   using DestroyCallback = base::Callback<void(AcceleratorRegistrarImpl*)>;
   AcceleratorRegistrarImpl(WindowManager* window_manager,
@@ -56,6 +58,10 @@ class AcceleratorRegistrarImpl : public ::mus::mojom::AcceleratorRegistrar {
                       const AddAcceleratorCallback& callback) override;
   void RemoveAccelerator(uint32_t accelerator_id) override;
 
+  // WindowManagerObserver:
+  void OnAccelerator(uint32_t id, const ui::Event& event) override;
+  void OnWindowTreeClientDestroyed() override;
+
   WindowManager* window_manager_;
   ::mus::mojom::AcceleratorHandlerPtr accelerator_handler_;
   mojo::Binding<AcceleratorRegistrar> binding_;

ash/mus/test/wm_test_helper.cc

@@ -24,8 +24,7 @@ WmTestHelper::~WmTestHelper() {}
 
 void WmTestHelper::Init() {
   message_loop_.reset(new base::MessageLoopForUI());
-  window_manager_app_.window_manager_.reset(
-      new WindowManager(&window_manager_app_, nullptr));
+  window_manager_app_.window_manager_.reset(new WindowManager(nullptr));
   screen_ = new WmTestScreen;
   window_manager_app_.window_manager_->screen_.reset(screen_);
 

ash/mus/window_manager.cc

@@ -15,9 +15,8 @@
 #include "ash/mus/non_client_frame_controller.h"
 #include "ash/mus/property_util.h"
 #include "ash/mus/root_window_controller.h"
-#include "ash/mus/root_windows_observer.h"
 #include "ash/mus/shadow_controller.h"
-#include "ash/mus/window_manager_application.h"
+#include "ash/mus/window_manager_observer.h"
 #include "ash/public/interfaces/container.mojom.h"
 #include "components/mus/common/event_matcher_util.h"
 #include "components/mus/common/types.h"
@@ -39,9 +38,8 @@ void AssertTrue(bool success) {
   DCHECK(success);
 }
 
-WindowManager::WindowManager(WindowManagerApplication* window_manager_app,
-                             shell::Connector* connector)
-    : window_manager_app_(window_manager_app), connector_(connector) {}
+WindowManager::WindowManager(shell::Connector* connector)
+    : connector_(connector) {}
 
 WindowManager::~WindowManager() {
   // NOTE: |window_tree_client_| may already be null.
@@ -99,12 +97,12 @@ std::set<RootWindowController*> WindowManager::GetRootWindowControllers() {
   return result;
 }
 
-void WindowManager::AddRootWindowsObserver(RootWindowsObserver* observer) {
-  root_windows_observers_.AddObserver(observer);
+void WindowManager::AddObserver(WindowManagerObserver* observer) {
+  observers_.AddObserver(observer);
 }
 
-void WindowManager::RemoveRootWindowsObserver(RootWindowsObserver* observer) {
-  root_windows_observers_.RemoveObserver(observer);
+void WindowManager::RemoveObserver(WindowManagerObserver* observer) {
+  observers_.RemoveObserver(observer);
 }
 
 void WindowManager::AddAccelerators() {
@@ -134,7 +132,7 @@ RootWindowController* WindowManager::CreateRootWindowController(
   root_window_controllers_.insert(std::move(root_window_controller_ptr));
   window->AddObserver(this);
 
-  FOR_EACH_OBSERVER(RootWindowsObserver, root_windows_observers_,
+  FOR_EACH_OBSERVER(WindowManagerObserver, observers_,
                     OnRootWindowControllerAdded(root_window_controller));
   return root_window_controller;
 }
@@ -143,7 +141,7 @@ void WindowManager::OnWindowDestroying(::mus::Window* window) {
   for (auto it = root_window_controllers_.begin();
        it != root_window_controllers_.end(); ++it) {
     if ((*it)->root() == window) {
-      FOR_EACH_OBSERVER(RootWindowsObserver, root_windows_observers_,
+      FOR_EACH_OBSERVER(WindowManagerObserver, observers_,
                         OnWillDestroyRootWindowController((*it).get()));
       return;
     }
@@ -178,8 +176,11 @@ void WindowManager::OnWindowTreeClientDestroyed(
   shell_.reset();
   shadow_controller_.reset();
 
+  FOR_EACH_OBSERVER(WindowManagerObserver, observers_,
+                    OnWindowTreeClientDestroyed());
+
   window_tree_client_ = nullptr;
-  // TODO(sky): this should likely shutdown.
+  window_manager_client_ = nullptr;
 }
 
 void WindowManager::OnEventObserved(const ui::Event& event,
@@ -235,7 +236,8 @@ void WindowManager::OnAccelerator(uint32_t id, const ui::Event& event) {
       window_manager_client()->ActivateNextWindow();
       break;
     default:
-      window_manager_app_->OnAccelerator(id, event);
+      FOR_EACH_OBSERVER(WindowManagerObserver, observers_,
+                        OnAccelerator(id, event));
       break;
   }
 }

ash/mus/window_manager.h

@@ -31,9 +31,8 @@ namespace ash {
 namespace mus {
 
 class RootWindowController;
-class RootWindowsObserver;
 class ShadowController;
-class WindowManagerApplication;
+class WindowManagerObserver;
 class WmShellMus;
 class WmLookupMus;
 class WmTestHelper;
@@ -46,8 +45,7 @@ class WindowManager : public ::mus::WindowManagerDelegate,
                       public ::mus::WindowObserver,
                       public ::mus::WindowTreeClientDelegate {
  public:
-  WindowManager(WindowManagerApplication* window_manager_app,
-                shell::Connector* connector);
+  explicit WindowManager(shell::Connector* connector);
   ~WindowManager() override;
 
   void Init(::mus::WindowTreeClient* window_tree_client);
@@ -68,8 +66,8 @@ class WindowManager : public ::mus::WindowManagerDelegate,
 
   std::set<RootWindowController*> GetRootWindowControllers();
 
-  void AddRootWindowsObserver(RootWindowsObserver* observer);
-  void RemoveRootWindowsObserver(RootWindowsObserver* observer);
+  void AddObserver(WindowManagerObserver* observer);
+  void RemoveObserver(WindowManagerObserver* observer);
 
  private:
   friend class WmTestHelper;
@@ -105,9 +103,6 @@ class WindowManager : public ::mus::WindowManagerDelegate,
                       const display::Display& display) override;
   void OnAccelerator(uint32_t id, const ui::Event& event) override;
 
-  // TODO(sky): this is unfortunate, remove.
-  WindowManagerApplication* window_manager_app_;
-
   shell::Connector* connector_;
 
   ::mus::WindowTreeClient* window_tree_client_ = nullptr;
@@ -118,7 +113,7 @@ class WindowManager : public ::mus::WindowManagerDelegate,
 
   std::set<std::unique_ptr<RootWindowController>> root_window_controllers_;
 
-  base::ObserverList<RootWindowsObserver> root_windows_observers_;
+  base::ObserverList<WindowManagerObserver> observers_;
 
   std::unique_ptr<display::Screen> screen_;
 

ash/mus/window_manager_application.cc

@@ -8,7 +8,6 @@
 
 #include "ash/mus/accelerator_registrar_impl.h"
 #include "ash/mus/root_window_controller.h"
-#include "ash/mus/root_windows_observer.h"
 #include "ash/mus/shelf_layout_impl.h"
 #include "ash/mus/user_window_controller_impl.h"
 #include "ash/mus/window_manager.h"
@@ -43,16 +42,6 @@ WindowManagerApplication::~WindowManagerApplication() {
   window_manager_.reset();
 }
 
-void WindowManagerApplication::OnAccelerator(uint32_t id,
-                                             const ui::Event& event) {
-  for (auto* registrar : accelerator_registrars_) {
-    if (registrar->OwnsAccelerator(id)) {
-      registrar->ProcessAccelerator(id, event);
-      break;
-    }
-  }
-}
-
 void WindowManagerApplication::OnAcceleratorRegistrarDestroyed(
     AcceleratorRegistrarImpl* registrar) {
   accelerator_registrars_.erase(registrar);
@@ -61,7 +50,7 @@ void WindowManagerApplication::OnAcceleratorRegistrarDestroyed(
 void WindowManagerApplication::InitWindowManager(
     ::mus::WindowTreeClient* window_tree_client) {
   window_manager_->Init(window_tree_client);
-  window_manager_->AddRootWindowsObserver(this);
+  window_manager_->AddObserver(this);
 }
 
 void WindowManagerApplication::Initialize(shell::Connector* connector,
@@ -69,7 +58,7 @@ void WindowManagerApplication::Initialize(shell::Connector* connector,
                                           uint32_t id) {
   connector_ = connector;
   ::mus::GpuService::Initialize(connector);
-  window_manager_.reset(new WindowManager(this, connector_));
+  window_manager_.reset(new WindowManager(connector_));
 
   aura_init_.reset(new views::AuraInit(connector_, "ash_mus_resources.pak"));
 
@@ -122,6 +111,9 @@ void WindowManagerApplication::Create(
 void WindowManagerApplication::Create(
     shell::Connection* connection,
     mojo::InterfaceRequest<::mus::mojom::AcceleratorRegistrar> request) {
+  if (!window_manager_->window_manager_client())
+    return;  // Can happen during shutdown.
+
   static int accelerator_registrar_count = 0;
   if (accelerator_registrar_count == std::numeric_limits<int>::max()) {
     // Restart from zero if we have reached the limit. It is technically

ash/mus/window_manager_application.h

@@ -10,7 +10,7 @@
 #include <memory>
 #include <set>
 
-#include "ash/mus/root_windows_observer.h"
+#include "ash/mus/window_manager_observer.h"
 #include "ash/public/interfaces/shelf_layout.mojom.h"
 #include "ash/public/interfaces/user_window_controller.mojom.h"
 #include "base/macros.h"
@@ -49,7 +49,7 @@ class WindowManagerApplication
       public shell::InterfaceFactory<mojom::UserWindowController>,
       public shell::InterfaceFactory<::mus::mojom::AcceleratorRegistrar>,
       public mash::session::mojom::ScreenlockStateListener,
-      public RootWindowsObserver {
+      public WindowManagerObserver {
  public:
   WindowManagerApplication();
   ~WindowManagerApplication() override;
@@ -58,9 +58,6 @@ class WindowManagerApplication
 
   WindowManager* window_manager() { return window_manager_.get(); }
 
-  // TODO(sky): figure out right place for this code.
-  void OnAccelerator(uint32_t id, const ui::Event& event);
-
   mash::session::mojom::Session* session() { return session_.get(); }
 
  private:
@@ -94,7 +91,7 @@ class WindowManagerApplication
   // session::mojom::ScreenlockStateListener:
   void ScreenlockStateChanged(bool locked) override;
 
-  // RootWindowsObserver:
+  // WindowManagerObserver:
   void OnRootWindowControllerAdded(RootWindowController* controller) override;
   void OnWillDestroyRootWindowController(
       RootWindowController* controller) override;

ash/mus/root_windows_observer.h → ash/mus/window_manager_observer.h

@@ -2,28 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef ASH_MUS_ROOT_WINDOW_OBSERVERS_H_
-#define ASH_MUS_ROOT_WINDOW_OBSERVERS_H_
+#ifndef ASH_MUS_WINDOW_OBSERVER_H_
+#define ASH_MUS_WINDOW_OBSERVER_H_
+
+#include <stdint.h>
+
+namespace ui {
+class Event;
+}
 
 namespace ash {
 namespace mus {
 
 class RootWindowController;
 
-class RootWindowsObserver {
+class WindowManagerObserver {
  public:
-  // Called once a new display has been created and the root Window obtained.
-  virtual void OnRootWindowControllerAdded(RootWindowController* controller) {}
+  // Called when the WindowTreeClient associated with the WindowManager is
+  // about to be destroyed.
+  virtual void OnWindowTreeClientDestroyed() {}
 
-  // Called before a RootWindowController is destroyed.
+  virtual void OnAccelerator(uint32_t id, const ui::Event& event) {}
+
+  virtual void OnRootWindowControllerAdded(RootWindowController* controller) {}
   virtual void OnWillDestroyRootWindowController(
       RootWindowController* controller) {}
 
  protected:
-  ~RootWindowsObserver() {}
+  virtual ~WindowManagerObserver() {}
 };
 
 }  // namespace mus
 }  // namespace ash
 
-#endif  // ASH_MUS_ROOT_WINDOW_OBSERVERS_H_
+#endif  // ASH_MUS_WINDOW_OBSERVER_H_