Reland 179231

> 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

BUG=164791,172842
TEST=No memory leak as in 172842.

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

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

7 files changed, 46 insertions, 4 deletions

diff --git a/ui/oak/oak_window.cc b/ui/oak/oak_window.cc
index 832a7a9..7cf7dff 100644
--- a/ui/oak/oak_window.cc
+++ b/ui/oak/oak_window.cc
@@ -68,6 +68,7 @@ bool OakWindow::ShouldShowWindowIcon() const {
 
 void OakWindow::DeleteDelegate() {
   instance = NULL;
+  delete this;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
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 f42b877..6e6ed07 100644
--- a/ui/views/widget/widget_unittest.cc
+++ b/ui/views/widget/widget_unittest.cc
@@ -827,6 +827,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.
 //
@@ -903,7 +926,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;