[mms-engine] Make sure that context deactivation call completes befor…

…e we exit

Otherwise there is a chance that org.ofono.ConnectionContext.SetProperty call
gets canceled before it reaches ofono and MMS context may remain active after
mms-engine exits.

mms-lib/include/mms_connection.h

@@ -82,6 +82,9 @@ mms_connection_close(
 
 #define mms_connection_is_open(connection) \
     (mms_connection_state(connection) == MMS_CONNECTION_STATE_OPEN)
+#define mms_connection_is_active(connection) ((connection) && \
+    ((connection)->state == MMS_CONNECTION_STATE_OPEN || \
+     (connection)->state == MMS_CONNECTION_STATE_OPENING))
 
 #endif /* JOLLA_MMS_CONNECTION_H */
 

mms-lib/src/mms_dispatcher.c

@@ -43,6 +43,7 @@ struct mms_dispatcher {
     GQueue* tasks;
     guint next_run_id;
     guint network_idle_id;
+    gboolean started;
 };
 
 typedef void (*MMSDispatcherIdleCallbackProc)(MMSDispatcher* disp);
@@ -64,29 +65,63 @@ mms_dispatcher_run(
     MMSDispatcher* disp);
 
 /**
- * Close the network connection
+ * Checks if we are done and notifies the delegate if necessary
  */
 static
 void
-mms_dispatcher_close_connection(
+mms_dispatcher_check_if_done(
+    MMSDispatcher* disp)
+{
+    if (!mms_dispatcher_is_active(disp)) {
+        /* Cancel pending runs */
+        if (disp->next_run_id) {
+            g_source_remove(disp->next_run_id);
+            disp->next_run_id = 0;
+        }
+        /* Notify the delegate that we are done */
+        if (disp->delegate && disp->delegate->fn_done && disp->started) {
+            disp->started = FALSE;
+            disp->delegate->fn_done(disp->delegate, disp);
+        }
+    }
+}
+
+/**
+ * Drops the reference to the network connection
+ */
+static
+void
+mms_dispatcher_drop_connection(
     MMSDispatcher* disp)
 {
     if (disp->connection) {
-        disp->connection->delegate = NULL;
-        mms_connection_close(disp->connection);
+        MMS_ASSERT(!mms_connection_is_active(disp->connection));
         mms_connection_unref(disp->connection);
+        disp->connection->delegate = NULL;
         disp->connection = NULL;
-
-        if (!mms_dispatcher_is_active(disp)) {
-            /* Report to delegate that we are done */
-            if (disp->delegate && disp->delegate->fn_done) {
-                disp->delegate->fn_done(disp->delegate, disp);
-            }
+        if (disp->network_idle_id) {
+            g_source_remove(disp->network_idle_id);
+            disp->network_idle_id = 0;
         }
     }
-    if (disp->network_idle_id) {
-        g_source_remove(disp->network_idle_id);
-        disp->network_idle_id = 0;
+}
+
+/**
+ * Close the network connection
+ */
+static
+void
+mms_dispatcher_close_connection(
+    MMSDispatcher* disp)
+{
+    if (disp->connection) {
+        mms_connection_close(disp->connection);
+        /* Assert that connection state changes are asynchronous */
+        MMS_ASSERT(disp->connection);
+        if (!mms_connection_is_active(disp->connection)) {
+            mms_dispatcher_drop_connection(disp);
+        }
+        mms_dispatcher_check_if_done(disp);
     }
 }
 
@@ -225,8 +260,8 @@ gboolean
 mms_dispatcher_is_active(
     MMSDispatcher* disp)
 {
-    return disp && (disp->connection || disp->active_task ||
-        !g_queue_is_empty(disp->tasks));
+    return disp && (mms_connection_is_active(disp->connection) ||
+        disp->active_task || !g_queue_is_empty(disp->tasks));
 }
 
 /**
@@ -373,17 +408,7 @@ mms_dispatcher_run(
         }
     }
 
-    if (!mms_dispatcher_is_active(disp)) {
-        /* Cancel pending runs */
-        if (disp->next_run_id) {
-            g_source_remove(disp->next_run_id);
-            disp->next_run_id = 0;
-        }
-        /* Report to delegate that we are done */
-        if (disp->delegate && disp->delegate->fn_done) {
-            disp->delegate->fn_done(disp->delegate, disp);
-        }
-    }
+    mms_dispatcher_check_if_done(disp);
 }
 
 /**
@@ -397,6 +422,7 @@ mms_dispatcher_start(
     int err = g_mkdir_with_parents(root_dir, MMS_DIR_PERM);
     if (!err || errno == EEXIST) {
         if (!g_queue_is_empty(disp->tasks)) {
+            disp->started = TRUE;
             mms_dispatcher_next_run_schedule(disp);
             return TRUE;
         }
@@ -577,6 +603,8 @@ mms_dispatcher_delegate_connection_state_changed(
                 break;
             }
         }
+        mms_dispatcher_drop_connection(disp);
+        mms_dispatcher_check_if_done(disp);
     }
     if (!disp->active_task) {
         mms_dispatcher_next_run_schedule(disp);
@@ -648,7 +676,7 @@ mms_dispatcher_finalize(
     const char* root_dir = disp->settings->config->root_dir;
     char* msg_dir = g_strconcat(root_dir, "/" MMS_MESSAGE_DIR "/", NULL);
     MMS_VERBOSE_("");
-    mms_dispatcher_close_connection(disp);
+    mms_dispatcher_drop_connection(disp);
     while ((task = g_queue_pop_head(disp->tasks)) != NULL) {
         task->delegate = NULL;
         mms_task_cancel(task);

mms-lib/test/common/test_connection.c

@@ -27,32 +27,45 @@ G_DEFINE_TYPE(MMSConnectionTest, mms_connection_test, MMS_TYPE_CONNECTION);
 #define MMS_CONNECTION_TEST(obj) (G_TYPE_CHECK_INSTANCE_CAST((obj),\
    MMS_TYPE_CONNECTION_TEST, MMSConnectionTest))
 
+typedef struct test_connection_state_change {
+    MMSConnectionTest* test;
+    MMS_CONNECTION_STATE state;
+} MMSConnectionStateChange;
+
 static
 gboolean
-mms_connection_test_set_state(
-    MMSConnection* test,
-    MMS_CONNECTION_STATE state)
+test_connection_test_state_change_cb(
+    void* param)
 {
-    if (test->state != state && test->state != MMS_CONNECTION_STATE_CLOSED) {
-        test->state = state;
+    MMSConnectionStateChange* change = param;
+    MMSConnectionTest* test = change->test;
+    if (test->state != MMS_CONNECTION_STATE_CLOSED &&
+        test->state != MMS_CONNECTION_STATE_FAILED &&
+        test->state != change->state) {
+        test->state = change->state;
         if (test->delegate &&
             test->delegate->fn_connection_state_changed) {
             test->delegate->fn_connection_state_changed(test->delegate, test);
         }
     }
-    return TRUE;
+    mms_connection_unref(change->test);
+    g_free(change);
+    return FALSE;
 }
 
 static
 gboolean
-test_connection_test_open(
-    void* param)
+mms_connection_test_set_state(
+    MMSConnection* test,
+    MMS_CONNECTION_STATE state)
 {
-    MMSConnectionTest* test = param;
-    mms_connection_test_set_state(test, test->netif ?
-        MMS_CONNECTION_STATE_OPEN : MMS_CONNECTION_STATE_FAILED);
-    mms_connection_unref(test);
-    return FALSE;
+    if (test->state != MMS_CONNECTION_STATE_CLOSED) {
+        MMSConnectionStateChange* change = g_new0(MMSConnectionStateChange,1);
+        change->state = state;
+        change->test = mms_connection_ref(test);
+        g_idle_add(test_connection_test_state_change_cb, change);
+    }
+    return TRUE;
 }
 
 MMSConnection*
@@ -73,7 +86,8 @@ mms_connection_test_new(
         }
     }
     test->state = MMS_CONNECTION_STATE_OPENING;
-    g_idle_add(test_connection_test_open, mms_connection_ref(test));
+    mms_connection_test_set_state(test, test->netif ?
+        MMS_CONNECTION_STATE_OPEN : MMS_CONNECTION_STATE_FAILED);
     return test;
 }
 

mms-ofono/src/mms_ofono_connection.c

@@ -237,6 +237,7 @@ mms_ofono_connection_dispose(
 {
     MMSOfonoConnection* ofono = MMS_OFONO_CONNECTION(object);
     MMS_VERBOSE_("%p", ofono);
+    MMS_ASSERT(!mms_connection_is_active(&ofono->connection));
     if (ofono->property_change_signal_id) {
         g_signal_handler_disconnect(ofono->proxy,
             ofono->property_change_signal_id);

mms-ofono/src/mms_ofono_context.c

@@ -25,6 +25,23 @@
 #include "mms_ofono_log.h"
 MMS_LOG_MODULE_DEFINE("mms-ofono-context");
 
+static
+void
+mms_ofono_context_drop_connection(
+    MMSOfonoContext* context,
+    MMS_CONNECTION_STATE state)
+{
+    MMSOfonoConnection* ofono = context->connection;
+    if (ofono) {
+        context->connection = NULL;
+        ofono->context = NULL;
+        if (state != MMS_CONNECTION_STATE_INVALID) {
+            mms_ofono_connection_set_state(ofono, state);
+        }
+        mms_ofono_connection_unref(ofono);
+    }
+}
+
 static
 void
 mms_ofono_context_property_changed(
@@ -43,17 +60,15 @@ mms_ofono_context_property_changed(
             if (context->connection && !mms_ofono_connection_set_state(
                 context->connection, MMS_CONNECTION_STATE_OPEN)) {
                 /* Connection is in a wrong state? */
-                context->connection->context = NULL;
-                mms_ofono_connection_unref(context->connection);
-                context->connection = NULL;
+                mms_ofono_context_drop_connection(context,
+                    MMS_CONNECTION_STATE_INVALID);
             }
             if (!context->connection) {
                 context->connection = mms_ofono_connection_new(context, FALSE);
             }
         } else if (context->connection) {
-            context->connection->context = NULL;
-            mms_ofono_connection_unref(context->connection);
-            context->connection = NULL;
+            mms_ofono_context_drop_connection(context,
+                MMS_CONNECTION_STATE_CLOSED);
         }
     } else {
         MMS_VERBOSE_("%s %s", context->path, key);
@@ -63,7 +78,7 @@ mms_ofono_context_property_changed(
 
 static
 void
-mms_ofono_context_set_active_done(
+mms_ofono_context_activate_done(
     GObject* proxy,
     GAsyncResult* result,
     gpointer user_data)
@@ -75,55 +90,60 @@ mms_ofono_context_set_active_done(
 
     if (!ok) {
         MMSOfonoConnection* ofono = context->connection;
-        if (ofono && ofono->connection.state == MMS_CONNECTION_STATE_OPENING) {
-            /* Connection failed to open, fire state change event and drop
-             * our reference to it */
-            context->connection = NULL;
-            ofono->context = NULL;
+        if (ofono) {
             MMS_ERR("Connection %s failed: %s", ofono->connection.imsi,
                 MMS_ERRMSG(error));
+            if (ofono->connection.state == MMS_CONNECTION_STATE_OPENING) {
+                mms_ofono_context_drop_connection(context,
+                    MMS_CONNECTION_STATE_FAILED);
+            }
+        }
+        g_error_free(error);
+    }
+}
+
+static
+void
+mms_ofono_context_deactivate_done(
+    GObject* proxy,
+    GAsyncResult* result,
+    gpointer user_data)
+{
+    GError* error = NULL;
+    MMSOfonoContext* context = user_data;
+    gboolean ok = org_ofono_connection_context_call_set_property_finish(
+        ORG_OFONO_CONNECTION_CONTEXT(proxy), result, &error);
 
-            mms_ofono_connection_set_state(ofono, MMS_CONNECTION_STATE_FAILED);
-            mms_ofono_connection_unref(ofono);
+    if (!ok) {
+        if (context->connection) {
+            MMS_DEBUG("Connection %s failed tp deactivate: %s",
+                context->connection->connection.imsi, MMS_ERRMSG(error));
         }
+        g_error_free(error);
     }
 
-    if (error) g_error_free(error);
+    /* Regardless of whether or nor deactivation has succeeded, we mark this
+     * connection as closed. */
+    mms_ofono_context_drop_connection(context, MMS_CONNECTION_STATE_CLOSED);
 }
 
 void
 mms_ofono_context_set_active(
     MMSOfonoContext* context,
     gboolean active)
 {
-    GCancellable* cancel;
-    GAsyncReadyCallback cb;
-    gpointer data;
-    if (active) {
-        MMS_DEBUG("Opening connection %s", context->modem->imsi);
-        if (context->set_active_cancel) {
-            g_cancellable_cancel(context->set_active_cancel);
-            g_object_unref(context->set_active_cancel);
-        }
-        cancel = context->set_active_cancel = g_cancellable_new();
-        cb = mms_ofono_context_set_active_done;
-        data = context;
-    } else {
-        MMS_DEBUG("Closing connection %s", context->modem->imsi);
-        cancel = NULL;
-        cb = NULL;
-        data = NULL;
-        if (context->connection) {
-            MMSOfonoConnection* ofono = context->connection;
-            context->connection = NULL;
-            ofono->context = NULL;
-            mms_ofono_connection_set_state(ofono,MMS_CONNECTION_STATE_CLOSED);
-            mms_ofono_connection_unref(ofono);
-        }
+    MMS_DEBUG("%s connection %s", active ? "Opening":"Closing",
+        context->modem->imsi);
+    if (context->set_active_cancel) {
+        g_cancellable_cancel(context->set_active_cancel);
+        g_object_unref(context->set_active_cancel);
     }
+    context->set_active_cancel = g_cancellable_new();
     org_ofono_connection_context_call_set_property(context->proxy,
         OFONO_CONTEXT_PROPERTY_ACTIVE, g_variant_new_variant(
-        g_variant_new_boolean(active)), cancel, cb, data);
+        g_variant_new_boolean(active)), context->set_active_cancel,
+        active ? mms_ofono_context_activate_done :
+        mms_ofono_context_deactivate_done, context);
 }
 
 MMSOfonoContext*