plugin-manager: disallow port additions to an already cancelled device context

If the DeviceContext is cancelled, we remove it from the list of
tracked DeviceContexts in the MMPluginManager, so that new port
additions happening at the same time don't end up being added to
it.

This should allow an early cancellation of the DeviceContext without
asserting on g_assert (!device_context->extra_probing_time_id) as
there is no chance that a new port addition has reseted the extra
probing time timeout.

    0x00007c1154f16abf (libc.so.6 - pthread_kill.c: 44)	__pthread_kill_implementation
    0x00007c1154ecbf0c (libc.so.6 - raise.c: 26)	raise
    0x00007c1154eb74aa (libc.so.6 - abort.c: 79)	abort
    0x00007c11556c08a9 (libglib-2.0.so.0 - gtestutils.c: 3450)	g_assertion_message
    0x00007c11556c091d (libglib-2.0.so.0 - gtestutils.c: 3476)	g_assertion_message_expr
    0x00005897d1ed6e7b (ModemManager - mm-plugin-manager.c: 805)	device_context_unref
    0x00005897d1ed6ab9 (ModemManager - mm-plugin-manager.c: 163)	common_async_context_free
    0x00005897d1ed5dd6 (ModemManager - mm-plugin-manager.c: 1136)	port_context_run_ready
    0x00007c1155550e86 (libgio-2.0.so.0 - gtask.c: 1309)	g_task_return_now
    0x00007c115554fe50 (libgio-2.0.so.0 - gtask.c: 1378)	g_task_return
    0x00007c115555047b (libgio-2.0.so.0 - gtask.c: 2022)	g_task_return_new_error
    0x00005897d1ed581a (ModemManager - mm-plugin-manager.c: 296)	port_context_complete
    0x00005897d1ed6259 (ModemManager - mm-plugin-manager.c)	plugin_supports_port_ready
    0x00007c1155550e86 (libgio-2.0.so.0 - gtask.c: 1309)	g_task_return_now
    0x00007c1155550eb9 (libgio-2.0.so.0 - gtask.c: 1323)	complete_in_idle_cb
    0x00007c115569c91b (libglib-2.0.so.0 - gmain.c: 3460)	g_main_dispatch
    0x00007c115569c91b (libglib-2.0.so.0 - gmain.c: 4200)	g_main_context_dispatch
    0x00007c115569cc37 (libglib-2.0.so.0 - gmain.c: 4276)	g_main_context_iterate
    0x00007c115569ceb5 (libglib-2.0.so.0 - gmain.c: 4479)	g_main_loop_run
    0x00005897d1e715bf (ModemManager - main.c: 236)	main
    0x00007c1154eb7705 (libc.so.6 - libc_start_call_main.h: 58)	__libc_start_call_main
    0x00007c1154eb77c1 (libc.so.6 - libc-start.c: 360)	__libc_start_main_impl
    0x00005897d1e70f10 (ModemManager + 0x000b3f10)	_start
    0x00007ffd5acc5057

Manually reproducing this issue was challenging, so adding here the
steps taken, for reference. This is with ModemManager running with
--no-udev (i.e. port additions reported via mmcli). The key to
reproduce the problem is the different timing between the port
additions and removals of the three ttyACM ports.

  COUNT=1
  while ((COUNT < 10000)); do
      echo "$COUNT..."
      COUNT=$((COUNT + 1))
      (
          sudo mmcli --report-kernel-event="name=ttyACM0,subsystem=tty,action=add"
          TIMEOUT="0.0$(((RANDOM % 10) + 1))"
          sleep $TIMEOUT
          sudo mmcli --report-kernel-event="name=ttyACM0,subsystem=tty,action=remove"
      ) &
      (
          sudo mmcli --report-kernel-event="name=ttyACM1,subsystem=tty,action=add"
          TIMEOUT="0.0$(((RANDOM % 10) + 1))"
          sleep $TIMEOUT
          sudo mmcli --report-kernel-event="name=ttyACM1,subsystem=tty,action=remove"
      ) &
      (
          sudo mmcli --report-kernel-event="name=ttyACM2,subsystem=tty,action=add"
          TIMEOUT="0.0$(((RANDOM % 10) + 1))"
          sleep $TIMEOUT
          sudo mmcli --report-kernel-event="name=ttyACM2,subsystem=tty,action=remove"
      ) &
      TIMEOUT="0.0$(((RANDOM % 10) + 1))"
      sleep $TIMEOUT
      COUNT=$((COUNT + 1))
  done

1 files changed, 36 insertions, 25 deletions

diff --git a/src/mm-plugin-manager.c b/src/mm-plugin-manager.c
index f34db194..f8cf0097 100644
--- a/src/mm-plugin-manager.c
+++ b/src/mm-plugin-manager.c
@@ -1326,26 +1326,16 @@ device_context_port_added (MMPluginManager *self,
 }
 
 static gboolean
-device_context_cancel (DeviceContext *_device_context)
+device_context_cancel (DeviceContext *device_context)
 {
-    g_autoptr(DeviceContext)  device_context = NULL;
-    MMPluginManager          *self;
-
-    /* The device context cancellation operation will also independently request cancellation
-     * of each port context. Depending on the probing task of the port, this may be completed
-     * asynchronously or in-place. Therefore we need to consider the case where the last port
-     * context cancellation also completes the device context operation in-place, so we must
-     * ensure the DeviceContext is valid throughout the whole method. */
-    device_context = device_context_ref (_device_context);
-
-    /* If cancelled already, do nothing */
-    if (g_cancellable_is_cancelled (device_context->cancellable))
-        return FALSE;
+    MMPluginManager *self;
 
+    g_assert (device_context->task);
     self = g_task_get_source_object (device_context->task);
     mm_obj_dbg (self, "task %s: cancellation requested", device_context->name);
 
     /* The device context is cancelled now */
+    g_assert (!g_cancellable_is_cancelled (device_context->cancellable));
     g_cancellable_cancel (device_context->cancellable);
 
     /* Remove all port contexts in the waiting list. This will allow early cancellation
@@ -1379,8 +1369,7 @@ device_context_cancel (DeviceContext *_device_context)
     /* If the device context task is not yet completed, wakeup the device context
      * logic. If we were still waiting for the min probing time, this will complete
      * the device context. */
-    if (device_context->task)
-        device_context_continue (device_context);
+    device_context_continue (device_context);
     return TRUE;
 }
 
@@ -1500,6 +1489,19 @@ plugin_manager_peek_device_context (MMPluginManager *self,
 }
 
 static void
+plugin_manager_untrack_device_context (MMPluginManager *self,
+                                       DeviceContext   *device_context)
+{
+    GList *found;
+
+    found = g_list_find (self->priv->device_contexts, device_context);
+    if (found) {
+        self->priv->device_contexts = g_list_remove_link (self->priv->device_contexts, found);
+        g_list_free_full (found, (GDestroyNotify)device_context_unref);
+    }
+}
+
+static void
 device_context_run_ready (MMPluginManager    *self,
                           GAsyncResult       *res,
                           CommonAsyncContext *common)
@@ -1512,13 +1514,11 @@ device_context_run_ready (MMPluginManager    *self,
 
     /*
      * Once the task is finished, we can also remove it from the plugin manager
-     * list. We MUST have the device context in the list at this point, because
-     * we're going to dispose the reference, so assert if this is not true.
+     * list. If the device support task was early cancelled, e.g. if the last
+     * port of an existing device is removed, the device context may not exist
+     * in the list tracked by the plugin manager.
      */
-    g_assert (g_list_find (common->self->priv->device_contexts, common->device_context));
-    common->self->priv->device_contexts = g_list_remove (common->self->priv->device_contexts,
-                                                         common->device_context);
-    device_context_unref (common->device_context);
+    plugin_manager_untrack_device_context (self, common->device_context);
 
     /* Report result or error once removed from our internal list */
     if (!best_plugin)
@@ -1582,14 +1582,25 @@ gboolean
 mm_plugin_manager_device_support_check_cancel (MMPluginManager *self,
                                                MMDevice        *device)
 {
-    DeviceContext *device_context;
+    g_autoptr(DeviceContext) device_context = NULL;
 
-    /* If the device context isn't found, ignore the cancellation request. */
+    /* If the device context isn't found, ignore the cancellation request */
     device_context = plugin_manager_peek_device_context (self, device);
     if (!device_context)
         return FALSE;
 
-    /* Request cancellation, will be completed asynchronously */
+    /* The device context cancellation operation will also independently request cancellation
+     * of each port context. Depending on the probing task of the port, this may be completed
+     * asynchronously or in-place. Therefore we need to consider the case where the last port
+     * context cancellation also completes the device context operation in-place, so we must
+     * ensure the DeviceContext is valid throughout the whole cancellation. */
+    device_context = device_context_ref (device_context);
+
+    /* Remove from the tracked list of device contexts, so that we never add
+     * new ports to a device context being cancelled */
+    plugin_manager_untrack_device_context (self, device_context);
+
+    /* Request cancellation, will be completed asynchronously. */
     return device_context_cancel (device_context);
 }