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 _______________________________________________ networkmanager-list mailing list networkmanager-list@gnome.org https://mail.gnome.org/mailman/listinfo/networkmanager-list