Make sure WindowObservers are removed from window before destruction

* Check if the window observer is empty upon destruction.
* remove the all observers in ~WindowObserver()
* Remove RemoveObserver that are no longer necessary
* Fix shell shutdown order so that observers are removed correctly before deleting all windows.

BUG=324018
R=skuhne@chromium.org, sky@chromium.org

Review URL: https://codereview.chromium.org/101013002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@239232 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 34 insertions, 28 deletions

diff --git a/ash/drag_drop/drag_drop_controller.cc b/ash/drag_drop/drag_drop_controller.cc
index 07ab4f3..1d5d6363 100644
--- a/ash/drag_drop/drag_drop_controller.cc
+++ b/ash/drag_drop/drag_drop_controller.cc
@@ -461,14 +461,10 @@ void DragDropController::OnGestureEvent(ui::GestureEvent* event) {
 }
 
 void DragDropController::OnWindowDestroyed(aura::Window* window) {
-  if (drag_window_ == window) {
-    drag_window_->RemoveObserver(this);
+  if (drag_window_ == window)
     drag_window_ = NULL;
-  }
-  if (drag_source_window_ == window) {
-    drag_source_window_->RemoveObserver(this);
+  if (drag_source_window_ == window)
     drag_source_window_ = NULL;
-  }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/ash/shell.cc b/ash/shell.cc
index cf11a46..5662693 100644
--- a/ash/shell.cc
+++ b/ash/shell.cc
@@ -632,44 +632,55 @@ Shell::~Shell() {
   // Drag-and-drop must be canceled prior to close all windows.
   drag_drop_controller_.reset();
 
+  // Controllers who have WindowObserver added must be deleted
+  // before |display_controller_| is deleted.
+
+#if defined(OS_CHROMEOS)
+  // VideoActivityNotifier must be deleted before |video_detector_| is
+  // deleted because it's observing video activity through
+  // VideoDetectorObserver interface.
+  video_activity_notifier_.reset();
+#endif  // defined(OS_CHROMEOS)
+  video_detector_.reset();
+
+  shadow_controller_.reset();
+  resize_shadow_controller_.reset();
+
+  window_cycle_controller_.reset();
+  mru_window_tracker_.reset();
+
   // Destroy all child windows including widgets.
   display_controller_->CloseChildWindows();
   display_controller_->CloseNonDesktopDisplay();
 
+  // Chrome implementation of launcher delegate depends on FocusClient,
+  // so must be deleted before |focus_client_|.
+  launcher_delegate_.reset();
+  focus_client_.reset();
+
   // Destroy SystemTrayNotifier after destroying SystemTray as TrayItems
   // needs to remove observers from it.
   system_tray_notifier_.reset();
 
-#if defined(OS_CHROMEOS)
-  // Destroy VideoActivityNotifier before destroying VideoDetector.
-  video_activity_notifier_.reset();
-#endif  // defined(OS_CHROMEOS)
-
   // These need a valid Shell instance to clean up properly, so explicitly
   // delete them before invalidating the instance.
   // Alphabetical. TODO(oshima): sort.
   magnification_controller_.reset();
   partial_magnification_controller_.reset();
-  resize_shadow_controller_.reset();
-  shadow_controller_.reset();
   tooltip_controller_.reset();
   event_client_.reset();
-  window_cycle_controller_.reset();
   nested_dispatcher_controller_.reset();
   user_action_client_.reset();
   visibility_controller_.reset();
-  launcher_delegate_.reset();
   // |shelf_item_delegate_manager_| observes |shelf_model_|. It must be
   // destroyed before |shelf_model_| is destroyed.
   shelf_item_delegate_manager_.reset();
   // |shelf_window_watcher_| has a weak pointer to |shelf_Model_|.
   shelf_window_watcher_.reset();
   shelf_model_.reset();
-  video_detector_.reset();
 
   power_button_controller_.reset();
   lock_state_controller_.reset();
-  mru_window_tracker_.reset();
 
   resolution_notification_controller_.reset();
   desktop_background_controller_.reset();
diff --git a/ash/wm/solo_window_tracker_unittest.cc b/ash/wm/solo_window_tracker_unittest.cc
index a874bb8..431ae00 100644
--- a/ash/wm/solo_window_tracker_unittest.cc
+++ b/ash/wm/solo_window_tracker_unittest.cc
@@ -29,10 +29,14 @@ namespace {
 class WindowRepaintChecker : public aura::WindowObserver {
  public:
   explicit WindowRepaintChecker(aura::Window* window)
-      : is_paint_scheduled_(false) {
-    window->AddObserver(this);
+      : window_(window),
+        is_paint_scheduled_(false) {
+    window_->AddObserver(this);
   }
+
   virtual ~WindowRepaintChecker() {
+    if (window_)
+      window_->RemoveObserver(this);
   }
 
   bool IsPaintScheduledAndReset() {
@@ -47,10 +51,12 @@ class WindowRepaintChecker : public aura::WindowObserver {
                                       const gfx::Rect& region) OVERRIDE {
     is_paint_scheduled_ = true;
   }
-  virtual void OnWindowDestroying(aura::Window* window) OVERRIDE {
-    window->RemoveObserver(this);
+  virtual void OnWindowDestroyed(aura::Window* window) OVERRIDE {
+    DCHECK_EQ(window_, window);
+    window_ = NULL;
   }
 
+  aura::Window* window_;
   bool is_paint_scheduled_;
 
   DISALLOW_COPY_AND_ASSIGN(WindowRepaintChecker);
diff --git a/ash/wm/window_cycle_list.cc b/ash/wm/window_cycle_list.cc
index 428804f..d6bf24a 100644
--- a/ash/wm/window_cycle_list.cc
+++ b/ash/wm/window_cycle_list.cc
@@ -72,8 +72,6 @@ int WindowCycleList::GetWindowIndex(aura::Window* window) {
 }
 
 void WindowCycleList::OnWindowDestroyed(aura::Window* window) {
-  window->RemoveObserver(this);
-
   WindowList::iterator i = std::find(windows_.begin(), windows_.end(), window);
   DCHECK(i != windows_.end());
   int removed_index = static_cast<int>(i - windows_.begin());
diff --git a/ash/wm/window_state.cc b/ash/wm/window_state.cc
index 071307f..0a47384 100644
--- a/ash/wm/window_state.cc
+++ b/ash/wm/window_state.cc
@@ -270,10 +270,6 @@ void WindowState::OnWindowPropertyChanged(aura::Window* window,
   }
 }
 
-void WindowState::OnWindowDestroying(aura::Window* window) {
-  window_->RemoveObserver(this);
-}
-
 void WindowState::SnapWindow(WindowShowType left_or_right,
                              const gfx::Rect& bounds) {
   // Compute the bounds that the window will restore to. If the window does not
diff --git a/ash/wm/window_state.h b/ash/wm/window_state.h
index 4484f95..d67c1e5 100644
--- a/ash/wm/window_state.h
+++ b/ash/wm/window_state.h
@@ -248,7 +248,6 @@ class ASH_EXPORT WindowState : public aura::WindowObserver {
   virtual void OnWindowPropertyChanged(aura::Window* window,
                                        const void* key,
                                        intptr_t old) OVERRIDE;
-  virtual void OnWindowDestroying(aura::Window* window) OVERRIDE;
 
  private:
   // Snaps the window to left or right of the desktop with given bounds.