views: Fix a Widget destruction crash.

For WIDGET_OWNS_NATIVE_WIDGET widget, we destroy root view before we delete
|native_widget_|. If a WidgetDelegateView is in root view's hierarchy, the
native widget destruction would crash because |widget_delegate_| is accessed
in OnNativeWidgetDestroying but it is deleted with root view already.

This CL marks WidgetDelegateView as owned by client so that it does not go
away with the root view hierarchy and be deleted on DeleteDelegate.

BUG=164791
TEST=Covered with a new unit test.

R=ben@chromium.org


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

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

6 files changed, 45 insertions, 4 deletions

diff --git a/ui/views/test/child_modal_window.cc b/ui/views/test/child_modal_window.cc
index ebf1ac2..04d5df5 100644
--- a/ui/views/test/child_modal_window.cc
+++ b/ui/views/test/child_modal_window.cc
@@ -173,6 +173,8 @@ void ChildModalParent::DeleteDelegate() {
     child_->Close();
     child_ = NULL;
   }
+
+  delete this;
 }
 
 void ChildModalParent::Layout() {
diff --git a/ui/views/widget/widget_delegate.cc b/ui/views/widget/widget_delegate.cc
index 7a42ec4..cb7b469d 100644
--- a/ui/views/widget/widget_delegate.cc
+++ b/ui/views/widget/widget_delegate.cc
@@ -160,11 +160,17 @@ bool WidgetDelegate::ShouldDescendIntoChildForEventHandling(
 // WidgetDelegateView:
 
 WidgetDelegateView::WidgetDelegateView() {
+  // A WidgetDelegate should be deleted on DeleteDelegate.
+  set_owned_by_client();
 }
 
 WidgetDelegateView::~WidgetDelegateView() {
 }
 
+void WidgetDelegateView::DeleteDelegate() {
+  delete this;
+}
+
 Widget* WidgetDelegateView::GetWidget() {
   return View::GetWidget();
 }
diff --git a/ui/views/widget/widget_delegate.h b/ui/views/widget/widget_delegate.h
index 640ccbc..f7b5ffc 100644
--- a/ui/views/widget/widget_delegate.h
+++ b/ui/views/widget/widget_delegate.h
@@ -164,13 +164,15 @@ class VIEWS_EXPORT WidgetDelegate {
 
 // A WidgetDelegate implementation that is-a View. Used to override GetWidget()
 // to call View's GetWidget() for the common case where a WidgetDelegate
-// implementation is-a View.
+// implementation is-a View. Note that WidgetDelegateView is not owned by
+// view's hierarchy and is expected to be deleted on DeleteDelegate call.
 class VIEWS_EXPORT WidgetDelegateView : public WidgetDelegate, public View {
  public:
   WidgetDelegateView();
   virtual ~WidgetDelegateView();
 
   // Overridden from WidgetDelegate:
+  virtual void DeleteDelegate() OVERRIDE;
   virtual Widget* GetWidget() OVERRIDE;
   virtual const Widget* GetWidget() const OVERRIDE;
 
diff --git a/ui/views/widget/widget_unittest.cc b/ui/views/widget/widget_unittest.cc
index c23fd0f..45c34c1 100644
--- a/ui/views/widget/widget_unittest.cc
+++ b/ui/views/widget/widget_unittest.cc
@@ -826,6 +826,29 @@ TEST_F(WidgetOwnershipTest,
   EXPECT_TRUE(state.native_widget_deleted);
 }
 
+// Widget owns its NativeWidget and has a WidgetDelegateView as its contents.
+TEST_F(WidgetOwnershipTest,
+       Ownership_WidgetOwnsNativeWidgetWithWithWidgetDelegateView) {
+  OwnershipTestState state;
+
+  WidgetDelegateView* delegate_view = new WidgetDelegateView;
+
+  scoped_ptr<Widget> widget(new OwnershipTestWidget(&state));
+  Widget::InitParams params = CreateParams(Widget::InitParams::TYPE_POPUP);
+  params.native_widget =
+      new OwnershipTestNativeWidgetPlatform(widget.get(), &state);
+  params.ownership = Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;
+  params.delegate = delegate_view;
+  widget->Init(params);
+  widget->SetContentsView(delegate_view);
+
+  // Now delete the Widget. There should be no crash or use-after-free.
+  widget.reset();
+
+  EXPECT_TRUE(state.widget_deleted);
+  EXPECT_TRUE(state.native_widget_deleted);
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // Widget observer tests.
 //
@@ -902,7 +925,6 @@ class WidgetObserverTest : public WidgetTest, public WidgetObserver {
   const Widget* widget_bounds_changed() const { return widget_bounds_changed_; }
 
  private:
-
   Widget* active_;
 
   Widget* widget_closed_;
diff --git a/ui/views/window/dialog_delegate.cc b/ui/views/window/dialog_delegate.cc
index 33a1b9a..711554d 100644
--- a/ui/views/window/dialog_delegate.cc
+++ b/ui/views/window/dialog_delegate.cc
@@ -151,7 +151,10 @@ ui::AccessibilityTypes::Role DialogDelegate::GetAccessibleWindowRole() const {
 ////////////////////////////////////////////////////////////////////////////////
 // DialogDelegateView:
 
-DialogDelegateView::DialogDelegateView() {}
+DialogDelegateView::DialogDelegateView() {
+  // A WidgetDelegate should be deleted on DeleteDelegate.
+  set_owned_by_client();
+}
 
 DialogDelegateView::~DialogDelegateView() {}
 
@@ -162,6 +165,10 @@ Widget* DialogDelegateView::CreateDialogWidget(DialogDelegateView* dialog,
   return CreateDialogWidgetImpl(dialog, context, parent);
 }
 
+void DialogDelegateView::DeleteDelegate() {
+  delete this;
+}
+
 Widget* DialogDelegateView::GetWidget() {
   return View::GetWidget();
 }
diff --git a/ui/views/window/dialog_delegate.h b/ui/views/window/dialog_delegate.h
index 01ef5eb..389f210 100644
--- a/ui/views/window/dialog_delegate.h
+++ b/ui/views/window/dialog_delegate.h
@@ -112,7 +112,8 @@ class VIEWS_EXPORT DialogDelegate : public WidgetDelegate {
 
 // A DialogDelegate implementation that is-a View. Used to override GetWidget()
 // to call View's GetWidget() for the common case where a DialogDelegate
-// implementation is-a View.
+// implementation is-a View. Note that DialogDelegateView is not owned by
+// view's hierarchy and is expected to be deleted on DeleteDelegate call.
 class VIEWS_EXPORT DialogDelegateView : public DialogDelegate,
                                         public View {
  public:
@@ -125,6 +126,7 @@ class VIEWS_EXPORT DialogDelegateView : public DialogDelegate,
                                     gfx::NativeWindow parent);
 
   // Overridden from DialogDelegate:
+  virtual void DeleteDelegate() OVERRIDE;
   virtual Widget* GetWidget() OVERRIDE;
   virtual const Widget* GetWidget() const OVERRIDE;
   virtual View* GetContentsView() OVERRIDE;