GNOME Bugzilla – Bug 741397
g_dbus_method_invocation_return_value() twice for the same GDBusMethodInvocation explodes randomly
Last modified: 2018-05-24 17:19:39 UTC
Due to a bug in my code, I was calling g_dbus_method_invocation_return_value() twice for the same GDBusMethodInvocation on one rarely-used conditional. Rather than asserting (or something else sane) I got semi-random backtraces that looked like: > g_source_callback_unref (cb_data) at gmain.c:1532 > g_source_destroy_internal (source, context, have_lock=1) at gmain.c:1178 > g_main_context_dispatch (context) at gmain.c:3134 > (gdb) p g_source_get_name(source) > $1 = '[gio] call_in_idle_cb' This took me *HOURS* to track down. Something like g_assert (invocation->has_not_returned_yet) would have been awesome for debugging, rather than poking around with gdb and banging my head against the wall.
This is unfixable without breaking API. It was basically a poor design choice (imho) with how we treat invocations. They are marked as 'transfer full' both on handing to the user's method call handler and on calling _return_value(). This means that you automatically get a ref at the start of the call and you lose it when you do _return_value(). Doing _return_value() twice is the same as unreffing twice, which is why it explodes. The better interface here would be to get rid of both (transfer full). That would solve quite a lot of problems: 1) we could reliably g_warning() if you forget to reply by the time your method call handler returns (unless you explicitly took a ref to indicate that you will handle it later) 2) we could warn on sending more than one reply without exploding 3) the current way this works is just extremely confusing... The problem is that we probably would break existing applications that rely on the current behaviour (ie: not having to take a ref on the thing in order to deal with it later). The only thing I can imagine us doing here is to keep the current behaviour in implementation but switch to the new behaviour in theory: 1) change the annotations and remove the (transfer full) stuff everywhere 2) give a critical at the end of the method call handler if you did not either handle the invocation or increase its refcount. This would throw criticals on existing good code, but it would be a way to get them to move over to the 'new official behaviour' 3) throw a critical and do nothing on any second call to _return_value() which would prevent multiple returns from freeing the object more than once. Returning more than once is already undefined behaviour, so that's OK. The result would be that pretty much anyone who is using the API in bad ways would see at least one critical about it. The downside is that this would include people who are currently using the API correctly as of today....
Some annoying considerations: Under the new regime, one might imagine that someone (validly) handles their D-Bus messages by taking a ref on the invocation and sending it off to another thread for handling. If that other thread is fast, it could drop the ref on the object before our method handler in the main thread has a chance to return (at which point we check the refcount). We have to take care for that case. It's also theoretically possible under the new regime that it is valid not to reply to an invocation at all (ie: in the case of the no-reply-required flag). It will be almost impossible to tell that (intentionally not replying) case apart from the case where the caller has held onto the invocation without explicitly taking the ref. One thing that we could do is to take note if the user has inspected the flags field on the message since receiving the invocation (which they would have to do in order to know that they can skip the reply), but now we're into some really deeply messed up black magic...
A solution, I think: By the time the method handler has returned, at least one of the following must be true: 1) the refcount is more than 1; or 2) a value has been returned; or 3) the user has called a new g_dbus_method_invocation_is_reply_required() function and the result was false If any of those are false, we leave the user with their ref and throw the warning. Through explicit use of the new function, the user is opting in to the new behaviour. This means that we trap another class of users in our g_warning() spam: those who checked the message flags "the old way" in order to decide not to reply. That's better than crashing, though.
s/If any of those are false/If all of those are false/
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/970.