Add CHECK to diagnose the crash.

Observer OnWindowDestroying instead of OnWindowDestroyed
  OnWindowDestroying is much safer to use because it's called
  before removing children, being removed from parent, and
  other stuff.
  I don't think this will, just to rule out the possibility.

Misc cleaning up.

BUG=483491

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

Cr-Commit-Position: refs/heads/master@{#328685}

3 files changed, 40 insertions, 36 deletions

diff --git a/ash/wm/window_cycle_controller_unittest.cc b/ash/wm/window_cycle_controller_unittest.cc
index 5d8e9bd..6a19cbb 100644
--- a/ash/wm/window_cycle_controller_unittest.cc
+++ b/ash/wm/window_cycle_controller_unittest.cc
@@ -58,6 +58,11 @@ class WindowCycleControllerTest : public test::AshTestBase {
     return window;
   }
 
+  const WindowCycleList::WindowList& GetWindows(
+      WindowCycleController* controller) {
+    return controller->window_cycle_list()->windows();
+  }
+
  private:
   scoped_ptr<test::ShelfViewTestAPI> shelf_view_test_;
 
@@ -119,10 +124,10 @@ TEST_F(WindowCycleControllerTest, HandleCycleWindow) {
 
   // Window lists should return the topmost window in front.
   ASSERT_TRUE(controller->window_cycle_list());
-  ASSERT_EQ(3u, controller->window_cycle_list()->windows().size());
-  ASSERT_EQ(window0.get(), controller->window_cycle_list()->windows()[0]);
-  ASSERT_EQ(window1.get(), controller->window_cycle_list()->windows()[1]);
-  ASSERT_EQ(window2.get(), controller->window_cycle_list()->windows()[2]);
+  ASSERT_EQ(3u, GetWindows(controller).size());
+  ASSERT_EQ(window0.get(), GetWindows(controller)[0]);
+  ASSERT_EQ(window1.get(), GetWindows(controller)[1]);
+  ASSERT_EQ(window2.get(), GetWindows(controller)[2]);
 
   controller->StopCycling();
   EXPECT_TRUE(wm::IsActiveWindow(window1.get()));
@@ -269,10 +274,10 @@ TEST_F(WindowCycleControllerTest, AlwaysOnTopWindow) {
 
   // Window lists should return the topmost window in front.
   ASSERT_TRUE(controller->window_cycle_list());
-  ASSERT_EQ(3u, controller->window_cycle_list()->windows().size());
-  EXPECT_EQ(window0.get(), controller->window_cycle_list()->windows()[0]);
-  EXPECT_EQ(window2.get(), controller->window_cycle_list()->windows()[1]);
-  EXPECT_EQ(window1.get(), controller->window_cycle_list()->windows()[2]);
+  ASSERT_EQ(3u, GetWindows(controller).size());
+  EXPECT_EQ(window0.get(), GetWindows(controller)[0]);
+  EXPECT_EQ(window2.get(), GetWindows(controller)[1]);
+  EXPECT_EQ(window1.get(), GetWindows(controller)[2]);
 
   controller->StopCycling();
   EXPECT_TRUE(wm::IsActiveWindow(window2.get()));
@@ -314,11 +319,11 @@ TEST_F(WindowCycleControllerTest, AlwaysOnTopMultiWindow) {
 
   // Window lists should return the topmost window in front.
   ASSERT_TRUE(controller->window_cycle_list());
-  ASSERT_EQ(4u, controller->window_cycle_list()->windows().size());
-  EXPECT_EQ(window0.get(), controller->window_cycle_list()->windows()[0]);
-  EXPECT_EQ(window3.get(), controller->window_cycle_list()->windows()[1]);
-  EXPECT_EQ(window2.get(), controller->window_cycle_list()->windows()[2]);
-  EXPECT_EQ(window1.get(), controller->window_cycle_list()->windows()[3]);
+  ASSERT_EQ(4u, GetWindows(controller).size());
+  EXPECT_EQ(window0.get(), GetWindows(controller)[0]);
+  EXPECT_EQ(window3.get(), GetWindows(controller)[1]);
+  EXPECT_EQ(window2.get(), GetWindows(controller)[2]);
+  EXPECT_EQ(window1.get(), GetWindows(controller)[3]);
 
   controller->StopCycling();
   EXPECT_TRUE(wm::IsActiveWindow(window3.get()));
@@ -393,11 +398,11 @@ TEST_F(WindowCycleControllerTest, AlwaysOnTopMultipleRootWindows) {
 
   // Window lists should return the topmost window in front.
   ASSERT_TRUE(controller->window_cycle_list());
-  ASSERT_EQ(4u, controller->window_cycle_list()->windows().size());
-  EXPECT_EQ(window2.get(), controller->window_cycle_list()->windows()[0]);
-  EXPECT_EQ(window3.get(), controller->window_cycle_list()->windows()[1]);
-  EXPECT_EQ(window1.get(), controller->window_cycle_list()->windows()[2]);
-  EXPECT_EQ(window0.get(), controller->window_cycle_list()->windows()[3]);
+  ASSERT_EQ(4u, GetWindows(controller).size());
+  EXPECT_EQ(window2.get(), GetWindows(controller)[0]);
+  EXPECT_EQ(window3.get(), GetWindows(controller)[1]);
+  EXPECT_EQ(window1.get(), GetWindows(controller)[2]);
+  EXPECT_EQ(window0.get(), GetWindows(controller)[3]);
 
   controller->StopCycling();
   EXPECT_TRUE(wm::IsActiveWindow(window3.get()));
@@ -437,10 +442,10 @@ TEST_F(WindowCycleControllerTest, MostRecentlyUsed) {
 
   // Window lists should return the topmost window in front.
   ASSERT_TRUE(controller->window_cycle_list());
-  ASSERT_EQ(3u, controller->window_cycle_list()->windows().size());
-  EXPECT_EQ(window0.get(), controller->window_cycle_list()->windows()[0]);
-  EXPECT_EQ(window2.get(), controller->window_cycle_list()->windows()[1]);
-  EXPECT_EQ(window1.get(), controller->window_cycle_list()->windows()[2]);
+  ASSERT_EQ(3u, GetWindows(controller).size());
+  EXPECT_EQ(window0.get(), GetWindows(controller)[0]);
+  EXPECT_EQ(window2.get(), GetWindows(controller)[1]);
+  EXPECT_EQ(window1.get(), GetWindows(controller)[2]);
 
   controller->HandleCycleWindow(WindowCycleController::FORWARD);
   controller->StopCycling();
diff --git a/ash/wm/window_cycle_list.cc b/ash/wm/window_cycle_list.cc
index f442001..eef24ec 100644
--- a/ash/wm/window_cycle_list.cc
+++ b/ash/wm/window_cycle_list.cc
@@ -40,12 +40,10 @@ class ScopedShowWindow : public aura::WindowObserver {
   // Cancel restoring the window on going out of scope.
   void CancelRestore();
 
-  aura::Window* window() { return window_; }
-
+ private:
   // aura::WindowObserver:
   void OnWillRemoveWindow(aura::Window* window) override;
 
- private:
   // The window being shown.
   aura::Window* window_;
 
@@ -112,17 +110,16 @@ WindowCycleList::WindowCycleList(const WindowList& windows)
       current_index_(0) {
   ash::Shell::GetInstance()->mru_window_tracker()->SetIgnoreActivations(true);
 
-  for (WindowList::const_iterator i = windows_.begin(); i != windows_.end();
-       ++i) {
-    (*i)->AddObserver(this);
-  }
+  for (auto* window : windows_)
+    window->AddObserver(this);
 }
 
 WindowCycleList::~WindowCycleList() {
   ash::Shell::GetInstance()->mru_window_tracker()->SetIgnoreActivations(false);
-  for (WindowList::const_iterator i = windows_.begin(); i != windows_.end();
-       ++i) {
-    (*i)->RemoveObserver(this);
+  for (auto* window : windows_) {
+    // TODO(oshima): Remove this once crbug.com/483491 is fixed.
+    CHECK(window);
+    window->RemoveObserver(this);
   }
   if (showing_window_)
     showing_window_->CancelRestore();
@@ -155,11 +152,12 @@ void WindowCycleList::Step(WindowCycleController::Direction direction) {
   showing_window_->Show(windows_[current_index_]);
 }
 
-void WindowCycleList::OnWindowDestroyed(aura::Window* window) {
+void WindowCycleList::OnWindowDestroying(aura::Window* window) {
   window->RemoveObserver(this);
 
   WindowList::iterator i = std::find(windows_.begin(), windows_.end(), window);
-  DCHECK(i != windows_.end());
+  // TODO(oshima): Change this back to DCHECK once crbug.com/483491 is fixed.
+  CHECK(i != windows_.end());
   int removed_index = static_cast<int>(i - windows_.begin());
   windows_.erase(i);
   if (current_index_ > removed_index ||
diff --git a/ash/wm/window_cycle_list.h b/ash/wm/window_cycle_list.h
index 8c8b33c..1318c42 100644
--- a/ash/wm/window_cycle_list.h
+++ b/ash/wm/window_cycle_list.h
@@ -32,14 +32,15 @@ class ASH_EXPORT WindowCycleList : public aura::WindowObserver {
   // Cycles to the next or previous window based on |direction|.
   void Step(WindowCycleController::Direction direction);
 
+ private:
+  friend class WindowCycleControllerTest;
   const WindowList& windows() const { return windows_; }
 
- private:
   // aura::WindowObserver overrides:
   // There is a chance a window is destroyed, for example by JS code. We need to
   // take care of that even if it is not intended for the user to close a window
   // while window cycling.
-  void OnWindowDestroyed(aura::Window* window) override;
+  void OnWindowDestroying(aura::Window* window) override;
 
   // List of weak pointers to windows to use while cycling with the keyboard.
   // List is built when the user initiates the gesture (i.e. hits alt-tab the