Fix leak of GtkWidgets in NativeViewHost, and in doing so fix a crash on shutdown due to a check in OwnedWidgetGtk for unbalanced refcnt.

The NativeViewHostGtk now owns the GtkWidget attached to it for the lifetime of its attachment. This removes my crazy ownership games from before and makes it a lot simpler.

BUG=none
TEST=none

Review URL: http://codereview.chromium.org/159751

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

2 files changed, 18 insertions, 19 deletions

diff --git a/views/controls/native/native_view_host_gtk.cc b/views/controls/native/native_view_host_gtk.cc
index 6143113..5e91d18 100644
--- a/views/controls/native/native_view_host_gtk.cc
+++ b/views/controls/native/native_view_host_gtk.cc
@@ -47,6 +47,10 @@ void NativeViewHostGtk::NativeViewAttached() {
   // Always layout though.
   host_->Layout();
 
+  // We own the native view as long as it's attached, so that we can safely
+  // reparent it in multiple passes.
+  gtk_widget_ref(host_->native_view());
+
   // TODO(port): figure out focus.
 }
 
@@ -60,6 +64,9 @@ void NativeViewHostGtk::NativeViewDetaching() {
   // TODO(port): focus.
   // FocusManager::UninstallFocusSubclass(native_view());
   installed_clip_ = false;
+
+  // Release ownership back to the caller.
+  gtk_widget_unref(host_->native_view());
 }
 
 void NativeViewHostGtk::AddedToWidget() {
@@ -87,8 +94,6 @@ void NativeViewHostGtk::RemovedFromWidget() {
   if (!host_->native_view())
     return;
 
-  // TODO(beng): We leak host_->native_view() here. Fix: make all widgets not be
-  //             refcounted.
   DestroyFixed();
 }
 
@@ -156,7 +161,7 @@ void NativeViewHostGtk::SetFocus() {
 // NativeViewHostGtk, private:
 
 void NativeViewHostGtk::CreateFixed(bool needs_window) {
-  bool native_view_addrefed = DestroyFixed();
+  DestroyFixed();
 
   fixed_ = gtk_fixed_new();
   gtk_fixed_set_has_window(GTK_FIXED(fixed_), needs_window);
@@ -168,28 +173,23 @@ void NativeViewHostGtk::CreateFixed(bool needs_window) {
     widget_gtk->AddChild(fixed_);
   if (host_->native_view())
     gtk_container_add(GTK_CONTAINER(fixed_), host_->native_view());
-  if (native_view_addrefed)
-    gtk_widget_unref(host_->native_view());
 }
 
-bool NativeViewHostGtk::DestroyFixed() {
-  bool native_view_addrefed = false;
+void NativeViewHostGtk::DestroyFixed() {
   if (!fixed_)
-    return native_view_addrefed;
+    return;
 
   gtk_widget_hide(fixed_);
   GetHostWidget()->RemoveChild(fixed_);
 
   if (host_->native_view()) {
-    // We can't allow the hosted NativeView's refcount to drop to zero.
-    gtk_widget_ref(host_->native_view());
-    native_view_addrefed = true;
+    // We can safely remove the widget from its container since we own the
+    // widget from the moment it is attached.
     gtk_container_remove(GTK_CONTAINER(fixed_), host_->native_view());
   }
 
   gtk_widget_destroy(fixed_);
   fixed_ = NULL;
-  return native_view_addrefed;
 }
 
 WidgetGtk* NativeViewHostGtk::GetHostWidget() const {
diff --git a/views/controls/native/native_view_host_gtk.h b/views/controls/native/native_view_host_gtk.h
index 8292d52..70cf438 100644
--- a/views/controls/native/native_view_host_gtk.h
+++ b/views/controls/native/native_view_host_gtk.h
@@ -17,6 +17,10 @@ namespace views {
 class View;
 class WidgetGtk;
 
+// Note that the NativeViewHostGtk assumes ownership of the GtkWidget attached
+// to it for the duration of its attachment. This is so the NativeViewHostGtk
+// can safely reparent the GtkWidget in multiple passes without having to worry
+// about the GtkWidget's refcnt falling to 0.
 class NativeViewHostGtk : public NativeViewHostWrapper {
  public:
   explicit NativeViewHostGtk(NativeViewHost* host);
@@ -42,13 +46,8 @@ class NativeViewHostGtk : public NativeViewHostWrapper {
   // regardless of whether or not there's an X Window. It's not that hard.
   void CreateFixed(bool needs_window);
 
-  // DestroyFixed returns true if an associated GtkWidget was addref'ed.
-  // It does this because when the fixed is destroyed the refcount for the
-  // contained GtkWidget is decremented, which may cause it to be destroyed
-  // which we do not want. If this function returns true, the caller is
-  // responsible for unrefing the GtkWidget after it has been added to the new
-  // container.
-  bool DestroyFixed();
+  // Destroys the GtkFixed that performs clipping on our hosted GtkWidget.
+  void DestroyFixed();
 
   WidgetGtk* GetHostWidget() const;