webrtc: Improve robustness of nice agent signal handlers

NiceAgent and it's associated thread is alive for as long as
GstWebRTCICE is alive so make sure any signal handlers connected to
NiceAgent do not access data that is deleted earlier.

Part-of: <https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/2073>

subprojects/gst-plugins-bad/ext/webrtc/gstwebrtcice.c

@@ -855,8 +855,12 @@ _clear_ice_stream (struct NiceStreamItem *item)
     return;
 
   if (item->stream) {
-    g_signal_handlers_disconnect_by_data (item->stream->ice->priv->nice_agent,
+    GstWebRTCICE *ice = g_weak_ref_get (&item->stream->ice_weak);
-        item->stream);
+    if (ice != NULL) {
+      g_signal_handlers_disconnect_by_data (ice->priv->nice_agent,
+          item->stream);
+      gst_object_unref (ice);
+    }
     gst_object_unref (item->stream);
   }
 }

subprojects/gst-plugins-bad/ext/webrtc/icestream.c

@@ -47,6 +47,7 @@ struct _GstWebRTCICEStreamPrivate
   gboolean gathered;
   GList *transports;
   gboolean gathering_started;
+  gulong candidate_gathering_done_id;
 };
 
 #define gst_webrtc_ice_stream_parent_class parent_class
@@ -63,8 +64,7 @@ gst_webrtc_ice_stream_set_property (GObject * object, guint prop_id,
 
   switch (prop_id) {
     case PROP_ICE:
-      /* XXX: weak-ref this? */
+      g_weak_ref_set (&stream->ice_weak, g_value_get_object (value));
-      stream->ice = g_value_get_object (value);
       break;
     case PROP_STREAM_ID:
       stream->stream_id = g_value_get_uint (value);
@@ -83,7 +83,7 @@ gst_webrtc_ice_stream_get_property (GObject * object, guint prop_id,
 
   switch (prop_id) {
     case PROP_ICE:
-      g_value_set_object (value, stream->ice);
+      g_value_set_object (value, g_weak_ref_get (&stream->ice_weak));
       break;
     case PROP_STREAM_ID:
       g_value_set_uint (value, stream->stream_id);
@@ -98,22 +98,42 @@ static void
 gst_webrtc_ice_stream_finalize (GObject * object)
 {
   GstWebRTCICEStream *stream = GST_WEBRTC_ICE_STREAM (object);
+  GstWebRTCICE *ice = g_weak_ref_get (&stream->ice_weak);
+
+  if (ice) {
+    NiceAgent *agent;
+    g_object_get (ice, "agent", &agent, NULL);
+
+    if (stream->priv->candidate_gathering_done_id != 0) {
+      g_signal_handler_disconnect (agent,
+          stream->priv->candidate_gathering_done_id);
+    }
+
+    g_object_unref (agent);
+    gst_object_unref (ice);
+  }
 
   g_list_free (stream->priv->transports);
   stream->priv->transports = NULL;
 
+  g_weak_ref_clear (&stream->ice_weak);
+
   G_OBJECT_CLASS (parent_class)->finalize (object);
 }
 
 static void
 _on_candidate_gathering_done (NiceAgent * agent, guint stream_id,
-    GstWebRTCICEStream * ice)
+    GWeakRef * ice_weak)
 {
+  GstWebRTCICEStream *ice = g_weak_ref_get (ice_weak);
   GList *l;
 
-  if (stream_id != ice->stream_id)
+  if (!ice)
     return;
 
+  if (stream_id != ice->stream_id)
+    goto cleanup;
+
   GST_DEBUG_OBJECT (ice, "%u gathering done", stream_id);
 
   ice->priv->gathered = TRUE;
@@ -124,6 +144,9 @@ _on_candidate_gathering_done (NiceAgent * agent, guint stream_id,
     gst_webrtc_ice_transport_gathering_state_change (ice,
         GST_WEBRTC_ICE_GATHERING_STATE_COMPLETE);
   }
+
+cleanup:
+  gst_object_unref (ice);
 }
 
 GstWebRTCICETransport *
@@ -152,17 +175,36 @@ gst_webrtc_ice_stream_find_transport (GstWebRTCICEStream * stream,
   return ret;
 }
 
+static GWeakRef *
+weak_new (GstWebRTCICEStream * stream)
+{
+  GWeakRef *weak = g_new0 (GWeakRef, 1);
+  g_weak_ref_init (weak, stream);
+  return weak;
+}
+
+static void
+weak_free (GWeakRef * weak)
+{
+  g_weak_ref_clear (weak);
+  g_free (weak);
+}
+
 static void
 gst_webrtc_ice_stream_constructed (GObject * object)
 {
   GstWebRTCICEStream *stream = GST_WEBRTC_ICE_STREAM (object);
   NiceAgent *agent;
+  GstWebRTCICE *ice = g_weak_ref_get (&stream->ice_weak);
 
-  g_object_get (stream->ice, "agent", &agent, NULL);
+  g_assert (ice != NULL);
-  g_signal_connect (agent, "candidate-gathering-done",
+  g_object_get (ice, "agent", &agent, NULL);
-      G_CALLBACK (_on_candidate_gathering_done), stream);
+  stream->priv->candidate_gathering_done_id = g_signal_connect_data (agent,
+      "candidate-gathering-done", G_CALLBACK (_on_candidate_gathering_done),
+      weak_new (stream), (GClosureNotify) weak_free, (GConnectFlags) 0);
 
   g_object_unref (agent);
+  gst_object_unref (ice);
 
   G_OBJECT_CLASS (parent_class)->constructed (object);
 }
@@ -172,6 +214,8 @@ gst_webrtc_ice_stream_gather_candidates (GstWebRTCICEStream * stream)
 {
   NiceAgent *agent;
   GList *l;
+  GstWebRTCICE *ice;
+  gboolean ret = TRUE;
 
   g_return_val_if_fail (GST_IS_WEBRTC_ICE_STREAM (stream), FALSE);
 
@@ -187,28 +231,31 @@ gst_webrtc_ice_stream_gather_candidates (GstWebRTCICEStream * stream)
         GST_WEBRTC_ICE_GATHERING_STATE_GATHERING);
   }
 
-  g_object_get (stream->ice, "agent", &agent, NULL);
+  ice = g_weak_ref_get (&stream->ice_weak);
+  g_assert (ice != NULL);
+
+  g_object_get (ice, "agent", &agent, NULL);
 
   if (!stream->priv->gathering_started) {
-    if (stream->ice->min_rtp_port != 0 || stream->ice->max_rtp_port != 65535) {
+    if (ice->min_rtp_port != 0 || ice->max_rtp_port != 65535) {
-      if (stream->ice->min_rtp_port > stream->ice->max_rtp_port) {
+      if (ice->min_rtp_port > ice->max_rtp_port) {
-        GST_ERROR_OBJECT (stream->ice,
+        GST_ERROR_OBJECT (ice,
             "invalid port range: min-rtp-port %d must be <= max-rtp-port %d",
-            stream->ice->min_rtp_port, stream->ice->max_rtp_port);
+            ice->min_rtp_port, ice->max_rtp_port);
-        return FALSE;
+        ret = FALSE;
+        goto cleanup;
       }
 
       nice_agent_set_port_range (agent, stream->stream_id,
-          NICE_COMPONENT_TYPE_RTP, stream->ice->min_rtp_port,
+          NICE_COMPONENT_TYPE_RTP, ice->min_rtp_port, ice->max_rtp_port);
-          stream->ice->max_rtp_port);
     }
     /* mark as gathering started to prevent changing ports again */
     stream->priv->gathering_started = TRUE;
   }
 
   if (!nice_agent_gather_candidates (agent, stream->stream_id)) {
-    g_object_unref (agent);
+    ret = FALSE;
-    return FALSE;
+    goto cleanup;
   }
 
   for (l = stream->priv->transports; l; l = l->next) {
@@ -217,8 +264,13 @@ gst_webrtc_ice_stream_gather_candidates (GstWebRTCICEStream * stream)
     gst_webrtc_nice_transport_update_buffer_size (trans);
   }
 
-  g_object_unref (agent);
+cleanup:
-  return TRUE;
+  if (agent)
+    g_object_unref (agent);
+  if (ice)
+    gst_object_unref (ice);
+
+  return ret;
 }
 
 static void
@@ -247,9 +299,11 @@ gst_webrtc_ice_stream_class_init (GstWebRTCICEStreamClass * klass)
 }
 
 static void
-gst_webrtc_ice_stream_init (GstWebRTCICEStream * ice)
+gst_webrtc_ice_stream_init (GstWebRTCICEStream * stream)
 {
-  ice->priv = gst_webrtc_ice_stream_get_instance_private (ice);
+  stream->priv = gst_webrtc_ice_stream_get_instance_private (stream);
+
+  g_weak_ref_init (&stream->ice_weak, NULL);
 }
 
 GstWebRTCICEStream *

subprojects/gst-plugins-bad/ext/webrtc/icestream.h

@@ -40,7 +40,7 @@ struct _GstWebRTCICEStream
 {
   GstObject                 parent;
 
-  GstWebRTCICE             *ice;
+  GWeakRef                  ice_weak;
 
   guint                     stream_id;
 

subprojects/gst-plugins-bad/ext/webrtc/nicetransport.c

@@ -51,6 +51,8 @@ struct _GstWebRTCNiceTransportPrivate
 
   gint send_buffer_size;
   gint receive_buffer_size;
+  gulong on_new_selected_pair_id;
+  gulong on_component_state_changed_id;
 };
 
 #define gst_webrtc_nice_transport_parent_class parent_class
@@ -162,6 +164,24 @@ static void
 gst_webrtc_nice_transport_finalize (GObject * object)
 {
   GstWebRTCNiceTransport *nice = GST_WEBRTC_NICE_TRANSPORT (object);
+  NiceAgent *agent;
+  GstWebRTCICE *webrtc_ice = g_weak_ref_get (&nice->stream->ice_weak);
+
+  if (webrtc_ice) {
+    g_object_get (webrtc_ice, "agent", &agent, NULL);
+
+    if (nice->priv->on_component_state_changed_id != 0) {
+      g_signal_handler_disconnect (agent,
+          nice->priv->on_component_state_changed_id);
+    }
+
+    if (nice->priv->on_new_selected_pair_id != 0) {
+      g_signal_handler_disconnect (agent, nice->priv->on_new_selected_pair_id);
+    }
+
+    g_object_unref (agent);
+    gst_object_unref (webrtc_ice);
+  }
 
   gst_object_unref (nice->stream);
 
@@ -174,13 +194,17 @@ gst_webrtc_nice_transport_update_buffer_size (GstWebRTCNiceTransport * nice)
   NiceAgent *agent = NULL;
   GPtrArray *sockets;
   guint i;
+  GstWebRTCICE *webrtc_ice = g_weak_ref_get (&nice->stream->ice_weak);
 
-  g_object_get (nice->stream->ice, "agent", &agent, NULL);
+  g_assert (webrtc_ice != NULL);
+
+  g_object_get (webrtc_ice, "agent", &agent, NULL);
   g_assert (agent != NULL);
 
   sockets = nice_agent_get_sockets (agent, nice->stream->stream_id, 1);
   if (sockets == NULL) {
     g_object_unref (agent);
+    gst_object_unref (webrtc_ice);
     return;
   }
 
@@ -209,49 +233,82 @@ gst_webrtc_nice_transport_update_buffer_size (GstWebRTCNiceTransport * nice)
   }
   g_ptr_array_unref (sockets);
   g_object_unref (agent);
+  gst_object_unref (webrtc_ice);
 }
 
 
 static void
 _on_new_selected_pair (NiceAgent * agent, guint stream_id,
     NiceComponentType component, NiceCandidate * lcandidate,
-    NiceCandidate * rcandidate, GstWebRTCNiceTransport * nice)
+    NiceCandidate * rcandidate, GWeakRef * nice_weak)
 {
-  GstWebRTCICETransport *ice = GST_WEBRTC_ICE_TRANSPORT (nice);
+  GstWebRTCNiceTransport *nice = g_weak_ref_get (nice_weak);
+  GstWebRTCICETransport *ice;
   GstWebRTCICEComponent comp = _nice_component_to_gst (component);
   guint our_stream_id;
 
+  if (!nice)
+    return;
+
+  ice = GST_WEBRTC_ICE_TRANSPORT (nice);
+
   g_object_get (nice->stream, "stream-id", &our_stream_id, NULL);
 
   if (stream_id != our_stream_id)
-    return;
+    goto cleanup;
   if (comp != ice->component)
-    return;
+    goto cleanup;
 
   gst_webrtc_ice_transport_selected_pair_change (ice);
+
+cleanup:
+  gst_object_unref (nice);
 }
 
 static void
 _on_component_state_changed (NiceAgent * agent, guint stream_id,
-    NiceComponentType component, NiceComponentState state,
+    NiceComponentType component, NiceComponentState state, GWeakRef * nice_weak)
-    GstWebRTCNiceTransport * nice)
 {
-  GstWebRTCICETransport *ice = GST_WEBRTC_ICE_TRANSPORT (nice);
+  GstWebRTCNiceTransport *nice = g_weak_ref_get (nice_weak);
+  GstWebRTCICETransport *ice;
   GstWebRTCICEComponent comp = _nice_component_to_gst (component);
   guint our_stream_id;
 
+  if (!nice)
+    return;
+
+  ice = GST_WEBRTC_ICE_TRANSPORT (nice);
+
   g_object_get (nice->stream, "stream-id", &our_stream_id, NULL);
 
   if (stream_id != our_stream_id)
-    return;
+    goto cleanup;
   if (comp != ice->component)
-    return;
+    goto cleanup;
 
   GST_DEBUG_OBJECT (ice, "%u %u %s", stream_id, component,
       nice_component_state_to_string (state));
 
   gst_webrtc_ice_transport_connection_state_change (ice,
       _nice_component_state_to_gst (state));
+
+cleanup:
+  gst_object_unref (nice);
+}
+
+static GWeakRef *
+weak_new (GstWebRTCNiceTransport * nice)
+{
+  GWeakRef *weak = g_new0 (GWeakRef, 1);
+  g_weak_ref_init (weak, nice);
+  return weak;
+}
+
+static void
+weak_free (GWeakRef * weak)
+{
+  g_weak_ref_clear (weak);
+  g_free (weak);
 }
 
 static void
@@ -263,19 +320,23 @@ gst_webrtc_nice_transport_constructed (GObject * object)
   gboolean controlling_mode;
   guint our_stream_id;
   NiceAgent *agent;
+  GstWebRTCICE *webrtc_ice = g_weak_ref_get (&nice->stream->ice_weak);
 
+  g_assert (webrtc_ice != NULL);
   g_object_get (nice->stream, "stream-id", &our_stream_id, NULL);
-  g_object_get (nice->stream->ice, "agent", &agent, NULL);
+  g_object_get (webrtc_ice, "agent", &agent, NULL);
 
   g_object_get (agent, "controlling-mode", &controlling_mode, NULL);
   ice->role =
       controlling_mode ? GST_WEBRTC_ICE_ROLE_CONTROLLING :
       GST_WEBRTC_ICE_ROLE_CONTROLLED;
 
-  g_signal_connect (agent, "component-state-changed",
+  nice->priv->on_component_state_changed_id = g_signal_connect_data (agent,
-      G_CALLBACK (_on_component_state_changed), nice);
+      "component-state-changed", G_CALLBACK (_on_component_state_changed),
-  g_signal_connect (agent, "new-selected-pair-full",
+      weak_new (nice), (GClosureNotify) weak_free, (GConnectFlags) 0);
-      G_CALLBACK (_on_new_selected_pair), nice);
+  nice->priv->on_new_selected_pair_id = g_signal_connect_data (agent,
+      "new-selected-pair-full", G_CALLBACK (_on_new_selected_pair),
+      weak_new (nice), (GClosureNotify) weak_free, (GConnectFlags) 0);
 
   ice->src = gst_element_factory_make ("nicesrc", NULL);
   if (ice->src) {
@@ -290,6 +351,7 @@ gst_webrtc_nice_transport_constructed (GObject * object)
   }
 
   g_object_unref (agent);
+  gst_object_unref (webrtc_ice);
 
   G_OBJECT_CLASS (parent_class)->constructed (object);
 }