Merge branch '52-try-more-handlers' into master

Originally targeted for 5.2, but probably too risky there.

Reviewed-by: Alberto Mardegan <alberto.mardegan@nokia.com>

6 files changed, 336 insertions, 254 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index a031fca1..07d36101 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -11,7 +11,6 @@ mc_headers = \
 	mcd-account-conditions.h \
 	mcd-account-manager.h \
 	mcd-debug.h \
-	mcd-dispatch-operation.h \
 	mcd-mission.h \
 	mcd-operation.h \
 	mcd-master.h \
diff --git a/src/mcd-dispatch-operation-priv.h b/src/mcd-dispatch-operation-priv.h
index c216edc5..a485a8ca 100644
--- a/src/mcd-dispatch-operation-priv.h
+++ b/src/mcd-dispatch-operation-priv.h
@@ -24,10 +24,51 @@
 #ifndef __MCD_DISPATCH_OPERATION_PRIV_H__
 #define __MCD_DISPATCH_OPERATION_PRIV_H__
 
-#include "mcd-dispatch-operation.h"
+#include <telepathy-glib/dbus.h>
+#include <telepathy-glib/enums.h>
 
 G_BEGIN_DECLS
 
+typedef struct _McdDispatchOperation McdDispatchOperation;
+typedef struct _McdDispatchOperationPrivate McdDispatchOperationPrivate;
+typedef struct _McdDispatchOperationClass McdDispatchOperationClass;
+
+#include "mcd-account.h"
+
+struct _McdDispatchOperation
+{
+    GObject parent;
+    McdDispatchOperationPrivate *priv;
+};
+
+struct _McdDispatchOperationClass
+{
+    GObjectClass parent_class;
+};
+
+
+#define MC_DISPATCH_OPERATION_DBUS_OBJECT_BASE "/org/freedesktop/Telepathy/DispatchOperation/"
+
+G_GNUC_INTERNAL GType _mcd_dispatch_operation_get_type (void);
+
+G_GNUC_INTERNAL const gchar *_mcd_dispatch_operation_get_path
+    (McdDispatchOperation *operation);
+G_GNUC_INTERNAL GHashTable *_mcd_dispatch_operation_get_properties
+    (McdDispatchOperation *operation);
+G_GNUC_INTERNAL gboolean _mcd_dispatch_operation_is_claimed
+    (McdDispatchOperation *operation);
+G_GNUC_INTERNAL const gchar *_mcd_dispatch_operation_get_handler
+    (McdDispatchOperation *operation);
+G_GNUC_INTERNAL void _mcd_dispatch_operation_approve
+    (McdDispatchOperation *self);
+
+#define MCD_TYPE_DISPATCH_OPERATION         (_mcd_dispatch_operation_get_type ())
+#define MCD_DISPATCH_OPERATION(o)           (G_TYPE_CHECK_INSTANCE_CAST ((o), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperation))
+#define MCD_DISPATCH_OPERATION_CLASS(k)     (G_TYPE_CHECK_CLASS_CAST((k), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperationClass))
+#define MCD_IS_DISPATCH_OPERATION(o)        (G_TYPE_CHECK_INSTANCE_TYPE ((o), MCD_TYPE_DISPATCH_OPERATION))
+#define MCD_IS_DISPATCH_OPERATION_CLASS(k)  (G_TYPE_CHECK_CLASS_TYPE ((k), MCD_TYPE_DISPATCH_OPERATION))
+#define MCD_DISPATCH_OPERATION_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperationClass))
+
 G_GNUC_INTERNAL McdDispatchOperation *_mcd_dispatch_operation_new (
     TpDBusDaemon *dbus_daemon, GList *channels, GStrv possible_handlers);
 G_GNUC_INTERNAL void _mcd_dispatch_operation_lose_channel (
diff --git a/src/mcd-dispatch-operation.c b/src/mcd-dispatch-operation.c
index c6bfe47b..c14e6493 100644
--- a/src/mcd-dispatch-operation.c
+++ b/src/mcd-dispatch-operation.c
@@ -67,7 +67,7 @@ static const McdInterfaceData dispatch_operation_interfaces[] = {
     { G_TYPE_INVALID, }
 };
 
-G_DEFINE_TYPE_WITH_CODE (McdDispatchOperation, mcd_dispatch_operation,
+G_DEFINE_TYPE_WITH_CODE (McdDispatchOperation, _mcd_dispatch_operation,
                          G_TYPE_OBJECT,
     MCD_DBUS_INIT_INTERFACES (dispatch_operation_interfaces);
     G_IMPLEMENT_INTERFACE (TP_TYPE_SVC_DBUS_PROPERTIES, properties_iface_init);
@@ -255,22 +255,34 @@ _mcd_dispatch_operation_finish (McdDispatchOperation *operation)
     return TRUE;
 }
 
+static gboolean mcd_dispatch_operation_check_handle_with (
+    McdDispatchOperation *self, const gchar *handler_name, GError **error);
+
 static void
-dispatch_operation_handle_with (TpSvcChannelDispatchOperation *self,
+dispatch_operation_handle_with (TpSvcChannelDispatchOperation *cdo,
                                 const gchar *handler_name,
                                 DBusGMethodInvocation *context)
 {
     GError *error = NULL;
+    McdDispatchOperation *self = MCD_DISPATCH_OPERATION (cdo);
+
+    DEBUG ("%s/%p", self->priv->unique_name, self);
 
-    mcd_dispatch_operation_handle_with (MCD_DISPATCH_OPERATION (self),
-                                        handler_name, &error);
-    if (error)
+    if (!mcd_dispatch_operation_check_handle_with (self, handler_name, &error))
     {
         dbus_g_method_return_error (context, error);
         g_error_free (error);
+        return;
     }
-    else
-        tp_svc_channel_dispatch_operation_return_from_handle_with (context);
+
+    if (handler_name != NULL && handler_name[0] != '\0')
+    {
+        self->priv->handler = g_strdup (handler_name +
+                                        MCD_CLIENT_BASE_NAME_LEN);
+    }
+
+    _mcd_dispatch_operation_finish (self);
+    tp_svc_channel_dispatch_operation_return_from_handle_with (context);
 }
 
 static void
@@ -327,7 +339,7 @@ mcd_dispatch_operation_constructor (GType type, guint n_params,
                                     GObjectConstructParam *params)
 {
     GObjectClass *object_class =
-        (GObjectClass *)mcd_dispatch_operation_parent_class;
+        (GObjectClass *)_mcd_dispatch_operation_parent_class;
     GObject *object;
     McdDispatchOperation *operation;
     McdDispatchOperationPrivate *priv;
@@ -448,7 +460,7 @@ mcd_dispatch_operation_finalize (GObject *object)
     g_free (priv->object_path);
     g_free (priv->claimer);
 
-    G_OBJECT_CLASS (mcd_dispatch_operation_parent_class)->finalize (object);
+    G_OBJECT_CLASS (_mcd_dispatch_operation_parent_class)->finalize (object);
 }
 
 static void
@@ -484,11 +496,11 @@ mcd_dispatch_operation_dispose (GObject *object)
         g_object_unref (priv->dbus_daemon);
         priv->dbus_daemon = NULL;
     }
-    G_OBJECT_CLASS (mcd_dispatch_operation_parent_class)->dispose (object);
+    G_OBJECT_CLASS (_mcd_dispatch_operation_parent_class)->dispose (object);
 }
 
 static void
-mcd_dispatch_operation_class_init (McdDispatchOperationClass * klass)
+_mcd_dispatch_operation_class_init (McdDispatchOperationClass * klass)
 {
     GObjectClass *object_class = G_OBJECT_CLASS (klass);
     g_type_class_add_private (object_class,
@@ -518,7 +530,7 @@ mcd_dispatch_operation_class_init (McdDispatchOperationClass * klass)
 }
 
 static void
-mcd_dispatch_operation_init (McdDispatchOperation *operation)
+_mcd_dispatch_operation_init (McdDispatchOperation *operation)
 {
     McdDispatchOperationPrivate *priv;
 
@@ -554,21 +566,21 @@ _mcd_dispatch_operation_new (TpDBusDaemon *dbus_daemon,
     return MCD_DISPATCH_OPERATION (obj);
 }
 
-/**
- * mcd_dispatch_operation_get_path:
+/*
+ * _mcd_dispatch_operation_get_path:
  * @operation: the #McdDispatchOperation.
  *
  * Returns: the D-Bus object path of @operation.
  */
 const gchar *
-mcd_dispatch_operation_get_path (McdDispatchOperation *operation)
+_mcd_dispatch_operation_get_path (McdDispatchOperation *operation)
 {
     g_return_val_if_fail (MCD_IS_DISPATCH_OPERATION (operation), NULL);
     return operation->priv->object_path;
 }
 
-/**
- * mcd_dispatch_operation_get_properties:
+/*
+ * _mcd_dispatch_operation_get_properties:
  * @operation: the #McdDispatchOperation.
  *
  * Gets the immutable properties of @operation.
@@ -577,7 +589,7 @@ mcd_dispatch_operation_get_path (McdDispatchOperation *operation)
  * not incremented.
  */
 GHashTable *
-mcd_dispatch_operation_get_properties (McdDispatchOperation *operation)
+_mcd_dispatch_operation_get_properties (McdDispatchOperation *operation)
 {
     McdDispatchOperationPrivate *priv;
 
@@ -615,14 +627,14 @@ mcd_dispatch_operation_get_properties (McdDispatchOperation *operation)
     return priv->properties;
 }
 
-/**
- * mcd_dispatch_operation_is_claimed:
+/*
+ * _mcd_dispatch_operation_is_claimed:
  * @operation: the #McdDispatchOperation.
  *
  * Returns: %TRUE if the operation was claimed, %FALSE otherwise.
  */
 gboolean
-mcd_dispatch_operation_is_claimed (McdDispatchOperation *operation)
+_mcd_dispatch_operation_is_claimed (McdDispatchOperation *operation)
 {
     g_return_val_if_fail (MCD_IS_DISPATCH_OPERATION (operation), FALSE);
     return (operation->priv->claimer != NULL);
@@ -648,54 +660,66 @@ _mcd_dispatch_operation_is_finished (McdDispatchOperation *self)
     return self->priv->finished;
 }
 
-/**
- * mcd_dispatch_operation_get_handler:
+/*
+ * _mcd_dispatch_operation_get_handler:
  * @operation: the #McdDispatchOperation.
  *
  * Returns: the well-known name of the choosen channel handler.
  */
 const gchar *
-mcd_dispatch_operation_get_handler (McdDispatchOperation *operation)
+_mcd_dispatch_operation_get_handler (McdDispatchOperation *operation)
 {
     g_return_val_if_fail (MCD_IS_DISPATCH_OPERATION (operation), NULL);
     return operation->priv->handler;
 }
 
-void
-mcd_dispatch_operation_handle_with (McdDispatchOperation *operation,
-                                    const gchar *handler_name,
-                                    GError **error)
+static gboolean
+mcd_dispatch_operation_check_handle_with (McdDispatchOperation *self,
+                                          const gchar *handler_name,
+                                          GError **error)
 {
-    McdDispatchOperationPrivate *priv;
-
-    g_return_if_fail (MCD_IS_DISPATCH_OPERATION (operation));
-    priv = operation->priv;
-
-    DEBUG ("%s/%p", priv->unique_name, operation);
+    g_return_val_if_fail (MCD_IS_DISPATCH_OPERATION (self), FALSE);
 
-    if (priv->finished)
+    if (self->priv->finished)
     {
         DEBUG ("NotYours: already finished");
         g_set_error (error, TP_ERRORS, TP_ERROR_NOT_YOURS,
                      "CDO already finished");
-        return;
+        return FALSE;
     }
 
-    if (handler_name != NULL && handler_name[0] != '\0')
+    if (handler_name == NULL || handler_name[0] == '\0')
     {
-        if (!g_str_has_prefix (handler_name, MCD_CLIENT_BASE_NAME) ||
-            !tp_dbus_check_valid_bus_name (handler_name,
-                                           TP_DBUS_NAME_TYPE_WELL_KNOWN, NULL))
-        {
-            DEBUG ("InvalidArgument: handler name %s is bad", handler_name);
-            g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
-                         "Invalid handler name");
-            return;
-        }
-        priv->handler = g_strdup (handler_name + MCD_CLIENT_BASE_NAME_LEN);
+        /* no handler name given */
+        return TRUE;
+    }
+
+    if (!g_str_has_prefix (handler_name, MCD_CLIENT_BASE_NAME) ||
+        !tp_dbus_check_valid_bus_name (handler_name,
+                                       TP_DBUS_NAME_TYPE_WELL_KNOWN, NULL))
+    {
+        DEBUG ("InvalidArgument: handler name %s is bad", handler_name);
+        g_set_error (error, TP_ERRORS, TP_ERROR_INVALID_ARGUMENT,
+                     "Invalid handler name");
+        return FALSE;
+    }
+
+    return TRUE;
+}
+
+void
+_mcd_dispatch_operation_approve (McdDispatchOperation *self)
+{
+    g_return_if_fail (MCD_IS_DISPATCH_OPERATION (self));
+
+    DEBUG ("%s/%p", self->priv->unique_name, self);
+
+    if (!mcd_dispatch_operation_check_handle_with (self, NULL, NULL))
+    {
+        return;
     }
 
-    _mcd_dispatch_operation_finish (operation);
+    _mcd_dispatch_operation_finish (self);
 }
 
 void
diff --git a/src/mcd-dispatch-operation.h b/src/mcd-dispatch-operation.h
index 918afa5e..e69de29b 100644
--- a/src/mcd-dispatch-operation.h
+++ b/src/mcd-dispatch-operation.h
@@ -1,71 +0,0 @@
-/* vi: set et sw=4 ts=4 cino=t0,(0: */
-/* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
-/*
- * mcd-operation.h - the Telepathy DispatchOperation D-Bus interface (service side)
- *
- * Copyright (C) 2008 Collabora Ltd. <http://www.collabora.co.uk/>
- * Copyright (C) 2008 Nokia Corporation
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.1 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
- */
-
-#ifndef __MCD_DISPATCH_OPERATION_H__
-#define __MCD_DISPATCH_OPERATION_H__
-
-#include <telepathy-glib/dbus.h>
-#include <telepathy-glib/enums.h>
-
-G_BEGIN_DECLS
-#define MCD_TYPE_DISPATCH_OPERATION         (mcd_dispatch_operation_get_type ())
-#define MCD_DISPATCH_OPERATION(o)           (G_TYPE_CHECK_INSTANCE_CAST ((o), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperation))
-#define MCD_DISPATCH_OPERATION_CLASS(k)     (G_TYPE_CHECK_CLASS_CAST((k), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperationClass))
-#define MCD_IS_DISPATCH_OPERATION(o)        (G_TYPE_CHECK_INSTANCE_TYPE ((o), MCD_TYPE_DISPATCH_OPERATION))
-#define MCD_IS_DISPATCH_OPERATION_CLASS(k)  (G_TYPE_CHECK_CLASS_TYPE ((k), MCD_TYPE_DISPATCH_OPERATION))
-#define MCD_DISPATCH_OPERATION_GET_CLASS(o) (G_TYPE_INSTANCE_GET_CLASS ((o), MCD_TYPE_DISPATCH_OPERATION, McdDispatchOperationClass))
-
-typedef struct _McdDispatchOperation McdDispatchOperation;
-typedef struct _McdDispatchOperationPrivate McdDispatchOperationPrivate;
-typedef struct _McdDispatchOperationClass McdDispatchOperationClass;
-
-#include "mcd-account.h"
-
-struct _McdDispatchOperation
-{
-    GObject parent;
-    McdDispatchOperationPrivate *priv;
-};
-
-struct _McdDispatchOperationClass
-{
-    GObjectClass parent_class;
-};
-
-
-#define MC_DISPATCH_OPERATION_DBUS_OBJECT_BASE "/org/freedesktop/Telepathy/DispatchOperation/"
-
-GType mcd_dispatch_operation_get_type (void);
-
-const gchar *mcd_dispatch_operation_get_path (McdDispatchOperation *operation);
-GHashTable *mcd_dispatch_operation_get_properties
-    (McdDispatchOperation *operation);
-gboolean mcd_dispatch_operation_is_claimed (McdDispatchOperation *operation);
-const gchar *mcd_dispatch_operation_get_handler
-    (McdDispatchOperation *operation);
-void mcd_dispatch_operation_handle_with (McdDispatchOperation *operation,
-                                         const gchar *handler_path,
-                                         GError **error);
-
-G_END_DECLS
-#endif
diff --git a/src/mcd-dispatcher.c b/src/mcd-dispatcher.c
index 719ab4e5..fbb09d7f 100644
--- a/src/mcd-dispatcher.c
+++ b/src/mcd-dispatcher.c
@@ -53,7 +53,6 @@
 #include "mcd-channel-priv.h"
 #include "mcd-dispatcher-context.h"
 #include "mcd-dispatcher-priv.h"
-#include "mcd-dispatch-operation.h"
 #include "mcd-dispatch-operation-priv.h"
 #include "mcd-handler-map-priv.h"
 #include "mcd-misc.h"
@@ -94,10 +93,25 @@ struct _McdDispatcherContext
     /* If this flag is TRUE, dispatching must be cancelled ASAP */
     guint cancelled : 1;
 
-    /* This is set to TRUE if the incoming channel being dispatched has being
-     * requested before the approvers could be run; in that case, the approval
-     * phase should be skipped */
-    guint skip_approval : 1;
+    /* If TRUE, either the channels being dispatched were requested, or they
+     * were pre-approved by being returned as a response to another request,
+     * or a client approved processing with arbitrary handlers */
+    guint approved : 1;
+
+    /* If TRUE, at least one Approver has accepted the CDO. This is a
+     * client lock.
+     *
+     * CTXREF14 is held while we await approval. */
+    guint awaiting_approval : 1;
+
+    /* If TRUE, we're still working out what Observers and Approvers to
+     * run. This is a temporary client lock. CTXREF07 is held while this
+     * lock is active. */
+    guint invoking_clients : 1;
+
+    /* If TRUE, either we've already arranged for the channels to get a
+     * handler, or there are no channels left. */
+    guint channels_handled : 1;
 
     McdDispatcher *dispatcher;
 
@@ -108,15 +122,23 @@ struct _McdDispatcherContext
     /* bus names (including the common prefix) in preference order */
     GStrv possible_handlers;
 
-    /* This variable is the count of locks that must be removed before handlers
-     * can be invoked. Each call to an observer increments this count (and
-     * decrements it on return), and for unrequested channels we have an
-     * approver lock, too.
-     * When the variable gets back to 0, handlers are run. */
-    gint client_locks;
+    /* set of handlers we already tried
+     * dup'd bus name (string) => arbitrary non-NULL pointer */
+    GHashTable *failed_handlers;
 
-    /* Number of approvers that we invoked */
-    gint approvers_invoked;
+    /* The number of observers that have not yet returned from ObserveChannels.
+     * Until they have done so, we can't allow the dispatch operation to
+     * finish. This is a client lock.
+     *
+     * One instance of CTXREF05 is held for each pending observer. */
+    gsize observers_pending;
+
+    /* The number of approvers that have not yet returned from
+     * AddDispatchOperation. Until they have done so, we can't allow the
+     * dispatch operation to finish. This is a client lock.
+     *
+     * One instance of CTXREF06 is held for each pending approver. */
+    gsize approvers_pending;
 
     gchar *protocol;
 
@@ -620,6 +642,8 @@ mcd_dispatcher_set_channel_handled_by (McdDispatcher *self,
     g_signal_emit_by_name (self, "dispatched", channel);
 }
 
+static void mcd_dispatcher_run_handlers (McdDispatcherContext *context);
+
 static void
 handle_channels_cb (TpClient *proxy, const GError *error, gpointer user_data,
                     GObject *weak_object)
@@ -631,34 +655,20 @@ handle_channels_cb (TpClient *proxy, const GError *error, gpointer user_data,
     mcd_dispatcher_context_ref (context, "CTXREF02");
     if (error)
     {
-        GError *mc_error = NULL;
-
-        g_warning ("%s got error: %s", G_STRFUNC, error->message);
+        DEBUG ("error: %s", error->message);
 
-        /* We can't reliably map channel handler error codes to MC error
-         * codes. So just using generic error message.
-         */
-        mc_error = g_error_new (MC_ERROR, MC_CHANNEL_REQUEST_GENERIC_ERROR,
-                                "Handle channel failed: %s", error->message);
-
-        for (list = call_data->channels; list != NULL; list = list->next)
-        {
-            McdChannel *channel = list->data;
+        /* we created this in mcd_dispatcher_run_handlers, so it had better
+         * exist */
+        g_assert (context->failed_handlers != NULL);
 
-            /* if the channel is no longer in the context, don't even try to
-             * access it */
-            if (!g_list_find (context->channels, channel))
-                continue;
+         /* the value is an arbitrary non-NULL pointer - the hash table itself
+          * will do nicely */
+        g_hash_table_insert (context->failed_handlers,
+                             g_strdup (tp_proxy_get_bus_name (proxy)),
+                             context->failed_handlers);
 
-            mcd_channel_take_error (channel, g_error_copy (mc_error));
-            g_signal_emit_by_name (context->dispatcher, "dispatch-failed",
-                                   channel, mc_error);
-
-            /* FIXME: try to dispatch the channels to another handler, instead
-             * of just destroying them? */
-            _mcd_channel_undispatchable (channel);
-        }
-        g_error_free (mc_error);
+        /* try again */
+        mcd_dispatcher_run_handlers (context);
     }
     else
     {
@@ -928,6 +938,13 @@ mcd_dispatcher_run_handlers (McdDispatcherContext *context)
     sp_timestamp ("run handlers");
     mcd_dispatcher_context_ref (context, "CTXREF04");
 
+    if (context->failed_handlers == NULL)
+    {
+        context->failed_handlers = g_hash_table_new_full (g_str_hash,
+                                                          g_str_equal,
+                                                          g_free, NULL);
+    }
+
     /* mcd_dispatcher_handle_channels steals this list */
     channels = g_list_copy (context->channels);
 
@@ -935,7 +952,7 @@ mcd_dispatcher_run_handlers (McdDispatcherContext *context)
      * one we'll consider. */
     if (context->operation)
     {
-        const gchar *approved_handler = mcd_dispatch_operation_get_handler (
+        const gchar *approved_handler = _mcd_dispatch_operation_get_handler (
             context->operation);
 
         if (approved_handler != NULL && approved_handler[0] != '\0')
@@ -944,15 +961,19 @@ mcd_dispatcher_run_handlers (McdDispatcherContext *context)
                                            approved_handler, NULL);
             McdClient *handler = g_hash_table_lookup (self->priv->clients,
                                                       bus_name);
+            gboolean failed = (g_hash_table_lookup (context->failed_handlers,
+                                                    bus_name) != NULL);
 
-            DEBUG ("Approved handler is %s (still exists: %c)",
-                   bus_name, handler != NULL ? 'Y' : 'N');
+            DEBUG ("Approved handler is %s (still exists: %c, "
+                   "already failed: %c)", bus_name,
+                   handler != NULL ? 'Y' : 'N',
+                   failed ? 'Y' : 'N');
 
             g_free (bus_name);
 
-            /* Maybe the handler has exited since we chose it? Otherwise, it's
-             * the right choice. */
-            if (handler != NULL)
+            /* Maybe the handler has exited since we chose it, or maybe we
+             * already tried it? Otherwise, it's the right choice. */
+            if (handler != NULL && !failed)
             {
                 mcd_dispatcher_handle_channels (context, channels, handler);
                 goto finally;
@@ -971,11 +992,13 @@ mcd_dispatcher_run_handlers (McdDispatcherContext *context)
     for (iter = context->possible_handlers; *iter != NULL; iter++)
     {
         McdClient *handler = g_hash_table_lookup (self->priv->clients, *iter);
+        gboolean failed = (g_hash_table_lookup (context->failed_handlers,
+                                                *iter) != NULL);
 
-        DEBUG ("Possible handler: %s (still exists: %c)", *iter,
-               handler != NULL ? 'Y' : 'N');
+        DEBUG ("Possible handler: %s (still exists: %c, already failed: %c)",
+               *iter, handler != NULL ? 'Y' : 'N', failed ? 'Y' : 'N');
 
-        if (handler != NULL)
+        if (handler != NULL && !failed)
         {
             mcd_dispatcher_handle_channels (context, channels, handler);
             goto finally;
@@ -1007,20 +1030,32 @@ finally:
 }
 
 static void
-mcd_dispatcher_context_release_client_lock (McdDispatcherContext *context)
+mcd_dispatcher_context_check_client_locks (McdDispatcherContext *context)
 {
-    g_return_if_fail (context->client_locks > 0);
-    DEBUG ("called on %p, locks = %d", context, context->client_locks);
-    context->client_locks--;
-    if (context->client_locks == 0)
+    if (!context->invoking_clients &&
+        context->observers_pending == 0 &&
+        context->approved)
     {
-        /* no observers left, let's go on with the dispatching */
-        mcd_dispatcher_run_handlers (context);
-        mcd_dispatcher_context_unref (context, "CTXREF13");
+        /* no observers etc. left */
+        if (!context->channels_handled)
+        {
+            context->channels_handled = TRUE;
+            mcd_dispatcher_run_handlers (context);
+        }
     }
 }
 
 static void
+mcd_dispatcher_context_release_pending_observer (McdDispatcherContext *context)
+{
+    DEBUG ("called on %p, %" G_GSIZE_FORMAT " pending",
+           context, context->observers_pending);
+    g_return_if_fail (context->observers_pending > 0);
+    context->observers_pending--;
+    mcd_dispatcher_context_check_client_locks (context);
+}
+
+static void
 observe_channels_cb (TpClient *proxy, const GError *error,
                      gpointer user_data, GObject *weak_object)
 {
@@ -1038,7 +1073,7 @@ observe_channels_cb (TpClient *proxy, const GError *error,
         _mcd_dispatch_operation_unblock_finished (context->operation);
     }
 
-    mcd_dispatcher_context_release_client_lock (context);
+    mcd_dispatcher_context_release_pending_observer (context);
 }
 
 /* The returned GPtrArray is allocated, but the contents are borrowed. */
@@ -1137,11 +1172,11 @@ mcd_dispatcher_run_observers (McdDispatcherContext *context)
         if (context->operation)
         {
             dispatch_operation_path =
-                mcd_dispatch_operation_get_path (context->operation);
+                _mcd_dispatch_operation_get_path (context->operation);
             _mcd_dispatch_operation_block_finished (context->operation);
         }
 
-        context->client_locks++;
+        context->observers_pending++;
         mcd_dispatcher_context_ref (context, "CTXREF05");
         DEBUG ("calling ObserveChannels on %s for context %p",
                client->name, context);
@@ -1166,7 +1201,7 @@ mcd_dispatcher_run_observers (McdDispatcherContext *context)
 }
 
 /*
- * mcd_dispatcher_context_approver_not_invoked:
+ * mcd_dispatcher_context_release_pending_approver:
  * @context: the #McdDispatcherContext.
  *
  * This function is called when an approver returned error on
@@ -1174,13 +1209,20 @@ mcd_dispatcher_run_observers (McdDispatcherContext *context)
  * have contacted. If all of them fail, then we continue the dispatching.
  */
 static void
-mcd_dispatcher_context_approver_not_invoked (McdDispatcherContext *context)
+mcd_dispatcher_context_release_pending_approver (McdDispatcherContext *context)
 {
-    g_return_if_fail (context->approvers_invoked > 0);
-    context->approvers_invoked--;
+    g_return_if_fail (context->approvers_pending > 0);
+    context->approvers_pending--;
+
+    if (context->approvers_pending == 0 &&
+        !context->awaiting_approval)
+    {
+        DEBUG ("No approver accepted the channels; considering them to be "
+               "approved");
+        context->approved = TRUE;
+    }
 
-    if (context->approvers_invoked == 0)
-        mcd_dispatcher_context_release_client_lock (context);
+    mcd_dispatcher_context_check_client_locks (context);
 }
 
 static void
@@ -1193,23 +1235,29 @@ add_dispatch_operation_cb (TpClient *proxy, const GError *error,
     {
         DEBUG ("AddDispatchOperation %s (context %p) on approver %s failed: "
                "%s",
-               mcd_dispatch_operation_get_path (context->operation),
+               _mcd_dispatch_operation_get_path (context->operation),
                context, tp_proxy_get_object_path (proxy), error->message);
-
-        /* if all approvers fail to add the DO, then we behave as if no
-         * approver was registered: i.e., we continue dispatching */
-        context->approvers_invoked--;
-        if (context->approvers_invoked == 0)
-            mcd_dispatcher_context_release_client_lock (context);
     }
     else
     {
         DEBUG ("Approver %s accepted AddDispatchOperation %s (context %p)",
                tp_proxy_get_object_path (proxy),
-               mcd_dispatch_operation_get_path (context->operation),
+               _mcd_dispatch_operation_get_path (context->operation),
                context);
+
+        if (!context->awaiting_approval)
+        {
+            context->awaiting_approval = TRUE;
+            mcd_dispatcher_context_ref (context, "CTXREF14");
+        }
     }
 
+    /* If all approvers fail to add the DO, then we behave as if no
+     * approver was registered: i.e., we continue dispatching. If at least
+     * one approver accepted it, then we can still continue dispatching,
+     * since it will be stalled until awaiting_approval becomes FALSE. */
+    mcd_dispatcher_context_release_pending_approver (context);
+
     if (context->operation)
     {
         _mcd_dispatch_operation_unblock_finished (context->operation);
@@ -1236,9 +1284,8 @@ mcd_dispatcher_run_approvers (McdDispatcherContext *context)
     /* we temporarily increment this count and decrement it at the end of the
      * function, to make sure it won't become 0 while we are still invoking
      * approvers */
-    context->approvers_invoked = 1;
+    context->approvers_pending = 1;
 
-    context->client_locks++;
     channels = context->channels;
     g_hash_table_iter_init (&iter, priv->clients);
     while (g_hash_table_iter_next (&iter, NULL, (gpointer *) &client))
@@ -1265,9 +1312,9 @@ mcd_dispatcher_run_approvers (McdDispatcherContext *context)
         if (!matched) continue;
 
         dispatch_operation =
-            mcd_dispatch_operation_get_path (context->operation);
+            _mcd_dispatch_operation_get_path (context->operation);
         properties =
-            mcd_dispatch_operation_get_properties (context->operation);
+            _mcd_dispatch_operation_get_properties (context->operation);
         channel_details =
             _mcd_dispatch_operation_dup_channel_details (context->operation);
 
@@ -1275,7 +1322,7 @@ mcd_dispatcher_run_approvers (McdDispatcherContext *context)
                "of context %p", client->name, dispatch_operation,
                context->operation, context);
 
-        context->approvers_invoked++;
+        context->approvers_pending++;
         _mcd_dispatch_operation_block_finished (context->operation);
 
         mcd_dispatcher_context_ref (context, "CTXREF06");
@@ -1291,7 +1338,7 @@ mcd_dispatcher_run_approvers (McdDispatcherContext *context)
 
     /* This matches the approvers count set to 1 at the beginning of the
      * function */
-    mcd_dispatcher_context_approver_not_invoked (context);
+    mcd_dispatcher_context_release_pending_approver (context);
 }
 
 static gboolean
@@ -1334,26 +1381,28 @@ static void
 mcd_dispatcher_run_clients (McdDispatcherContext *context)
 {
     mcd_dispatcher_context_ref (context, "CTXREF07");
-    context->client_locks = 1; /* we release this lock at the end of the
-                                    function */
-
-    /* CTXREF13 is released after all client locks are released */
-    mcd_dispatcher_context_ref (context, "CTXREF13");
+    context->invoking_clients = TRUE;
 
     mcd_dispatcher_run_observers (context);
 
+    /* if we have a dispatch operation, it means that the channels were not
+     * requested: start the Approvers */
     if (context->operation)
     {
-        /* if we have a dispatch operation, it means that the channels were not
-         * requested: start the Approvers */
+        /* but if the handlers have the BypassApproval flag set, then don't
+         *
+         * FIXME: we should really run BypassApproval handlers as a separate
+         * stage, rather than considering the existence of a BypassApproval
+         * handler to constitute approval - this is fd.o #23687 */
+        if (handlers_can_bypass_approval (context))
+            context->approved = TRUE;
 
-        /* but if the handlers have the BypassApproval flag set, then don't */
-        if (!context->skip_approval &&
-            !handlers_can_bypass_approval (context))
+        if (!context->approved)
             mcd_dispatcher_run_approvers (context);
     }
 
-    mcd_dispatcher_context_release_client_lock (context);
+    context->invoking_clients = FALSE;
+    mcd_dispatcher_context_check_client_locks (context);
     mcd_dispatcher_context_unref (context, "CTXREF07");
 }
 
@@ -1452,24 +1501,31 @@ on_operation_finished (McdDispatchOperation *operation,
      * CDO: according to which of these have happened, we run the choosen
      * handler or we don't. */
 
+    mcd_dispatcher_context_ref (context, "CTXREF15");
+
     if (context->dispatcher->priv->operation_list_active)
     {
         tp_svc_channel_dispatcher_interface_operation_list_emit_dispatch_operation_finished (
-            context->dispatcher, mcd_dispatch_operation_get_path (operation));
+            context->dispatcher, _mcd_dispatch_operation_get_path (operation));
     }
 
+    /* Because of our calls to _mcd_dispatch_operation_block_finished,
+     * this cannot happen until all observers and all approvers have
+     * returned from ObserveChannels or AddDispatchOperation, respectively.
+     *
+     * (However, in observe_channels_cb we unblock finished a moment
+     * before we decrement observers_pending, so we can't actually assert
+     * that here.) */
+
+    g_assert (context->approvers_pending == 0);
+
     if (context->channels == NULL)
     {
         DEBUG ("Nothing left to dispatch");
 
-        if (context->client_locks > 0)
-        {
-            /* this would have been released when all the locks were released,
-             * but now we're never going to do that */
-            mcd_dispatcher_context_unref (context, "CTXREF13");
-        }
+        context->channels_handled = TRUE;
     }
-    else if (mcd_dispatch_operation_is_claimed (operation))
+    else if (_mcd_dispatch_operation_is_claimed (operation))
     {
         GList *list;
 
@@ -1485,18 +1541,19 @@ on_operation_finished (McdDispatchOperation *operation,
         }
 
         mcd_dispatcher_context_handler_done (context);
-
-        /* this would have been released when all the locks were released, but
-         * we're never going to do that */
-        g_assert (context->client_locks > 0);
-        mcd_dispatcher_context_unref (context, "CTXREF13");
+        g_assert (!context->channels_handled);
+        context->channels_handled = TRUE;
     }
-    else
+
+    if (context->awaiting_approval)
     {
-        /* this is the lock set in mcd_dispatcher_run_approvers(): releasing
-         * this will make the handlers run */
-        mcd_dispatcher_context_release_client_lock (context);
+        context->awaiting_approval = FALSE;
+        context->approved = TRUE;
+        mcd_dispatcher_context_unref (context, "CTXREF14");
     }
+
+    mcd_dispatcher_context_check_client_locks (context);
+    mcd_dispatcher_context_unref (context, "CTXREF15");
 }
 
 /* ownership of channels, possible_handlers is stolen */
@@ -1542,8 +1599,14 @@ _mcd_dispatcher_enter_state_machine (McdDispatcher *dispatcher,
            mcd_channel_get_object_path (context->channels->data));
 
     priv->contexts = g_list_prepend (priv->contexts, context);
-    if (!requested)
+
+    if (requested)
     {
+        context->approved = TRUE;
+    }
+    else
+    {
+        context->approved = FALSE;
         context->operation =
             _mcd_dispatch_operation_new (priv->dbus_daemon, channels,
                                          possible_handlers);
@@ -1552,8 +1615,8 @@ _mcd_dispatcher_enter_state_machine (McdDispatcher *dispatcher,
         {
             tp_svc_channel_dispatcher_interface_operation_list_emit_new_dispatch_operation (
                 dispatcher,
-                mcd_dispatch_operation_get_path (context->operation),
-                mcd_dispatch_operation_get_properties (context->operation));
+                _mcd_dispatch_operation_get_path (context->operation),
+                _mcd_dispatch_operation_get_properties (context->operation));
         }
 
         g_signal_connect (context->operation, "finished",
@@ -1570,22 +1633,12 @@ _mcd_dispatcher_enter_state_machine (McdDispatcher *dispatcher,
                                 context);
     }
 
-    if (priv->filters != NULL)
-    {
-        DEBUG ("entering state machine for context %p", context);
+    DEBUG ("entering state machine for context %p", context);
 
-        sp_timestamp ("invoke internal filters");
-
-        mcd_dispatcher_context_ref (context, "CTXREF01");
-	mcd_dispatcher_context_process (context, TRUE);
-    }
-    else
-    {
-        DEBUG ("No filters found for context %p, "
-               "starting the channel handler", context);
+    sp_timestamp ("invoke internal filters");
 
-	mcd_dispatcher_run_clients (context);
-    }
+    mcd_dispatcher_context_ref (context, "CTXREF01");
+    mcd_dispatcher_context_proceed (context);
 
     mcd_dispatcher_context_unref (context, "CTXREF11");
 }
@@ -1672,9 +1725,9 @@ _mcd_dispatcher_get_property (GObject * obj, guint prop_id,
                                   TP_HASH_TYPE_STRING_VARIANT_MAP);
 
                     g_value_set_boxed (va->values + 0,
-                        mcd_dispatch_operation_get_path (context->operation));
+                        _mcd_dispatch_operation_get_path (context->operation));
                     g_value_set_boxed (va->values + 1,
-                        mcd_dispatch_operation_get_properties (
+                        _mcd_dispatch_operation_get_properties (
                             context->operation));
 
                     g_ptr_array_add (operations, va);
@@ -3013,7 +3066,7 @@ mcd_dispatcher_context_close_all (McdDispatcherContext *context,
  * mcd_dispatcher_context_proceed (c), which should be used instead in new
  * code.
  *
- * mcd_dispatcher_context_process (c, TRUE) is exactly equivalent to
+ * mcd_dispatcher_context_process (c, FALSE) is exactly equivalent to
  * mcd_dispatcher_context_destroy_all (c) followed by
  * mcd_dispatcher_context_proceed (c), which should be used instead in new
  * code.
@@ -3064,7 +3117,7 @@ mcd_dispatcher_context_unref (McdDispatcherContext * context,
             {
                 tp_svc_channel_dispatcher_interface_operation_list_emit_dispatch_operation_finished (
                     context->dispatcher,
-                    mcd_dispatch_operation_get_path (context->operation));
+                    _mcd_dispatch_operation_get_path (context->operation));
             }
 
             g_object_unref (context->operation);
@@ -3076,6 +3129,11 @@ mcd_dispatcher_context_unref (McdDispatcherContext * context,
         priv = MCD_DISPATCHER_PRIV (context->dispatcher);
         priv->contexts = g_list_remove (priv->contexts, context);
 
+        if (context->failed_handlers != NULL)
+        {
+            g_hash_table_destroy (context->failed_handlers);
+        }
+
         g_strfreev (context->possible_handlers);
         g_free (context->protocol);
         g_free (context);
@@ -3614,17 +3672,18 @@ _mcd_dispatcher_add_channel_request (McdDispatcher *dispatcher,
 
             context = find_context_from_channel (dispatcher, channel);
             DEBUG ("channel %p is in context %p", channel, context);
-            if (context->approvers_invoked > 0)
+            if (context->approvers_pending > 0 || context->awaiting_approval)
             {
                 /* the existing channel is waiting for approval; but since the
                  * same channel has been requested, the approval operation must
                  * terminate */
                 if (G_LIKELY (context->operation))
-                    mcd_dispatch_operation_handle_with (context->operation,
-                                                        NULL, NULL);
+                    _mcd_dispatch_operation_approve (context->operation);
             }
             else
-                context->skip_approval = TRUE;
+            {
+                context->approved = TRUE;
+            }
         }
         DEBUG ("channel %p is proxying %p", request, channel);
     }
diff --git a/test/twisted/dispatcher/handle-channels-fails.py b/test/twisted/dispatcher/handle-channels-fails.py
index 43e3d236..84e6f8c4 100644
--- a/test/twisted/dispatcher/handle-channels-fails.py
+++ b/test/twisted/dispatcher/handle-channels-fails.py
@@ -41,6 +41,9 @@ def test(q, bus, mc):
         cs.CHANNEL + '.TargetHandleType': cs.HT_CONTACT,
         cs.CHANNEL + '.ChannelType': cs.CHANNEL_TYPE_TEXT,
         }, signature='sv')
+    vague_fixed_properties = dbus.Dictionary({
+        cs.CHANNEL + '.ChannelType': cs.CHANNEL_TYPE_TEXT,
+        }, signature='sv')
 
     empathy_bus = dbus.bus.BusConnection()
     q.attach_to_bus(empathy_bus)
@@ -48,8 +51,15 @@ def test(q, bus, mc):
             observe=[text_fixed_properties], approve=[text_fixed_properties],
             handle=[text_fixed_properties], bypass_approval=False)
 
+    # Kopete's filter is less specific than Empathy's, so we'll prefer Empathy
+    kopete_bus = dbus.bus.BusConnection()
+    q.attach_to_bus(kopete_bus)
+    kopete = SimulatedClient(q, kopete_bus, 'Kopete',
+            observe=[], approve=[],
+            handle=[vague_fixed_properties], bypass_approval=False)
+
     # wait for MC to download the properties
-    expect_client_setup(q, [empathy])
+    expect_client_setup(q, [empathy, kopete])
 
     # subscribe to the OperationList interface (MC assumes that until this
     # property has been retrieved once, nobody cares)
@@ -86,8 +96,11 @@ def test(q, bus, mc):
     assert cdo_properties[cs.CDO + '.Connection'] == conn.object_path
     assert cs.CDO + '.Interfaces' in cdo_properties
 
+    # In this test Empathy's filter has more things in it than Kopete's, so
+    # MC will prefer Empathy
     handlers = cdo_properties[cs.CDO + '.PossibleHandlers'][:]
-    assert handlers == [cs.tp_name_prefix + '.Client.Empathy'], handlers
+    assert handlers == [cs.tp_name_prefix + '.Client.Empathy',
+            cs.tp_name_prefix + '.Client.Kopete'], handlers
 
     assert cs.CD_IFACE_OP_LIST in cd_props.Get(cs.CD, 'Interfaces')
     assert cd_props.Get(cs.CD_IFACE_OP_LIST, 'DispatchOperations') ==\
@@ -151,6 +164,23 @@ def test(q, bus, mc):
     # Empathy rejects the channels
     q.dbus_raise(e.message, cs.NOT_AVAILABLE, 'Blind drunk', bus=empathy_bus)
 
+    # Kopete is asked to handle the channels
+    (k,) = q.expect_many(
+            EventPattern(
+                'dbus-method-call',
+                path=kopete.object_path,
+                interface=cs.HANDLER, method='HandleChannels',
+                handled=False),
+            )
+
+    # Kopete rejects the channels too
+    q.dbus_raise(k.message, cs.NOT_AVAILABLE, 'Also blind drunk',
+            bus=kopete_bus)
+
+    # MC gives up and closes the channel
+    q.expect('dbus-method-call', path=chan.object_path,
+            interface=cs.CHANNEL, method='Close', args=[])
+
     # Now there are no more active channel dispatch operations
     assert cd_props.Get(cs.CD_IFACE_OP_LIST, 'DispatchOperations') == []