accessibility: Clean up observers of AccessibilityController in shutdown

All observers of the AccessibilityController shall be removed before the
AccessibilityController is destroyed.

Change-Id: I8b4cab4ce71a3bfb53c2f04b940dde218a494124
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1816301
Commit-Queue: Avery Musbach <amusbach@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698628}

ash/accessibility/accessibility_controller_impl.cc

@@ -358,6 +358,9 @@ void AccessibilityControllerImpl::RegisterProfilePrefs(
 void AccessibilityControllerImpl::Shutdown() {
   Shell::Get()->tablet_mode_controller()->RemoveObserver(this);
   Shell::Get()->session_controller()->RemoveObserver(this);
+
+  for (auto& observer : observers_)
+    observer.OnAccessibilityControllerShutdown();
 }
 
 void AccessibilityControllerImpl::SetHighContrastAcceleratorDialogAccepted() {

ash/accessibility/accessibility_controller_impl.h

@@ -337,7 +337,7 @@ class ASH_EXPORT AccessibilityControllerImpl : public AccessibilityController,
   // Used to force the backlights off to darken the screen.
   std::unique_ptr<ScopedBacklightsForcedOff> scoped_backlights_forced_off_;
 
-  base::ObserverList<AccessibilityObserver>::Unchecked observers_;
+  base::ObserverList<AccessibilityObserver> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(AccessibilityControllerImpl);
 };

ash/accessibility/accessibility_controller_unittest.cc

@@ -999,6 +999,8 @@ TEST_F(AccessibilityControllerTest, SelectToSpeakStateChanges) {
   EXPECT_EQ(controller->GetSelectToSpeakState(),
             ash::SelectToSpeakState::kSelectToSpeakStateSpeaking);
   EXPECT_EQ(observer.status_changed_count_, 2);
+
+  controller->RemoveObserver(&observer);
 }
 
 namespace {

ash/accessibility/accessibility_observer.h

@@ -6,15 +6,21 @@
 #define ASH_ACCESSIBILITY_ACCESSIBILITY_OBSERVER_H_
 
 #include "ash/ash_export.h"
+#include "base/observer_list_types.h"
 
 namespace ash {
 
-class ASH_EXPORT AccessibilityObserver {
+class ASH_EXPORT AccessibilityObserver : public base::CheckedObserver {
  public:
-  virtual ~AccessibilityObserver() = default;
-
   // Called when any accessibility status changes.
   virtual void OnAccessibilityStatusChanged() = 0;
+
+  // Called when the accessibility controller is being shutdown. Provides an
+  // opportunity for observers to remove themselves.
+  virtual void OnAccessibilityControllerShutdown() {}
+
+ protected:
+  ~AccessibilityObserver() override = default;
 };
 
 }  // namespace ash

ash/accessibility/key_accessibility_enabler_unittest.cc

@@ -30,6 +30,7 @@ class KeyAccessibilityEnablerTest : public AshTestBase,
 
   void TearDown() override {
     ui::SetEventTickClockForTesting(nullptr);
+    Shell::Get()->accessibility_controller()->RemoveObserver(this);
     AshTestBase::TearDown();
   }
 

ash/accessibility/touch_exploration_manager.cc

@@ -49,7 +49,8 @@ TouchExplorationManager::TouchExplorationManager(
 }
 
 TouchExplorationManager::~TouchExplorationManager() {
-  // TODO(jamescook): Clean up shutdown order so this check isn't needed.
+  // TODO(jamescook): Clean up shutdown order so this check isn't needed. See
+  // also the TODO in |OnAccessibilityControllerShutdown|.
   if (Shell::Get()->accessibility_controller())
     Shell::Get()->accessibility_controller()->RemoveObserver(this);
   Shell::Get()->activation_client()->RemoveObserver(this);
@@ -64,6 +65,18 @@ void TouchExplorationManager::OnAccessibilityStatusChanged() {
   UpdateTouchExplorationState();
 }
 
+void TouchExplorationManager::OnAccessibilityControllerShutdown() {
+  // This code helps with shutdown, but it does not obviate the need for similar
+  // code in |TouchExplorationManager::~TouchExplorationManager|. That is
+  // because there is a |TouchExplorationManager| per display, but only one
+  // |AccessibilityController|. If you disconnect an external display, then the
+  // corresponding |TouchExplorationManager| will be destroyed, but
+  // |OnAccessibilityControllerShutdown| will not be called thereon.
+  // TODO(jamescook): Clean up shutdown order so this code is not reached (and
+  // then remove it). See also the TODO in |~TouchExplorationManager|.
+  Shell::Get()->accessibility_controller()->RemoveObserver(this);
+}
+
 void TouchExplorationManager::OnWindowPropertyChanged(aura::Window* winodw,
                                                       const void* key,
                                                       intptr_t old) {

ash/accessibility/touch_exploration_manager.h

@@ -49,6 +49,7 @@ class ASH_EXPORT TouchExplorationManager
 
   // AccessibilityObserver overrides:
   void OnAccessibilityStatusChanged() override;
+  void OnAccessibilityControllerShutdown() override;
 
   // aura::WindowObserver overrides:
   void OnWindowPropertyChanged(aura::Window* window,