[PATCH 1/1] applet: fix tracking of active access-point

[Thomas Haller <thaller redhat com>]

Caught the following valgrind error on
network-manager-applet-1.2.0-0.3.beta1.fc24:

  == Invalid read of size 8
  ==    at 0x822471D: g_type_check_instance (gtype.c:4137)
  ==    by 0x8218B63: g_signal_handlers_disconnect_matched (gsignal.c:2925)
  ==    by 0x129B3D: update_active_ap (applet-device-wifi.c:1195)
  ==    by 0x129C92: wifi_device_state_changed (applet-device-wifi.c:1219)
  ==    by 0x11C96E: foo_device_state_changed_cb (applet.c:2308)
  ==    by 0xF2FCC57: ffi_call_unix64 (in /usr/lib64/libffi.so.6.0.2)
  ==    by 0xF2FC6B9: ffi_call (in /usr/lib64/libffi.so.6.0.2)
  ==    by 0x81FF279: g_cclosure_marshal_generic_va (gclosure.c:1604)
  ==    by 0x81FE7A6: _g_closure_invoke_va (gclosure.c:867)
  ==    by 0x821A1D7: g_signal_emit_valist (gsignal.c:3294)
  ==    by 0x821A82E: g_signal_emit (gsignal.c:3441)
  ==    by 0x7ED59DC: g_simple_async_result_complete (gsimpleasyncresult.c:801)

This happens, because we hookup the access-point at the device, without
taking any strong references or otherwise ensuring proper lifetime
handling.

Fix that, by registering a weak-ref to the access-point, so that we
notice when the access-point gets destroyed. Note that we don't want
to take strong references, because neither device, access-point nor
applet should keep each other alive only because of an active
access-point.

Also, instead of registering the access-point at the device, register
it at the applet. In principle there could be multiple applet instances
and it is wrong that they all try to register the access-point on the
same device.
---
 src/applet-device-wifi.c | 186 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 155 insertions(+), 31 deletions(-)

diff --git a/src/applet-device-wifi.c b/src/applet-device-wifi.c
index d248566..b7bdd2c 100644
--- a/src/applet-device-wifi.c
+++ b/src/applet-device-wifi.c
@@ -46,6 +46,154 @@ static void _do_new_auto_connection (NMApplet *applet,
                                      AppletNewAutoConnectionCallback callback,
                                      gpointer callback_data);
 
+/*****************************************************************************/
+
+typedef struct {
+       NMApplet *applet;
+       NMDevice *device;
+       NMAccessPoint *ap;
+       gulong signal_id;
+} ActiveAPData;
+
+static void _active_ap_set (NMApplet *applet, NMDevice *device, NMAccessPoint *ap);
+static void _active_ap_set_weakref (gpointer data, GObject *where_the_object_was);
+
+static void
+_active_ap_set_notify (NMAccessPoint *ap, GParamSpec *pspec, gpointer user_data)
+{
+       ActiveAPData *d = user_data;
+
+       g_return_if_fail (NM_IS_ACCESS_POINT (ap));
+       g_return_if_fail (d);
+       g_return_if_fail (NM_IS_APPLET (d->applet));
+       g_return_if_fail (NM_IS_DEVICE (d->device));
+       g_return_if_fail (d->ap == ap);
+       g_return_if_fail (d->signal_id);
+
+       applet_schedule_update_icon (d->applet);
+}
+
+static void
+_active_ap_data_free (ActiveAPData *d)
+{
+       if (d->device)
+               g_object_weak_unref ((GObject *) d->device, _active_ap_set_weakref, d);
+       if (d->ap) {
+               g_object_weak_unref ((GObject *) d->ap, _active_ap_set_weakref, d);
+               g_signal_handler_disconnect (d->ap, d->signal_id);
+       }
+       g_slice_free (ActiveAPData, d);
+}
+
+static NMAccessPoint *
+_active_ap_get (NMApplet *applet, NMDevice *device)
+{
+       GSList *list, *iter;
+
+       g_return_val_if_fail (NM_IS_APPLET (applet), NULL);
+       g_return_val_if_fail (NM_IS_DEVICE (device), NULL);
+
+       list = g_object_get_data ((GObject *) applet, ACTIVE_AP_TAG);
+       for (iter = list; iter; iter = iter->next) {
+               ActiveAPData *d = iter->data;
+
+               if (device == d->device && d->ap)
+                       return d->ap;
+       }
+       return NULL;
+}
+
+static void
+_active_ap_set_destroy (gpointer data)
+{
+       g_slist_free_full (data, (GDestroyNotify) _active_ap_data_free);
+}
+
+static void
+_active_ap_set_weakref (gpointer data, GObject *where_the_object_was)
+{
+       ActiveAPData *d = data;
+
+       if ((GObject *) d->ap == where_the_object_was)
+               d->ap = NULL;
+       else if ((GObject *) d->device == where_the_object_was)
+               d->device = NULL;
+       else
+               g_return_if_reached ();
+       _active_ap_set (d->applet, NULL, NULL);
+
+       applet_schedule_update_icon (d->applet);
+}
+
+static void
+_active_ap_set (NMApplet *applet, NMDevice *device, NMAccessPoint *ap)
+{
+       GSList *list, *iter, *list0, *pcurrent;
+       ActiveAPData *d;
+
+       g_return_if_fail (NM_IS_APPLET (applet));
+       g_return_if_fail (!device || NM_IS_DEVICE (device));
+       g_return_if_fail (!ap || NM_IS_ACCESS_POINT (ap));
+
+       list0 = g_object_get_data ((GObject *) applet, ACTIVE_AP_TAG);
+       list = list0;
+
+remove_empty:
+       pcurrent = NULL;
+       for (iter = list; iter; iter = iter->next) {
+               d = iter->data;
+               if (!d->device || !d->ap) {
+                       _active_ap_data_free (d);
+                       list = g_slist_delete_link (list, iter);
+                       goto remove_empty;
+               }
+               if (device && d->device == device)
+                       pcurrent = iter;
+       }
+
+       if (!device)
+               goto out;
+
+       if (!ap) {
+               if (pcurrent) {
+                       _active_ap_data_free (pcurrent->data);
+                       list = g_slist_delete_link (list, pcurrent);
+               }
+               goto out;
+       }
+
+       if (pcurrent) {
+               d = pcurrent->data;
+
+               if (d->ap == ap)
+                       goto out;
+               g_object_weak_unref ((GObject *) d->ap, _active_ap_set_weakref, d);
+               g_signal_handler_disconnect (d->ap, d->signal_id);
+       } else {
+               d = g_slice_new (ActiveAPData);
+
+               d->applet = applet;
+               d->device = device;
+               g_object_weak_ref ((GObject *) device, _active_ap_set_weakref, d);
+               list = g_slist_append (list, d);
+       }
+       d->ap = ap;
+       g_object_weak_ref ((GObject *) ap, _active_ap_set_weakref, d);
+       d->signal_id = g_signal_connect (ap,
+                                        "notify::" NM_ACCESS_POINT_STRENGTH,
+                                        G_CALLBACK (_active_ap_set_notify),
+                                        d);
+
+out:
+       if (list0 != list) {
+               g_object_replace_data ((GObject *) applet, ACTIVE_AP_TAG,
+                                      list0, list,
+                                      _active_ap_set_destroy, NULL);
+       }
+}
+
+/*****************************************************************************/
+
 static void
 show_ignore_focus_stealing_prevention (GtkWidget *widget)
 {
@@ -1065,7 +1213,7 @@ access_point_added_cb (NMDeviceWifi *device,
                          "notify",
                          G_CALLBACK (notify_ap_prop_changed_cb),
                          applet);
-       
+
        queue_avail_access_point_notification (NM_DEVICE (device));
        applet_schedule_update_menu (applet);
 }
@@ -1081,9 +1229,9 @@ access_point_removed_cb (NMDeviceWifi *device,
        /* If this AP was the active AP, make sure ACTIVE_AP_TAG gets cleared from
         * its device.
         */
-       old = g_object_get_data (G_OBJECT (device), ACTIVE_AP_TAG);
+       old = _active_ap_get (applet, (NMDevice *) device);
        if (old == ap) {
-               g_object_set_data (G_OBJECT (device), ACTIVE_AP_TAG, NULL);
+               _active_ap_set (applet, (NMDevice *) device, NULL);
                applet_schedule_update_icon (applet);
                applet_schedule_update_menu (applet);
        }
@@ -1163,16 +1311,10 @@ wifi_device_added (NMDevice *device, NMApplet *applet)
                add_hash_to_ap (g_ptr_array_index (aps, i));
 }
 
-static void
-bssid_strength_changed (NMAccessPoint *ap, GParamSpec *pspec, gpointer user_data)
-{
-       applet_schedule_update_icon (NM_APPLET (user_data));
-}
-
 static NMAccessPoint *
 update_active_ap (NMDevice *device, NMDeviceState state, NMApplet *applet)
 {
-       NMAccessPoint *new = NULL, *old;
+       NMAccessPoint *new = NULL;
 
        if (state == NM_DEVICE_STATE_PREPARE ||
            state == NM_DEVICE_STATE_CONFIG ||
@@ -1182,25 +1324,7 @@ update_active_ap (NMDevice *device, NMDeviceState state, NMApplet *applet)
                new = nm_device_wifi_get_active_access_point (NM_DEVICE_WIFI (device));
        }
 
-       old = g_object_get_data (G_OBJECT (device), ACTIVE_AP_TAG);
-       if (new && (new == old))
-               return new;   /* no change */
-
-       if (old) {
-               g_signal_handlers_disconnect_by_func (old, G_CALLBACK (bssid_strength_changed), applet);
-               g_object_set_data (G_OBJECT (device), ACTIVE_AP_TAG, NULL);
-       }
-
-       if (new) {
-               g_object_set_data (G_OBJECT (device), ACTIVE_AP_TAG, new);
-
-               /* monitor this AP's signal strength for updating the applet icon */
-               g_signal_connect (new,
-                                 "notify::" NM_ACCESS_POINT_STRENGTH,
-                                 G_CALLBACK (bssid_strength_changed),
-                                 applet);
-       }
-
+       _active_ap_set (applet, device, new);
        return new;
 }
 
@@ -1227,7 +1351,7 @@ wifi_notify_connected (NMDevice *device,
        char *ssid_msg;
        const char *signal_strength_icon;
 
-       ap = g_object_get_data (G_OBJECT (device), ACTIVE_AP_TAG);
+       ap = _active_ap_get (applet, device);
 
        esc_ssid = get_ssid_utf8 (ap);
 
@@ -1261,7 +1385,7 @@ wifi_get_icon (NMDevice *device,
        g_return_if_fail (out_icon_name && !*out_icon_name);
        g_return_if_fail (tip && !*tip);
 
-       ap = g_object_get_data (G_OBJECT (device), ACTIVE_AP_TAG);
+       ap = _active_ap_get (applet, device);
 
        id = nm_device_get_iface (device);
        if (connection) {
-- 
2.5.0