Removing DeleteSoon() from WigetGtk so it behaves like WidgetWin

This is a retry of http://codereview.chromium.org/7002029/ after it has been reverted because of the failing test BrowserTest.CloseWithAppMenuOpen (failed only on buildbot)

BUG=chromium-os:15129
TEST=tests


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

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

8 files changed, 219 insertions, 19 deletions

diff --git a/ui/base/gtk/g_object_destructor_filo.cc b/ui/base/gtk/g_object_destructor_filo.cc
new file mode 100644
index 0000000..3e37f60
--- /dev/null
+++ b/ui/base/gtk/g_object_destructor_filo.cc
@@ -0,0 +1,85 @@
+// Copyright (c) 2011 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 "ui/base/gtk/g_object_destructor_filo.h"
+
+#include <glib-object.h>
+#include "base/logging.h"
+
+namespace ui {
+
+GObjectDestructorFILO::GObjectDestructorFILO() {
+}
+
+GObjectDestructorFILO::~GObjectDestructorFILO() {
+  // Probably CHECK(handler_map_.empty()) would look natural here. But
+  // some tests (some views_unittests) violate this assertion.
+}
+
+// static
+GObjectDestructorFILO* GObjectDestructorFILO::GetInstance() {
+  return Singleton<GObjectDestructorFILO>::get();
+}
+
+void GObjectDestructorFILO::Connect(
+    GObject* object, DestructorHook callback, void* context) {
+  const Hook hook(object, callback, context);
+  HandlerMap::iterator iter = handler_map_.find(object);
+  if (iter == handler_map_.end()) {
+    g_object_weak_ref(object, WeakNotifyThunk, this);
+    handler_map_[object].push_front(hook);
+  } else {
+    iter->second.push_front(hook);
+  }
+}
+
+void GObjectDestructorFILO::Disconnect(
+    GObject* object, DestructorHook callback, void* context) {
+  HandlerMap::iterator iter = handler_map_.find(object);
+  if (iter == handler_map_.end()) {
+    LOG(DFATAL) << "Unable to disconnect destructor hook for object " << object
+                << ": hook not found (" << callback << ", " << context << ").";
+    return;
+  }
+  HandlerList& dtors = iter->second;
+  if (dtors.empty()) {
+    LOG(DFATAL) << "Destructor list is empty for specified object " << object
+                << " Maybe it is being executed?";
+    return;
+  }
+  if (!dtors.front().equal(object, callback, context)) {
+    LOG(WARNING) << "Destructors should be unregistered the reverse order they "
+                 << "were registered. But for object " << object << " "
+                 << "deleted hook is "<< context << ", the last queued hook is "
+                 << dtors.front().context;
+  }
+  for (HandlerList::iterator i = dtors.begin(); i != dtors.end(); ++i) {
+    if (i->equal(object, callback, context)) {
+      dtors.erase(i);
+      break;
+    }
+  }
+  if (dtors.empty()) {
+    g_object_weak_unref(object, WeakNotifyThunk, this);
+    handler_map_.erase(iter);
+  }
+}
+
+void GObjectDestructorFILO::WeakNotify(GObject* where_the_object_was) {
+  HandlerMap::iterator iter = handler_map_.find(where_the_object_was);
+  DCHECK(iter != handler_map_.end());
+  DCHECK(!iter->second.empty());
+
+  // Save destructor list for given object into local copy to avoid reentrancy
+  // problem: if callee wants to modify the caller list.
+  HandlerList dtors;
+  iter->second.swap(dtors);
+  handler_map_.erase(iter);
+
+  // Execute hooks in local list in FILO order.
+  for (HandlerList::iterator i = dtors.begin(); i != dtors.end(); ++i)
+    i->callback(i->context, where_the_object_was);
+}
+
+}  // napespace ui
diff --git a/ui/base/gtk/g_object_destructor_filo.h b/ui/base/gtk/g_object_destructor_filo.h
new file mode 100644
index 0000000..90e7597
--- /dev/null
+++ b/ui/base/gtk/g_object_destructor_filo.h
@@ -0,0 +1,89 @@
+// Copyright (c) 2011 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.
+
+#ifndef UI_BASE_GTK_G_OBJECT_DESTRUCTOR_FILO_H_
+#define UI_BASE_GTK_G_OBJECT_DESTRUCTOR_FILO_H_
+#pragma once
+
+#include <glib.h>
+#include <map>
+#include <list>
+
+#include "base/memory/singleton.h"
+
+typedef struct _GObject GObject;
+
+namespace ui {
+
+// This class hooks calls to g_object_weak_ref()/unref() and executes them in
+// FILO order. This is important if there are several hooks to the single object
+// (set up at different levels of class hierarchy) and the lowest hook (set up
+// first) is deleting self - it must be called last (among hooks for the given
+// object). Unfortunately Glib does not provide this guarantee.
+//
+// Use it as follows:
+//
+// static void OnDestroyedThunk(gpointer data, GObject *where_the_object_was) {
+//   reinterpret_cast<MyClass*>(data)->OnDestroyed(where_the_object_was);
+// }
+// void MyClass::OnDestroyed(GObject *where_the_object_was) {
+//   destroyed_ = true;
+//   delete this;
+// }
+// MyClass::Init() {
+//   ...
+//   ui::GObjectDestructorFILO::GetInstance()->Connect(
+//       G_OBJECT(my_widget), &OnDestroyedThunk, this);
+// }
+// MyClass::~MyClass() {
+//   if (!destroyed_) {
+//     ui::GObjectDestructorFILO::GetInstance()->Disconnect(
+//         G_OBJECT(my_widget), &OnDestroyedThunk, this);
+//   }
+// }
+//
+// TODO(glotov): Probably worth adding ScopedGObjectDtor<T>.
+//
+// This class is a singleton. Not thread safe. Must be called within UI thread.
+class GObjectDestructorFILO {
+ public:
+  typedef void (*DestructorHook)(void* context, GObject* where_the_object_was);
+
+  static GObjectDestructorFILO* GetInstance();
+  void Connect(GObject* object, DestructorHook callback, void* context);
+  void Disconnect(GObject* object, DestructorHook callback, void* context);
+
+ private:
+  struct Hook {
+    Hook(GObject* o, DestructorHook cb, void* ctx)
+        : object(o), callback(cb), context(ctx) {
+    }
+    bool equal(GObject* o, DestructorHook cb, void* ctx) const {
+      return object == o && callback == cb && context == ctx;
+    }
+    GObject* object;
+    DestructorHook callback;
+    void* context;
+  };
+  typedef std::list<Hook> HandlerList;
+  typedef std::map<GObject*, HandlerList> HandlerMap;
+
+  GObjectDestructorFILO();
+  ~GObjectDestructorFILO();
+  friend struct DefaultSingletonTraits<GObjectDestructorFILO>;
+
+  void WeakNotify(GObject* where_the_object_was);
+  static void WeakNotifyThunk(gpointer data, GObject* where_the_object_was) {
+    reinterpret_cast<GObjectDestructorFILO*>(data)->WeakNotify(
+        where_the_object_was);
+  }
+
+  HandlerMap handler_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(GObjectDestructorFILO);
+};
+
+}  // namespace ui
+
+#endif  // UI_BASE_GTK_G_OBJECT_DESTRUCTOR_FILO_H_
diff --git a/ui/base/gtk/gtk_signal_registrar.cc b/ui/base/gtk/gtk_signal_registrar.cc
index c6b170f..9a5846e 100644
--- a/ui/base/gtk/gtk_signal_registrar.cc
+++ b/ui/base/gtk/gtk_signal_registrar.cc
@@ -7,6 +7,7 @@
 #include <glib-object.h>
 
 #include "base/logging.h"
+#include "ui/base/gtk/g_object_destructor_filo.h"
 
 namespace ui {
 
@@ -17,7 +18,8 @@ GtkSignalRegistrar::~GtkSignalRegistrar() {
   for (HandlerMap::iterator list_iter = handler_lists_.begin();
        list_iter != handler_lists_.end(); ++list_iter) {
     GObject* object = list_iter->first;
-    g_object_weak_unref(object, WeakNotifyThunk, this);
+    GObjectDestructorFILO::GetInstance()->Disconnect(
+        object, WeakNotifyThunk, this);
 
     HandlerList& handlers = list_iter->second;
     for (HandlerList::iterator ids_iter = handlers.begin();
@@ -51,7 +53,8 @@ glong GtkSignalRegistrar::ConnectInternal(gpointer instance,
 
   HandlerMap::iterator iter = handler_lists_.find(object);
   if (iter == handler_lists_.end()) {
-    g_object_weak_ref(object, WeakNotifyThunk, this);
+    GObjectDestructorFILO::GetInstance()->Connect(
+        object, WeakNotifyThunk, this);
     handler_lists_[object] = HandlerList();
     iter = handler_lists_.find(object);
   }
diff --git a/ui/ui_base.gypi b/ui/ui_base.gypi
index 9facff0..1cb4c6e 100644
--- a/ui/ui_base.gypi
+++ b/ui/ui_base.gypi
@@ -21,8 +21,8 @@
       ],
       'sources': [
         'base/accessibility/accessibility_types.h',
-        'base/accessibility/accessible_view_state.h',
         'base/accessibility/accessible_view_state.cc',
+        'base/accessibility/accessible_view_state.h',
         'base/animation/animation.cc',
         'base/animation/animation.h',
         'base/animation/animation_container.cc',
@@ -49,12 +49,14 @@
         'base/clipboard/clipboard_win.cc',
         'base/clipboard/scoped_clipboard_writer.cc',
         'base/clipboard/scoped_clipboard_writer.h',
+        'base/gtk/g_object_destructor_filo.cc',
+        'base/gtk/g_object_destructor_filo.h',
+        'base/gtk/gtk_im_context_util.cc',
+        'base/gtk/gtk_im_context_util.h',
         'base/ime/composition_text.cc',
         'base/ime/composition_text.h',
         'base/ime/composition_underline.h',
         'base/ime/text_input_type.h',
-        'base/gtk/gtk_im_context_util.cc',
-        'base/gtk/gtk_im_context_util.h',
         'base/range/range.cc',
         'base/range/range.h',
         'base/range/range.mm',
diff --git a/views/widget/native_widget_gtk.cc b/views/widget/native_widget_gtk.cc
index d4295d5..186f8ef 100644
--- a/views/widget/native_widget_gtk.cc
+++ b/views/widget/native_widget_gtk.cc
@@ -20,15 +20,16 @@
 #include "ui/base/dragdrop/drag_drop_types.h"
 #include "ui/base/dragdrop/os_exchange_data.h"
 #include "ui/base/dragdrop/os_exchange_data_provider_gtk.h"
+#include "ui/base/gtk/g_object_destructor_filo.h"
 #include "ui/base/gtk/gtk_windowing.h"
 #include "ui/base/gtk/scoped_handle_gtk.h"
 #include "ui/base/x/x11_util.h"
 #include "ui/gfx/canvas_skia_paint.h"
 #include "ui/gfx/path.h"
-#include "views/views_delegate.h"
 #include "views/controls/textfield/native_textfield_views.h"
 #include "views/focus/view_storage.h"
 #include "views/ime/input_method_gtk.h"
+#include "views/views_delegate.h"
 #include "views/widget/drop_target_gtk.h"
 #include "views/widget/gtk_views_fixed.h"
 #include "views/widget/gtk_views_window.h"
@@ -321,11 +322,17 @@ NativeWidgetGtk::NativeWidgetGtk(internal::NativeWidgetDelegate* delegate)
 }
 
 NativeWidgetGtk::~NativeWidgetGtk() {
+  if (widget_) {
+    ui::GObjectDestructorFILO::GetInstance()->Disconnect(
+        G_OBJECT(widget_), &OnDestroyedThunk, this);
+    if (ownership_ != Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
+      CloseNow();
+  }
+  DCHECK(ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET ||
+         widget_ == NULL);
   // We need to delete the input method before calling DestroyRootView(),
   // because it'll set focus_manager_ to NULL.
   input_method_.reset();
-  DCHECK(ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET ||
-         widget_ == NULL);
   if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
     delete delegate_;
   // TODO(altimofeev): investigate why OnDestroy could not be called.
@@ -720,6 +727,9 @@ void NativeWidgetGtk::InitNativeWidget(const Widget::InitParams& params) {
   g_signal_connect(G_OBJECT(widget_), "window-state-event",
                    G_CALLBACK(&OnWindowStateEventThunk), this);
 
+  ui::GObjectDestructorFILO::GetInstance()->Connect(
+      G_OBJECT(widget_), &OnDestroyedThunk, this);
+
   tooltip_manager_.reset(new TooltipManagerGtk(this));
 
   // Register for tooltips.
@@ -923,10 +933,10 @@ void NativeWidgetGtk::MoveAbove(gfx::NativeView native_view) {
 }
 
 void NativeWidgetGtk::SetShape(gfx::NativeRegion region) {
-  DCHECK(widget_);
-  DCHECK(widget_->window);
-  gdk_window_shape_combine_region(widget_->window, region, 0, 0);
-  gdk_region_destroy(region);
+  if (widget_ && widget_->window) {
+    gdk_window_shape_combine_region(widget_->window, region, 0, 0);
+    gdk_region_destroy(region);
+  }
 }
 
 void NativeWidgetGtk::Close() {
@@ -1373,13 +1383,13 @@ void NativeWidgetGtk::OnDestroy(GtkWidget* object) {
     ActiveWindowWatcherX::RemoveObserver(this);
   // Note that this handler is hooked to GtkObject::destroy.
   // NULL out pointers here since we might still be in an observer list
-  // until delstion happens.
+  // until deletion happens.
   widget_ = window_contents_ = NULL;
-  if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET) {
-    // Delays the deletion of this NativeWidgetGtk as we want its children to
-    // have access to it when destroyed.
-    MessageLoop::current()->DeleteSoon(FROM_HERE, this);
-  }
+}
+
+void NativeWidgetGtk::OnDestroyed(GObject *where_the_object_was) {
+  if (ownership_ == Widget::InitParams::NATIVE_WIDGET_OWNS_WIDGET)
+    delete this;
 }
 
 void NativeWidgetGtk::OnShow(GtkWidget* widget) {
diff --git a/views/widget/native_widget_gtk.h b/views/widget/native_widget_gtk.h
index 5ab27c9..7dca06f 100644
--- a/views/widget/native_widget_gtk.h
+++ b/views/widget/native_widget_gtk.h
@@ -257,6 +257,13 @@ class NativeWidgetGtk : public NativeWidget,
   CHROMEGTK_CALLBACK_1(NativeWidgetGtk, gboolean, OnWindowStateEvent,
                        GdkEventWindowState*);
 
+  // Invoked when the widget is destroyed and right before the object
+  // destruction. Useful for overriding.
+  virtual void OnDestroyed(GObject *where_the_object_was);
+  static void OnDestroyedThunk(gpointer data, GObject *where_the_object_was) {
+    reinterpret_cast<NativeWidgetGtk*>(data)->OnDestroyed(where_the_object_was);
+  }
+
   // Invoked when gtk grab is stolen by other GtkWidget in the same
   // application.
   virtual void HandleGtkGrabBroke();
diff --git a/views/window/native_window_gtk.cc b/views/window/native_window_gtk.cc
index a2bf870..cf09524 100644
--- a/views/window/native_window_gtk.cc
+++ b/views/window/native_window_gtk.cc
@@ -362,7 +362,11 @@ void NativeWindowGtk::SaveWindowPosition() {
 void NativeWindowGtk::OnDestroy(GtkWidget* widget) {
   delegate_->OnNativeWindowDestroying();
   NativeWidgetGtk::OnDestroy(widget);
+}
+
+void NativeWindowGtk::OnDestroyed(GObject *where_the_object_was) {
   delegate_->OnNativeWindowDestroyed();
+  NativeWidgetGtk::OnDestroyed(where_the_object_was);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -375,4 +379,3 @@ NativeWindow* NativeWindow::CreateNativeWindow(
 }
 
 }  // namespace views
-
diff --git a/views/window/native_window_gtk.h b/views/window/native_window_gtk.h
index 1d7a773..6516b33 100644
--- a/views/window/native_window_gtk.h
+++ b/views/window/native_window_gtk.h
@@ -78,6 +78,7 @@ class NativeWindowGtk : public NativeWidgetGtk, public NativeWindow {
   friend class Window;
 
   virtual void OnDestroy(GtkWidget* widget);
+  virtual void OnDestroyed(GObject *where_the_object_was);
 
  private:
   static gboolean CallConfigureEvent(GtkWidget* widget,