GNOME Bugzilla – Bug 690400
Cannot unsubscribe from notification in notify handler
Last modified: 2014-01-21 13:09:06 UTC
Quoting https://mail.gnome.org/archives/gupnp-list/2012-December/msg00010.html we've identified a problem with the GUPnPServiceProxy API that I would like to bring to your attention. The current implementation of notifications in GUPnPServiceProxy does not deal nicely with code that calls gupnp_service_proxy_remove_notify() from a notification callback. Actually it will most certainly crash if you attempt to do this. What happens is that we are inside this emit_notification() looping over registered callbacks that want to get notified about a change of a state variable: NotifyData *data; GList *l; data = g_hash_table_lookup (proxy->priv->notify_hash, var_node->name); if (data == NULL) return; /* some not relevant code removed here... */ /* Call callbacks */ for (l = data->callbacks; l; l = l->next) { CallbackData *callback_data; callback_data = l->data; callback_data->callback (proxy, (const char *) var_node->name, &value, callback_data->user_data); } Now if the callback uses gupnp_service_proxy_add_notify() or gupnp_service_proxy_remove_notify() it will change the list of callbacks in NotifyData while we are iterating it. Probably the most common case (and the one that we hit in our code) is that the callback removes itself from the service-proxy. In that case the GList struct that we are holding a pointer to will be released. This is likely going to cause a crash when the for-loop accesses l->next.
I have a patch for this but I'll wait until the test changes in bug 678701 are in so I can add a test as well.
Created attachment 266747 [details] [review] tests: Add test for #690400 Test that removing a notify-callback from the callback itself works.
Created attachment 266748 [details] [review] Survive ServiceProxy callback-list changes from callbacks Make notify-callback code more robust, so that removing a notify-callback from a notify-callback works.
Created attachment 266749 [details] [review] tests: refactor main loop handling to one function
Review of attachment 266748 [details] [review]: This patch should also remove the doc paragraph in gupnp_service_proxy_remove_notify that says that you cannot do this or up until which version it wasn't possible to do this or sth like that. Otherwise, +1 ::: libgupnp/gupnp-service-proxy.c @@ +1632,3 @@ + if (data->next_emit == NULL) + data->next_emit = g_list_last (data->callbacks); + Why is next_emit set to the newest notify added?
Review of attachment 266747 [details] [review]: +1
Review of attachment 266749 [details] [review]: +1
(In reply to comment #5) > Review of attachment 266748 [details] [review]: > > This patch should also remove the doc paragraph in > gupnp_service_proxy_remove_notify that says that you cannot do this or up until > which version it wasn't possible to do this or sth like that. Good catch. > ::: libgupnp/gupnp-service-proxy.c > @@ +1632,3 @@ > + if (data->next_emit == NULL) > + data->next_emit = g_list_last (data->callbacks); > + > > Why is next_emit set to the newest notify added? It makes sure we do things consistently: If next_emit is NULL it means either A) we're not in emit_notification() (in which case setting next_emit does not affect anything) or B) we're in emit_notification() calling the last callback, in which case we want to call the new callback too -- not because it makes so much sense but because it's consistent with what happens when callbacks are added from callbacks earlier in the list.
Ah. Thanks for the explanation.
Committed as 17442e3876fa85a861b5624d51a9211fef15112d bf151a712020250461ceb1f43e7bcf949466800c 4ad119c7186c690aa96aa34c64199c874ec33dbb