Makes destruction of DesktopNativeWidgetAura saner (2)

Attempt 2 at this. This is same as last patch but I'm disabling unit
test on linux as ownership of X widget is different than HWND when
destroying parent with transients. Will file bug to track it down.

Prior to this patch DesktopNativeWidgetAura destruction keyed off
destruction of the window_ (not the rootwindow). This meant that by
the time we destroyed the Widget the RootWindow was still live. This
meant any children of the rootwindow, such as child widgets, were
alive as well. The general expectation is that by the time a Widget is
destroyed and child Widgets have also been destroyed.

This patch changes destruction so that we key off the
AcceleratedWidget going away. When that happens we destroy the
RootWindow. By doing this we ensure child Widgets have also been
destroyed.

BUG=288953
TEST=covered by test
R=ben@chromium.org
TBR=ben@chromium.org

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

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

3 files changed, 135 insertions, 25 deletions

diff --git a/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc b/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
index 698a342..1151b86 100644
--- a/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
+++ b/ui/views/widget/desktop_aura/desktop_native_widget_aura.cc
@@ -192,12 +192,6 @@ DesktopNativeWidgetAura::~DesktopNativeWidgetAura() {
     delete native_widget_delegate_;
   else
     CloseNow();
-
-  stacking_client_.reset();  // Uses root_window_ at destruction.
-
-  root_window_->RemoveRootWindowObserver(this);
-  root_window_.reset();  // Uses input_method_event_filter_ at destruction.
-  input_method_event_filter_.reset();
 }
 
 // static
@@ -207,10 +201,41 @@ DesktopNativeWidgetAura* DesktopNativeWidgetAura::ForWindow(
 }
 
 void DesktopNativeWidgetAura::OnHostClosed() {
+  // Don't invoke Widget::OnNativeWidgetDestroying(), its done by
+  // DesktopRootWindowHost.
+
+  // Make sure we don't still have capture. Otherwise CaptureController and
+  // RootWindow are left referencing a deleted Window.
+  {
+    aura::Window* capture_window =
+        capture_client_->capture_client()->GetCaptureWindow();
+    if (capture_window && root_window_->Contains(capture_window))
+      capture_window->ReleaseCapture();
+  }
+
+  // DesktopRootWindowHost owns the ActivationController which ShadowController
+  // references. Make sure we destroy ShadowController early on.
+  shadow_controller_.reset();
+  tooltip_manager_.reset();
+  scoped_tooltip_client_.reset();
+  if (window_modality_controller_) {
+    root_window_->RemovePreTargetHandler(window_modality_controller_.get());
+    window_modality_controller_.reset();
+  }
+
   root_window_event_filter_->RemoveHandler(input_method_event_filter_.get());
-  // This will, through a long list of callbacks, trigger |root_window_| going
-  // away. See OnWindowDestroyed()
-  delete window_;
+
+  stacking_client_.reset();  // Uses root_window_ at destruction.
+
+  root_window_->RemoveRootWindowObserver(this);
+  root_window_.reset();  // Uses input_method_event_filter_ at destruction.
+  // RootWindow owns |desktop_root_window_host_|.
+  desktop_root_window_host_ = NULL;
+  window_ = NULL;
+
+  native_widget_delegate_->OnNativeWidgetDestroyed();
+  if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
+    delete this;
 }
 
 void DesktopNativeWidgetAura::InstallInputMethodEventFilter(
@@ -680,7 +705,8 @@ void DesktopNativeWidgetAura::ClearNativeFocus() {
 }
 
 gfx::Rect DesktopNativeWidgetAura::GetWorkAreaBoundsInScreen() const {
-  return desktop_root_window_host_->GetWorkAreaBoundsInScreen();
+  return desktop_root_window_host_ ?
+      desktop_root_window_host_->GetWorkAreaBoundsInScreen() : gfx::Rect();
 }
 
 void DesktopNativeWidgetAura::SetInactiveRenderingDisabled(bool value) {
@@ -782,23 +808,12 @@ void DesktopNativeWidgetAura::OnDeviceScaleFactorChanged(
 }
 
 void DesktopNativeWidgetAura::OnWindowDestroying() {
-  // DesktopRootWindowHost owns the ActivationController which ShadowController
-  // references. Make sure we destroy ShadowController early on.
-  shadow_controller_.reset();
-  // The DesktopRootWindowHost implementation sends OnNativeWidgetDestroying().
-  tooltip_manager_.reset();
-  scoped_tooltip_client_.reset();
-  if (window_modality_controller_) {
-    root_window_->RemovePreTargetHandler(window_modality_controller_.get());
-    window_modality_controller_.reset();
-  }
+  // Cleanup happens in OnHostClosed().
 }
 
 void DesktopNativeWidgetAura::OnWindowDestroyed() {
-  window_ = NULL;
-  native_widget_delegate_->OnNativeWidgetDestroyed();
-  if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
-    delete this;
+  // Cleanup happens in OnHostClosed(). We own |window_| (indirectly by way of
+  // |root_window_|) so there should be no need to do any processing here.
 }
 
 void DesktopNativeWidgetAura::OnWindowTargetVisibilityChanged(bool visible) {
diff --git a/ui/views/widget/desktop_aura/desktop_native_widget_aura.h b/ui/views/widget/desktop_aura/desktop_native_widget_aura.h
index c1c97a0..9185248 100644
--- a/ui/views/widget/desktop_aura/desktop_native_widget_aura.h
+++ b/ui/views/widget/desktop_aura/desktop_native_widget_aura.h
@@ -59,7 +59,7 @@ class VIEWS_EXPORT DesktopNativeWidgetAura
 
   // Called by our DesktopRootWindowHost after it has deleted native resources;
   // this is the signal that we should start our shutdown.
-  void OnHostClosed();
+  virtual void OnHostClosed();
 
   // Installs the input method filter on |root|. This is intended to be invoked
   // by the DesktopRootWindowHost implementation during Init().
diff --git a/ui/views/widget/widget_unittest.cc b/ui/views/widget/widget_unittest.cc
index 1c8a7a2..538788a 100644
--- a/ui/views/widget/widget_unittest.cc
+++ b/ui/views/widget/widget_unittest.cc
@@ -1872,5 +1872,100 @@ TEST_F(WidgetTest, GetAllChildWidgets) {
   EXPECT_TRUE(std::equal(expected.begin(), expected.end(), widgets.begin()));
 }
 
+// Used by DestroyChildWidgetsInOrder. On destruction adds the supplied name to
+// a vector.
+class DestroyedTrackingView : public View {
+ public:
+  DestroyedTrackingView(const std::string& name,
+                        std::vector<std::string>* add_to)
+      : name_(name),
+        add_to_(add_to) {
+  }
+
+  virtual ~DestroyedTrackingView() {
+    add_to_->push_back(name_);
+  }
+
+ private:
+  const std::string name_;
+  std::vector<std::string>* add_to_;
+
+  DISALLOW_COPY_AND_ASSIGN(DestroyedTrackingView);
+};
+
+class WidgetChildDestructionTest : public WidgetTest {
+ public:
+  WidgetChildDestructionTest() {}
+
+  // Creates a top level and a child, destroys the child and verifies the views
+  // of the child are destroyed before the views of the parent.
+  void RunDestroyChildWidgetsTest(bool top_level_has_desktop_native_widget_aura,
+                                  bool child_has_desktop_native_widget_aura) {
+    // When a View is destroyed its name is added here.
+    std::vector<std::string> destroyed;
+
+    Widget* top_level = new Widget;
+    Widget::InitParams params =
+        CreateParams(views::Widget::InitParams::TYPE_WINDOW);
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+    if (top_level_has_desktop_native_widget_aura)
+      params.native_widget = new DesktopNativeWidgetAura(top_level);
+#endif
+    top_level->Init(params);
+    top_level->GetRootView()->AddChildView(
+        new DestroyedTrackingView("parent", &destroyed));
+    top_level->Show();
+
+    Widget* child = new Widget;
+    Widget::InitParams child_params =
+        CreateParams(views::Widget::InitParams::TYPE_POPUP);
+    child_params.parent = top_level->GetNativeView();
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+    if (child_has_desktop_native_widget_aura)
+      child_params.native_widget = new DesktopNativeWidgetAura(child);
+#endif
+    child->Init(child_params);
+    child->GetRootView()->AddChildView(
+        new DestroyedTrackingView("child", &destroyed));
+    child->Show();
+
+    // Should trigger destruction of the child too.
+    top_level->native_widget_private()->CloseNow();
+
+    // Child should be destroyed first.
+    ASSERT_EQ(2u, destroyed.size());
+    EXPECT_EQ("child", destroyed[0]);
+    EXPECT_EQ("parent", destroyed[1]);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(WidgetChildDestructionTest);
+};
+
+#if defined(USE_AURA) && !defined(OS_CHROMEOS)
+// See description of RunDestroyChildWidgetsTest(). Parent uses
+// DesktopNativeWidgetAura.
+TEST_F(WidgetChildDestructionTest,
+       DestroyChildWidgetsInOrderWithDesktopNativeWidget) {
+  RunDestroyChildWidgetsTest(true, false);
+}
+
+// TODO: test fails on linux as destroying parent X widget does not
+// automatically destroy transients. http://crbug.com/300020 .
+#if !defined(OS_LINUX)
+// See description of RunDestroyChildWidgetsTest(). Both parent and child use
+// DesktopNativeWidgetAura.
+TEST_F(WidgetChildDestructionTest,
+       DestroyChildWidgetsInOrderWithDesktopNativeWidgetForBoth) {
+  RunDestroyChildWidgetsTest(true, true);
+}
+#endif
+#endif
+
+// See description of RunDestroyChildWidgetsTest().
+TEST_F(WidgetChildDestructionTest, DestroyChildWidgetsInOrder) {
+  RunDestroyChildWidgetsTest(false, false);
+}
+
 }  // namespace test
 }  // namespace views