Fixes use after free caused by delete in RootWindowController (2)

RootWindowController::CloseChildWindows() was explicitly deleting
windows. It should only do that for windows that are owned by the
parent, otherwise the window should be removed.

BUG=297028
TEST=covered by test now.
R=oshima@chromium.org
TBR=oshima@chromium.org

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

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

3 files changed, 76 insertions, 5 deletions

diff --git a/ash/root_window_controller.cc b/ash/root_window_controller.cc
index 9327cbd..a1b030e 100644
--- a/ash/root_window_controller.cc
+++ b/ash/root_window_controller.cc
@@ -437,7 +437,8 @@ void RootWindowController::CloseChildWindows() {
   workspace_controller_.reset();
   aura::client::SetTooltipClient(root_window_.get(), NULL);
 
-  // Remove all toplevel windows first.
+  // Explicitly destroy top level windows. We do this as during part of
+  // destruction such windows may query the RootWindow for state.
   std::queue<aura::Window*> non_toplevel_windows;
   non_toplevel_windows.push(root_window_.get());
   while (!non_toplevel_windows.empty()) {
@@ -446,6 +447,8 @@ void RootWindowController::CloseChildWindows() {
     aura::WindowTracker toplevel_windows;
     for (size_t i = 0; i < non_toplevel_window->children().size(); ++i) {
       aura::Window* child = non_toplevel_window->children()[i];
+      if (!child->owned_by_parent())
+        continue;
       if (child->delegate())
         toplevel_windows.Add(child);
       else
@@ -455,8 +458,14 @@ void RootWindowController::CloseChildWindows() {
       delete *toplevel_windows.windows().begin();
   }
   // And then remove the containers.
-  while (!root_window_->children().empty())
-    delete root_window_->children()[0];
+  while (!root_window_->children().empty()) {
+    aura::Window* window = root_window_->children()[0];
+    if (window->owned_by_parent()) {
+      delete window;
+    } else {
+      root_window_->RemoveChild(window);
+    }
+  }
 
   shelf_.reset(NULL);
 }
diff --git a/ash/root_window_controller_unittest.cc b/ash/root_window_controller_unittest.cc
index 4155c1a..cfab428 100644
--- a/ash/root_window_controller_unittest.cc
+++ b/ash/root_window_controller_unittest.cc
@@ -51,6 +51,7 @@ class TestDelegate : public views::WidgetDelegateView {
 
  private:
   bool system_modal_;
+
   DISALLOW_COPY_AND_ASSIGN(TestDelegate);
 };
 
@@ -528,6 +529,69 @@ TEST_F(RootWindowControllerTest, FocusBlockedWindow) {
   }
 }
 
+// Tracks whether OnWindowDestroying() has been invoked.
+class DestroyedWindowObserver : public aura::WindowObserver {
+ public:
+  DestroyedWindowObserver() : destroyed_(false), window_(NULL) {}
+  virtual ~DestroyedWindowObserver() {
+    Shutdown();
+  }
+
+  void SetWindow(Window* window) {
+    window_ = window;
+    window->AddObserver(this);
+  }
+
+  bool destroyed() const { return destroyed_; }
+
+  // WindowObserver overrides:
+  virtual void OnWindowDestroying(Window* window) OVERRIDE {
+    destroyed_ = true;
+    Shutdown();
+  }
+
+ private:
+  void Shutdown() {
+    if (!window_)
+      return;
+    window_->RemoveObserver(this);
+    window_ = NULL;
+  }
+
+  bool destroyed_;
+  Window* window_;
+
+  DISALLOW_COPY_AND_ASSIGN(DestroyedWindowObserver);
+};
+
+// Verifies shutdown doesn't delete windows that are not owned by the parent.
+TEST_F(RootWindowControllerTest, DontDeleteWindowsNotOwnedByParent) {
+  DestroyedWindowObserver observer1;
+  aura::test::TestWindowDelegate delegate1;
+  aura::Window* window1 = new aura::Window(&delegate1);
+  window1->SetType(aura::client::WINDOW_TYPE_CONTROL);
+  window1->set_owned_by_parent(false);
+  observer1.SetWindow(window1);
+  window1->Init(ui::LAYER_NOT_DRAWN);
+  window1->SetDefaultParentByRootWindow(
+      Shell::GetInstance()->GetPrimaryRootWindow(), gfx::Rect());
+
+  DestroyedWindowObserver observer2;
+  aura::Window* window2 = new aura::Window(NULL);
+  window2->set_owned_by_parent(false);
+  observer2.SetWindow(window2);
+  window2->Init(ui::LAYER_NOT_DRAWN);
+  Shell::GetInstance()->GetPrimaryRootWindow()->AddChild(window2);
+
+  Shell::GetInstance()->GetPrimaryRootWindowController()->CloseChildWindows();
+
+  ASSERT_FALSE(observer1.destroyed());
+  delete window1;
+
+  ASSERT_FALSE(observer2.destroyed());
+  delete window2;
+}
+
 typedef test::NoSessionAshTestBase NoSessionRootWindowControllerTest;
 
 // Make sure that an event handler exists for entire display area.
diff --git a/ash/wm/frame_painter.cc b/ash/wm/frame_painter.cc
index f92bc0b..3368001 100644
--- a/ash/wm/frame_painter.cc
+++ b/ash/wm/frame_painter.cc
@@ -177,8 +177,6 @@ bool IsSoloWindowHeaderCandidate(aura::Window* window) {
 // a transparent solo-window header.
 std::vector<Window*> GetWindowsForSoloHeaderUpdate(RootWindow* root_window) {
   std::vector<Window*> windows;
-  // During shutdown there may not be a workspace controller. In that case
-  // we don't care about updating any windows.
   // Avoid memory allocations for typical window counts.
   windows.reserve(16);
   // Collect windows from the desktop.