Revert 61269 (broke lots of browser and ui tests, like on your try run) - [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

TBR=estade@chromium.org
Review URL: http://codereview.chromium.org/3564007

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

10 files changed, 23 insertions, 66 deletions

diff --git a/base/debug_util.h b/base/debug_util.h
index 5814781..a7dba3a 100644
--- a/base/debug_util.h
+++ b/base/debug_util.h
@@ -44,9 +44,6 @@ 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 7e74aef..7d7aca4 100644
--- a/base/debug_util_posix.cc
+++ b/base/debug_util_posix.cc
@@ -312,12 +312,3 @@ 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 bb53e68..b901083 100644
--- a/chrome/browser/gtk/constrained_window_gtk.cc
+++ b/chrome/browser/gtk/constrained_window_gtk.cc
@@ -70,8 +70,11 @@ TabContentsViewGtk* ConstrainedWindowGtk::ContainingView() {
 gboolean ConstrainedWindowGtk::OnKeyPress(GtkWidget* sender,
                                           GdkEventKey* key) {
   if (key->keyval == GDK_Escape) {
-    // This will delete us!
-    CloseConstrainedWindow();
+    // Let the stack unwind so the event handler can release its ref
+    // on widget().
+    MessageLoop::current()->PostTask(FROM_HERE,
+        factory_.NewRunnableMethod(
+            &ConstrainedWindowGtk::CloseConstrainedWindow));
     return TRUE;
   }
 
diff --git a/chrome/browser/gtk/gtk_theme_provider_unittest.cc b/chrome/browser/gtk/gtk_theme_provider_unittest.cc
index 4779063..1836b80 100644
--- a/chrome/browser/gtk/gtk_theme_provider_unittest.cc
+++ b/chrome/browser/gtk/gtk_theme_provider_unittest.cc
@@ -4,7 +4,6 @@
 
 #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"
@@ -37,9 +36,6 @@ 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 c646cd7..2315371 100644
--- a/chrome/browser/gtk/owned_widget_gtk.cc
+++ b/chrome/browser/gtk/owned_widget_gtk.cc
@@ -6,32 +6,7 @@
 
 #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().";
@@ -59,16 +34,8 @@ void OwnedWidgetGtk::Destroy() {
   widget_ = NULL;
   gtk_widget_destroy(widget);
 
-#ifndef NDEBUG
   DCHECK(!g_object_is_floating(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));
+  // NOTE: Assumes some implementation details about glib internals.
+  DCHECK_EQ(G_OBJECT(widget)->ref_count, 1U);
+  g_object_unref(widget);
 }
diff --git a/chrome/browser/gtk/reload_button_gtk_unittest.cc b/chrome/browser/gtk/reload_button_gtk_unittest.cc
index 13c152d..12384b6 100644
--- a/chrome/browser/gtk/reload_button_gtk_unittest.cc
+++ b/chrome/browser/gtk/reload_button_gtk_unittest.cc
@@ -2,7 +2,6 @@
 // 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"
 
@@ -52,9 +51,6 @@ 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 13e6dbb..148e7ea 100644
--- a/chrome/browser/gtk/tabs/tab_gtk.cc
+++ b/chrome/browser/gtk/tabs/tab_gtk.cc
@@ -388,7 +388,12 @@ 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();
-  DestroyDragWidget();
+
+  // 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));
 
   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 70850a4..19f0806 100644
--- a/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc
+++ b/chrome/browser/renderer_host/gtk_key_bindings_handler_unittest.cc
@@ -1,7 +1,6 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2009 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>
@@ -11,7 +10,6 @@
 
 #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"
@@ -85,7 +83,6 @@ 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 a79129c..6c49c81 100644
--- a/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
+++ b/chrome/browser/renderer_host/render_widget_host_view_gtk.cc
@@ -685,6 +685,14 @@ 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 4b12f8c6..fb6641f 100644
--- a/chrome/browser/sync/glue/theme_util_unittest.cc
+++ b/chrome/browser/sync/glue/theme_util_unittest.cc
@@ -5,7 +5,6 @@
 #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"
@@ -22,8 +21,6 @@ namespace {
 using ::testing::Return;
 
 class ThemeUtilTest : public testing::Test {
- private:
-  MessageLoop message_loop_;
 };
 
 void MakeThemeExtension(Extension* extension,