[GTK] delay reference count check in OwnedWidgetGtk

It's a common problem that a GtkWidget will be referenced by an event that's currently on the callstack. These ref holders ignore the "destroy" signal, so when we go to destroy the owned widget, the refcount == 1 DCHECK fails. This change delays that DCHECK until the current stack has unwound, so we don't have to keep running into this.

BUG=none
TEST=in debug builds, Escape in a constrained window doesn't DCHECK, dragging a tab doesn't cause a DCHECK, pressing the custom frame [x] in lucid doesn't DCHECK; in general, no additional DCHECKs

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

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

10 files changed, 66 insertions, 23 deletions

diff --git a/base/debug_util.h b/base/debug_util.h
index a7dba3a..5814781 100644
--- a/base/debug_util.h
+++ b/base/debug_util.h
@@ -44,6 +44,9 @@ class StackTrace {
   // Resolves backtrace to symbols and write to stream.
   void OutputToStream(std::ostream* os);
 
+  // Returns the stacktrace as a string.
+  std::string AsString();
+
  private:
   // From http://msdn.microsoft.com/en-us/library/bb204633.aspx,
   // the sum of FramesToSkip and FramesToCapture must be less than 63,
diff --git a/base/debug_util_posix.cc b/base/debug_util_posix.cc
index 7d7aca4..7e74aef 100644
--- a/base/debug_util_posix.cc
+++ b/base/debug_util_posix.cc
@@ -312,3 +312,12 @@ void StackTrace::OutputToStream(std::ostream* os) {
     (*os) << "\t" << trace_strings[i] << "\n";
   }
 }
+
+std::string StackTrace::AsString() {
+  std::vector<std::string> trace_strings;
+  std::string output;
+  GetBacktraceStrings(trace_, count_, &trace_strings, NULL);
+  for (size_t i = 0; i < trace_strings.size(); ++i)
+    output += "\t" + trace_strings[i] + "\n";
+  return output;
+}
diff --git a/chrome/browser/gtk/constrained_window_gtk.cc b/chrome/browser/gtk/constrained_window_gtk.cc
index b901083..bb53e68 100644
--- a/chrome/browser/gtk/constrained_window_gtk.cc
+++ b/chrome/browser/gtk/constrained_window_gtk.cc
@@ -70,11 +70,8 @@ TabContentsViewGtk* ConstrainedWindowGtk::ContainingView() {
 gboolean ConstrainedWindowGtk::OnKeyPress(GtkWidget* sender,
                                           GdkEventKey* key) {
   if (key->keyval == GDK_Escape) {
-    // Let the stack unwind so the event handler can release its ref
-    // on widget().
-    MessageLoop::current()->PostTask(FROM_HERE,
-        factory_.NewRunnableMethod(
-            &ConstrainedWindowGtk::CloseConstrainedWindow));
+    // This will delete us!
+    CloseConstrainedWindow();
     return TRUE;
   }
 
diff --git a/chrome/browser/gtk/gtk_theme_provider_unittest.cc b/chrome/browser/gtk/gtk_theme_provider_unittest.cc
index 1836b80..4779063 100644
--- a/chrome/browser/gtk/gtk_theme_provider_unittest.cc
+++ b/chrome/browser/gtk/gtk_theme_provider_unittest.cc
@@ -4,6 +4,7 @@
 
 #include <gtk/gtk.h>
 
+#include "base/message_loop.h"
 #include "chrome/browser/gtk/gtk_theme_provider.h"
 #include "chrome/browser/prefs/pref_service.h"
 #include "chrome/browser/profile.h"
@@ -36,6 +37,9 @@ class GtkThemeProviderTest : public testing::Test {
     provider_ = GtkThemeProvider::GetFrom(&profile_);
   }
 
+ private:
+  MessageLoop message_loop_;
+
  protected:
   TestingProfile profile_;
 
diff --git a/chrome/browser/gtk/owned_widget_gtk.cc b/chrome/browser/gtk/owned_widget_gtk.cc
index 2315371..c646cd7 100644
--- a/chrome/browser/gtk/owned_widget_gtk.cc
+++ b/chrome/browser/gtk/owned_widget_gtk.cc
@@ -6,7 +6,32 @@
 
 #include <gtk/gtk.h>
 
+#include "base/debug_util.h"
 #include "base/logging.h"
+#include "base/message_loop.h"
+
+namespace {
+
+class CheckWidgetRefCountTask : public Task {
+ public:
+  CheckWidgetRefCountTask(GtkWidget* widget, const std::string& stack)
+      : widget_(widget), stack_(stack) {}
+
+  virtual ~CheckWidgetRefCountTask() {}
+
+  virtual void Run() {
+    // NOTE: Assumes some implementation details about glib internals.
+    DCHECK_EQ(G_OBJECT(widget_)->ref_count, 1U) <<
+        " with Destroy() stack: \n" << stack_;
+    g_object_unref(widget_);
+  }
+
+ private:
+  GtkWidget* widget_;
+  std::string stack_;
+};
+
+}  // namespace
 
 OwnedWidgetGtk::~OwnedWidgetGtk() {
   DCHECK(!widget_) << "You must explicitly call OwnerWidgetGtk::Destroy().";
@@ -34,8 +59,16 @@ void OwnedWidgetGtk::Destroy() {
   widget_ = NULL;
   gtk_widget_destroy(widget);
 
+#ifndef NDEBUG
   DCHECK(!g_object_is_floating(widget));
-  // NOTE: Assumes some implementation details about glib internals.
-  DCHECK_EQ(G_OBJECT(widget)->ref_count, 1U);
-  g_object_unref(widget);
+
+  std::string stack(StackTrace().AsString());
+#else
+  std::string stack;
+#endif
+
+  // Make sure all other ref holders let go of their references (but delay
+  // the check because some ref holders won't let go immediately).
+  MessageLoop::current()->PostTask(
+      FROM_HERE, new CheckWidgetRefCountTask(widget, stack));
 }
diff --git a/chrome/browser/gtk/reload_button_gtk_unittest.cc b/chrome/browser/gtk/reload_button_gtk_unittest.cc
index 12384b6..13c152d 100644
--- a/chrome/browser/gtk/reload_button_gtk_unittest.cc
+++ b/chrome/browser/gtk/reload_button_gtk_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/message_loop.h"
 #include "chrome/browser/gtk/reload_button_gtk.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -51,6 +52,9 @@ class ReloadButtonGtkTest : public testing::Test {
  protected:
   ReloadButtonGtkTest() : reload_(NULL, NULL), peer_(&reload_) { }
 
+ private:
+  MessageLoop message_loop_;
+
  protected:
   ReloadButtonGtk reload_;
   ReloadButtonGtkPeer peer_;
diff --git a/chrome/browser/gtk/tabs/tab_gtk.cc b/chrome/browser/gtk/tabs/tab_gtk.cc
index 148e7ea..13e6dbb 100644
--- a/chrome/browser/gtk/tabs/tab_gtk.cc
+++ b/chrome/browser/gtk/tabs/tab_gtk.cc
@@ -388,12 +388,7 @@ void TabGtk::EndDrag(bool canceled) {
   // Make sure we only run EndDrag once by canceling any tasks that want
   // to call EndDrag.
   drag_end_factory_.RevokeAll();
-
-  // We must let gtk clean up after we handle the drag operation, otherwise
-  // there will be outstanding references to the drag widget when we try to
-  // destroy it.
-  MessageLoop::current()->PostTask(FROM_HERE,
-      destroy_factory_.NewRunnableMethod(&TabGtk::DestroyDragWidget));
+  DestroyDragWidget();
 
   if (last_mouse_down_) {
     gdk_event_free(last_mouse_down_);
diff --git a/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc b/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc
index 19f0806..70850a4 100644
--- a/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc
+++ b/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc
@@ -1,6 +1,7 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
+
 #include "chrome/browser/renderer_host/gtk_key_bindings_handler.h"
 
 #include <gdk/gdkkeysyms.h>
@@ -10,6 +11,7 @@
 
 #include "base/basictypes.h"
 #include "base/file_util.h"
+#include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "chrome/common/chrome_paths.h"
@@ -83,6 +85,7 @@ class GtkKeyBindingsHandlerTest : public testing::Test {
  protected:
   GtkWidget* window_;
   GtkKeyBindingsHandler* handler_;
+  MessageLoop message_loop_;
 };
 
 TEST_F(GtkKeyBindingsHandlerTest, MoveCursor) {
diff --git a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
index 6c49c81..a79129c 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
@@ -685,14 +685,6 @@ void RenderWidgetHostViewGtk::Destroy() {
     gtk_widget_destroy(gtk_widget_get_parent(view_.get()));
   }
 
-  // Remove |view_| from all containers now, so nothing else can hold a
-  // reference to |view_|'s widget except possibly a gtk signal handler if
-  // this code is currently executing within the context of a gtk signal
-  // handler.  Note that |view_| is still alive after this call.  It will be
-  // deallocated in the destructor.
-  // See http://www.crbug.com/11847 for details.
-  gtk_widget_destroy(view_.get());
-
   // The RenderWidgetHost's destruction led here, so don't call it.
   host_ = NULL;
 
diff --git a/chrome/browser/sync/glue/theme_util_unittest.cc b/chrome/browser/sync/glue/theme_util_unittest.cc
index fb6641f..4b12f8c6 100644
--- a/chrome/browser/sync/glue/theme_util_unittest.cc
+++ b/chrome/browser/sync/glue/theme_util_unittest.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/sync/glue/theme_util.h"
 
 #include "base/file_path.h"
+#include "base/message_loop.h"
 #include "base/values.h"
 #include "chrome/browser/profile.h"
 #include "chrome/browser/sync/protocol/theme_specifics.pb.h"
@@ -21,6 +22,8 @@ namespace {
 using ::testing::Return;
 
 class ThemeUtilTest : public testing::Test {
+ private:
+  MessageLoop message_loop_;
 };
 
 void MakeThemeExtension(Extension* extension,