[glibmm] Glib::Dispatcher: Don't send messages to a deleted Dispatcher.

[Kjell Ahlstedt <kjellahl src gnome org>]

commit 0f64aef58ac9d8ad1604800d3ba74bb998f63987
Author: Kjell Ahlstedt <kjell ahlstedt bredband net>
Date:   Wed Apr 4 14:18:29 2012 +0200

    Glib::Dispatcher: Don't send messages to a deleted Dispatcher.
    
    * glib/glibmm/dispatcher.h: Add missing usage rules.
    * glib/glibmm/dispatcher.cc: Avoid delivering messages to deleted Dispatchers.
    Don't block message delivery while a second main loop is running.
    Bug #651942.

 ChangeLog                 |    9 +++++
 glib/glibmm/dispatcher.cc |   87 +++++++++++++++++++++++++++++++++++++++-----
 glib/glibmm/dispatcher.h  |    7 +++-
 3 files changed, 91 insertions(+), 12 deletions(-)
---
diff --git a/ChangeLog b/ChangeLog
index fb37558..bb519c8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,4 +1,13 @@
 2012-04-04  Kjell Ahlstedt  <kjell ahlstedt bredband net>
+ 
+	Glib::Dispatcher: Don't send messages to a deleted Dispatcher.
+
+	* glib/glibmm/dispatcher.h: Add missing usage rules.
+	* glib/glibmm/dispatcher.cc: Avoid delivering messages to deleted Dispatchers.
+	Don't block message delivery while a second main loop is running.
+	Bug #651942.
+
+2012-04-04  Kjell Ahlstedt  <kjell ahlstedt bredband net>
 
 	Make SignalTimeout,SignalIdle::connect_once() more thread safe.
 
diff --git a/glib/glibmm/dispatcher.cc b/glib/glibmm/dispatcher.cc
index 6bf8243..f6257f3 100644
--- a/glib/glibmm/dispatcher.cc
+++ b/glib/glibmm/dispatcher.cc
@@ -27,6 +27,7 @@
 #include <cerrno>
 #include <fcntl.h>
 #include <glib.h>
+#include <set>
 
 #ifdef G_OS_WIN32
 # include <windows.h>
@@ -126,8 +127,10 @@ class DispatchNotifier : public sigc::trackable
 public:
   ~DispatchNotifier();
 
-  static DispatchNotifier* reference_instance(const Glib::RefPtr<MainContext>& context);
-  static void unreference_instance(DispatchNotifier* notifier);
+  static DispatchNotifier* reference_instance(
+    const Glib::RefPtr<MainContext>& context, const Dispatcher* dispatcher);
+  static void unreference_instance(
+    DispatchNotifier* notifier, const Dispatcher* dispatcher);
 
   void send_notification(Dispatcher* dispatcher);
 
@@ -139,6 +142,8 @@ protected:
 private:
   static Glib::Threads::Private<DispatchNotifier> thread_specific_instance_;
 
+  std::set<const Dispatcher*>   deleted_dispatchers_;
+
   long                          ref_count_;
   Glib::RefPtr<MainContext>     context_;
 #ifdef G_OS_WIN32
@@ -152,6 +157,7 @@ private:
 
   void create_pipe();
   bool pipe_io_handler(Glib::IOCondition condition);
+  bool pipe_is_empty();
 
   // noncopyable
   DispatchNotifier(const DispatchNotifier&);
@@ -165,6 +171,7 @@ Glib::Threads::Private<DispatchNotifier> DispatchNotifier::thread_specific_insta
 
 DispatchNotifier::DispatchNotifier(const Glib::RefPtr<MainContext>& context)
 :
+  deleted_dispatchers_(),
   ref_count_    (0),
   context_      (context),
 #ifdef G_OS_WIN32
@@ -185,8 +192,20 @@ DispatchNotifier::DispatchNotifier(const Glib::RefPtr<MainContext>& context)
 #else
     const int fd = fd_receiver_;
 #endif
-    context_->signal_io().connect(sigc::mem_fun(*this, &DispatchNotifier::pipe_io_handler),
-                                  fd, Glib::IO_IN);
+    // The following code is equivalent to
+    // context_->signal_io().connect(
+    //   sigc::mem_fun(*this, &DispatchNotifier::pipe_io_handler), fd, Glib::IO_IN);
+    // except for source->set_can_recurse(true).
+
+    const Glib::RefPtr<IOSource> source = IOSource::create(fd, Glib::IO_IN);
+
+    // If the signal emission in pipe_io_handler() starts a new main loop,
+    // the event source shall not be blocked while that loop runs. (E.g. while
+    // a connected slot function shows a modal dialog box.)
+    source->set_can_recurse(true);
+
+    source->connect(sigc::mem_fun(*this, &DispatchNotifier::pipe_io_handler));
+    g_source_attach(source->gobj(), context_->gobj());
   }
   catch(...)
   {
@@ -248,7 +267,8 @@ void DispatchNotifier::create_pipe()
 }
 
 // static
-DispatchNotifier* DispatchNotifier::reference_instance(const Glib::RefPtr<MainContext>& context)
+DispatchNotifier* DispatchNotifier::reference_instance
+  (const Glib::RefPtr<MainContext>& context, const Dispatcher* dispatcher)
 {
   DispatchNotifier* instance = thread_specific_instance_.get();
 
@@ -261,6 +281,17 @@ DispatchNotifier* DispatchNotifier::reference_instance(const Glib::RefPtr<MainCo
   {
     // Prevent massive mess-up.
     g_return_val_if_fail(instance->context_ == context, 0);
+
+    // In the possible but unlikely case that a new dispatcher gets the same
+    // address as a newly deleted one, if the pipe still contains messages to
+    // the deleted dispatcher, those messages will be delivered to the new one.
+    // Not ideal, but perhaps the best that can be done without breaking ABI.
+    // The alternative would be to remove the following erase(), and risk not
+    // delivering messages sent to the new dispatcher.
+    //TODO: When we can break ABI, a better solution without this drawback can
+    // be implemented. See https://bugzilla.gnome.org/show_bug.cgi?id=651942
+    // especially comment 16.
+    instance->deleted_dispatchers_.erase(dispatcher);
   }
 
   ++instance->ref_count_; // initially 0
@@ -269,13 +300,22 @@ DispatchNotifier* DispatchNotifier::reference_instance(const Glib::RefPtr<MainCo
 }
 
 // static
-void DispatchNotifier::unreference_instance(DispatchNotifier* notifier)
+void DispatchNotifier::unreference_instance(
+  DispatchNotifier* notifier, const Dispatcher* dispatcher)
 {
-  DispatchNotifier *const instance = thread_specific_instance_.get();
+  DispatchNotifier* const instance = thread_specific_instance_.get();
 
   // Yes, the notifier argument is only used to check for sanity.
   g_return_if_fail(instance == notifier);
 
+  if (instance->pipe_is_empty())
+    // No messages in the pipe. No need to keep track of deleted dispatchers.
+    instance->deleted_dispatchers_.clear();
+  else
+    // There are messages in the pipe, possibly to the deleted dispatcher.
+    // Keep its address, so pipe_io_handler() can avoid delivering messages to it.
+    instance->deleted_dispatchers_.insert(dispatcher);
+
   if(--instance->ref_count_ <= 0)
   {
     g_return_if_fail(instance->ref_count_ == 0); // could be < 0 if messed up
@@ -331,6 +371,18 @@ void DispatchNotifier::send_notification(Dispatcher* dispatcher)
 #endif /* !G_OS_WIN32 */
 }
 
+bool DispatchNotifier::pipe_is_empty()
+{
+#ifdef G_OS_WIN32
+  return notify_queue_.empty();
+#else
+  PollFD poll_fd(fd_receiver_, Glib::IO_IN);
+  // GPollFD*, number of file descriptors to poll, timeout (ms)
+  g_poll(poll_fd.gobj(), 1, 0);
+  return (poll_fd.get_revents() & Glib::IO_IN) == 0;
+#endif
+}
+
 bool DispatchNotifier::pipe_io_handler(Glib::IOCondition)
 {
   DispatchNotifyData data;
@@ -385,6 +437,21 @@ bool DispatchNotifier::pipe_io_handler(Glib::IOCondition)
 
   g_return_val_if_fail(data.notifier == this, true);
 
+  // Drop the received message, if it is addressed to a deleted dispatcher.
+  const bool drop_message =
+    (deleted_dispatchers_.find(data.dispatcher) != deleted_dispatchers_.end());
+
+  // If the pipe is empty, there can be no messages to deleted dispatchers.
+  // No reason to keep track of them any more.
+  if (!deleted_dispatchers_.empty() && pipe_is_empty())
+    deleted_dispatchers_.clear();
+
+  if (drop_message)
+  {
+    g_warning("Dropped dispatcher message as the dispatcher no longer exists");
+    return true;
+  }
+
   // Actually, we wouldn't need the try/catch block because the Glib::Source
   // C callback already does it for us.  However, we do it anyway because the
   // default return value is 'false', which is not what we want.
@@ -405,18 +472,18 @@ bool DispatchNotifier::pipe_io_handler(Glib::IOCondition)
 Dispatcher::Dispatcher()
 :
   signal_   (),
-  notifier_ (DispatchNotifier::reference_instance(MainContext::get_default()))
+  notifier_ (DispatchNotifier::reference_instance(MainContext::get_default(), this))
 {}
 
 Dispatcher::Dispatcher(const Glib::RefPtr<MainContext>& context)
 :
   signal_   (),
-  notifier_ (DispatchNotifier::reference_instance(context))
+  notifier_ (DispatchNotifier::reference_instance(context, this))
 {}
 
 Dispatcher::~Dispatcher()
 {
-  DispatchNotifier::unreference_instance(notifier_);
+  DispatchNotifier::unreference_instance(notifier_, this);
 }
 
 void Dispatcher::emit()
diff --git a/glib/glibmm/dispatcher.h b/glib/glibmm/dispatcher.h
index e92512f..9672165 100644
--- a/glib/glibmm/dispatcher.h
+++ b/glib/glibmm/dispatcher.h
@@ -48,6 +48,9 @@ class DispatchNotifier;
  * @li The Dispatcher object must be instantiated by the receiver thread.
  * @li The Dispatcher object should be instantiated before creating any of the
  * sender threads, if you want to avoid extra locking.
+ * @li The Dispatcher object must be deleted by the receiver thread.
+ * @li All Dispatcher objects instantiated by the same receiver thread must
+ * use the same main context.
  *
  * Notes about performance:
  *
@@ -74,7 +77,7 @@ public:
    * @throw Glib::FileError
    */
   Dispatcher();
-  
+
   /** Create new Dispatcher instance using an arbitrary main context.
    * @throw Glib::FileError
    */
@@ -88,7 +91,7 @@ public:
 
 private:
   sigc::signal<void> signal_;
-  DispatchNotifier*   notifier_;
+  DispatchNotifier*  notifier_;
 
   // noncopyable
   Dispatcher(const Dispatcher&);