Cleans up a couple of places that were installing WindowObservers but

never removing them. This is problematic as it is possible for a
Window to be destroyed after the shell.

ScopedObserver is pretty generic, but I'll leave it here for now.

BUG=124860
TEST=covered by unit test
R=ben@chromium.org

Review URL: https://chromiumcodereview.appspot.com/10206027

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

10 files changed, 102 insertions, 18 deletions

diff --git a/ash/ash.gyp b/ash/ash.gyp
index a56b7eb..06c1053 100644
--- a/ash/ash.gyp
+++ b/ash/ash.gyp
@@ -230,6 +230,7 @@
         'wm/root_window_event_filter.h',
         'wm/root_window_layout_manager.cc',
         'wm/root_window_layout_manager.h',
+        'wm/scoped_observer.h',
         'wm/shadow.cc',
         'wm/shadow.h',
         'wm/shadow_controller.cc',
diff --git a/ash/shell_unittest.cc b/ash/shell_unittest.cc
index d4dd7ea..88e8fd9 100644
--- a/ash/shell_unittest.cc
+++ b/ash/shell_unittest.cc
@@ -317,4 +317,28 @@ TEST_F(ShellTest, FullscreenWindowHidesShelf) {
   widget->Close();
 }
 
+// This verifies WindowObservers are removed when a window is destroyed after
+// the Shell is destroyed. This scenario (aura::Windows being deleted after the
+// Shell) occurs if someone is holding a reference to an unparented Window, as
+// is the case with a RenderWidgetHostViewAura that isn't on screen. As long as
+// everything is ok, we won't crash. If there is a bug, window's destructor will
+// notify some deleted object (say VideoDetector or ActivationController) and
+// this will crash.
+class ShellTest2 : public test::AshTestBase {
+ public:
+  ShellTest2() {}
+  virtual ~ShellTest2() {}
+
+ protected:
+  scoped_ptr<aura::Window> window_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ShellTest2);
+};
+
+TEST_F(ShellTest2, DontCrashWhenWindowDeleted) {
+  window_.reset(new aura::Window(NULL));
+  window_->Init(ui::LAYER_NOT_DRAWN);
+}
+
 }  // namespace ash
diff --git a/ash/wm/activation_controller.cc b/ash/wm/activation_controller.cc
index bf792fa..b17c306 100644
--- a/ash/wm/activation_controller.cc
+++ b/ash/wm/activation_controller.cc
@@ -116,7 +116,8 @@ void StackTransientParentsBelowModalWindow(aura::Window* window) {
 // ActivationController, public:
 
 ActivationController::ActivationController()
-    : updating_activation_(false) {
+    : updating_activation_(false),
+      ALLOW_THIS_IN_INITIALIZER_LIST(observer_manager_(this)) {
   aura::client::SetActivationClient(Shell::GetRootWindow(), this);
   aura::Env::GetInstance()->AddObserver(this);
   Shell::GetRootWindow()->AddRootWindowObserver(this);
@@ -199,14 +200,14 @@ void ActivationController::OnWindowDestroying(aura::Window* window) {
         aura::client::kRootWindowActiveWindowKey);
     ActivateWindow(GetTopmostWindowToActivate(window));
   }
-  window->RemoveObserver(this);
+  observer_manager_.Remove(window);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ActivationController, aura::EnvObserver implementation:
 
 void ActivationController::OnWindowInitialized(aura::Window* window) {
-  window->AddObserver(this);
+  observer_manager_.Add(window);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
diff --git a/ash/wm/activation_controller.h b/ash/wm/activation_controller.h
index 9494345..7bdb19a 100644
--- a/ash/wm/activation_controller.h
+++ b/ash/wm/activation_controller.h
@@ -6,6 +6,7 @@
 #define ASH_WM_ACTIVATION_CONTROLLER_H_
 #pragma once
 
+#include "ash/wm/scoped_observer.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/scoped_ptr.h"
@@ -75,6 +76,8 @@ class ASH_EXPORT ActivationController
   // change notifications causing activation.
   bool updating_activation_;
 
+  ScopedObserver<aura::Window, aura::WindowObserver> observer_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(ActivationController);
 };
 
diff --git a/ash/wm/scoped_observer.h b/ash/wm/scoped_observer.h
new file mode 100644
index 0000000..60aee19
--- /dev/null
+++ b/ash/wm/scoped_observer.h
@@ -0,0 +1,53 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef ASH_WM_SCOPED_OBSERVER_H_
+#define ASH_WM_SCOPED_OBSERVER_H_
+#pragma once
+
+#include <algorithm>
+#include <vector>
+
+#include "base/basictypes.h"
+
+namespace ash {
+namespace internal {
+
+// ScopedObserver is used to keep track of the set of sources an object has
+// attached itself to as an observer. When ScopedObserver is destroyed it
+// removes the object as an observer from all sources it has been added to.
+template <class Source, class Observer>
+class ScopedObserver {
+ public:
+  explicit ScopedObserver(Observer* observer) : observer_(observer) {}
+
+  ~ScopedObserver() {
+    for (size_t i = 0; i < sources_.size(); ++i)
+      sources_[i]->RemoveObserver(observer_);
+  }
+
+  // Adds the object passed to the constructor as an observer on |source|.
+  void Add(Source* source) {
+    sources_.push_back(source);
+    source->AddObserver(observer_);
+  }
+
+  // Removse the object passed to the constructor as an observer from |source|.
+  void Remove(Source* source) {
+    sources_.erase(std::find(sources_.begin(), sources_.end(), source));
+    source->RemoveObserver(observer_);
+  }
+
+ private:
+  Observer* observer_;
+
+  std::vector<Source*> sources_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedObserver);
+};
+
+}  // namespace internal
+}  // namespace ash
+
+#endif  // ASH_WM_SCOPED_OBSERVER_H_
diff --git a/ash/wm/shadow_controller.cc b/ash/wm/shadow_controller.cc
index 6220a2f..172300c 100644
--- a/ash/wm/shadow_controller.cc
+++ b/ash/wm/shadow_controller.cc
@@ -68,23 +68,20 @@ Shadow::Style GetShadowStyleForWindowLosingActive(
 
 }  // namespace
 
-ShadowController::ShadowController() {
+ShadowController::ShadowController()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(observer_manager_(this)) {
   aura::Env::GetInstance()->AddObserver(this);
   // Watch for window activation changes.
   Shell::GetRootWindow()->AddObserver(this);
 }
 
 ShadowController::~ShadowController() {
-  for (WindowShadowMap::const_iterator it = window_shadows_.begin();
-       it != window_shadows_.end(); ++it) {
-    it->first->RemoveObserver(this);
-  }
   Shell::GetRootWindow()->RemoveObserver(this);
   aura::Env::GetInstance()->RemoveObserver(this);
 }
 
 void ShadowController::OnWindowInitialized(aura::Window* window) {
-  window->AddObserver(this);
+  observer_manager_.Add(window);
   SetShadowType(window, GetShadowTypeFromWindow(window));
   HandlePossibleShadowVisibilityChange(window);
 }
@@ -113,6 +110,7 @@ void ShadowController::OnWindowBoundsChanged(aura::Window* window,
 
 void ShadowController::OnWindowDestroyed(aura::Window* window) {
   window_shadows_.erase(window);
+  observer_manager_.Remove(window);
 }
 
 bool ShadowController::ShouldShowShadowForWindow(aura::Window* window) const {
diff --git a/ash/wm/shadow_controller.h b/ash/wm/shadow_controller.h
index 2a880c9..73f97cf 100644
--- a/ash/wm/shadow_controller.h
+++ b/ash/wm/shadow_controller.h
@@ -8,12 +8,13 @@
 
 #include <map>
 
+#include "ash/ash_export.h"
+#include "ash/wm/scoped_observer.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/linked_ptr.h"
 #include "ui/aura/env_observer.h"
 #include "ui/aura/window_observer.h"
-#include "ash/ash_export.h"
 
 namespace aura {
 class Window;
@@ -84,6 +85,8 @@ class ASH_EXPORT ShadowController : public aura::EnvObserver,
 
   WindowShadowMap window_shadows_;
 
+  ScopedObserver<aura::Window, aura::WindowObserver> observer_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(ShadowController);
 };
 
diff --git a/ash/wm/video_detector.cc b/ash/wm/video_detector.cc
index 07cd4f7..e24a5958 100644
--- a/ash/wm/video_detector.cc
+++ b/ash/wm/video_detector.cc
@@ -51,17 +51,13 @@ class VideoDetector::WindowInfo {
   DISALLOW_COPY_AND_ASSIGN(WindowInfo);
 };
 
-VideoDetector::VideoDetector() {
+VideoDetector::VideoDetector()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(observer_manager_(this)) {
   aura::Env::GetInstance()->AddObserver(this);
 }
 
 VideoDetector::~VideoDetector() {
   aura::Env::GetInstance()->RemoveObserver(this);
-  for (WindowInfoMap::const_iterator it = window_infos_.begin();
-       it != window_infos_.end(); ++it) {
-    aura::Window* window = it->first;
-    window->RemoveObserver(this);
-  }
 }
 
 void VideoDetector::AddObserver(VideoDetectorObserver* observer) {
@@ -73,7 +69,7 @@ void VideoDetector::RemoveObserver(VideoDetectorObserver* observer) {
 }
 
 void VideoDetector::OnWindowInitialized(aura::Window* window) {
-  window->AddObserver(this);
+  observer_manager_.Add(window);
 }
 
 void VideoDetector::OnWindowPaintScheduled(aura::Window* window,
@@ -90,6 +86,7 @@ void VideoDetector::OnWindowPaintScheduled(aura::Window* window,
 
 void VideoDetector::OnWindowDestroyed(aura::Window* window) {
   window_infos_.erase(window);
+  observer_manager_.Remove(window);
 }
 
 void VideoDetector::MaybeNotifyObservers(aura::Window* window,
diff --git a/ash/wm/video_detector.h b/ash/wm/video_detector.h
index 3ac083c..3a8aaa1 100644
--- a/ash/wm/video_detector.h
+++ b/ash/wm/video_detector.h
@@ -9,6 +9,7 @@
 #include <map>
 
 #include "ash/ash_export.h"
+#include "ash/wm/scoped_observer.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/memory/linked_ptr.h"
@@ -92,6 +93,9 @@ class ASH_EXPORT VideoDetector : public aura::EnvObserver,
   // simulate the passage of time.
   base::TimeTicks now_for_test_;
 
+  internal::ScopedObserver<aura::Window, aura::WindowObserver>
+      observer_manager_;
+
   DISALLOW_COPY_AND_ASSIGN(VideoDetector);
 };
 
diff --git a/ash/wm/video_detector_unittest.cc b/ash/wm/video_detector_unittest.cc
index a9ce21d..e1861c7 100644
--- a/ash/wm/video_detector_unittest.cc
+++ b/ash/wm/video_detector_unittest.cc
@@ -43,7 +43,7 @@ class VideoDetectorTest : public AshTestBase {
   VideoDetectorTest() {}
   virtual ~VideoDetectorTest() {}
 
-  void SetUp() OVERRIDE {
+  virtual void SetUp() OVERRIDE {
     AshTestBase::SetUp();
     observer_.reset(new TestVideoDetectorObserver);
     detector_ = Shell::GetInstance()->video_detector();