GNOME Bugzilla – Bug 725950
GApplication: call dbus_unregister only once, and before destruction
Last modified: 2020-02-27 14:56:39 UTC
Shutting down the application calls out to user code through dbus_unregister(), and is therefore unsafe to do in finalize(). Do it in dispose instead.
Created attachment 271329 [details] [review] GApplication: destroy the implementation in dispose, not finalize
Why is this unsafe?
If the vfunc is implemented in JS you risk calling JS code while finalizing, which is not allowed. I suspect python has a similar problem.
(In reply to comment #3) > If the vfunc is implemented in JS you risk calling JS code while finalizing, > which is not allowed. I suspect python has a similar problem. Makes sense -- except that we'd have the same risk from dispose, wouldn't we? By the time dispose is running, the last ref was already dropped... fwiw, I think we ought to be calling dbus_unregister() from shutdown() only. If it doesn't happen by then then it should just never happen. That's probably the correct spot to free the impl as well...
Yes, but if the unsafe code runs in dispose(), I can call run_dispose() beforehand. I thought of shutdown, but I assumed existing code wants to call run() multiple times, or do something with the app between run() and unref
Is this still something we want ?
This is still something that creates problems if a JS class wants to override dbus_unregister, but it's not an urgent issue because in practice there is no need to free resources as the process is exiting anyway.
This commit for bug 757372 has changed things a bit: https://git.gnome.org/browse/glib/commit/?id=21b1c390a3ce1f7e2816c6309f161c4b92470c46 Right now, the first invocation of dbus_unregister always happens before finalize. But it is not just JS. It was pretty weird for C code too. If the dbus_register vfunc is called from finalize, then, in terms of C, that's after the GApplication sub-class' dispose and finalize implementations have been run. People usually drop the references to their instance objects by then, and the exported objects are usually just that. So it is surprising to have the D-Bus unexport code called after that. Secondly, do we expect people to guard against dbus_unregister being called multiple times? It can happen if you tried to spawn a second instance of an application while the primary is still around. It is due to a g_application_impl_stop_primary call followed by g_application_impl_destroy. The exact call chain depends on whether the second instance had G_APPLICATION_IS_SERVICE set or not. See bug 783548 for an example. Every application has to deal with this oddity in one way or the other. Maybe we should avoid that? I am struggling to find a good way to justify the current behaviour in the documentation.
Created attachment 353421 [details] [review] GApplication: Don't call dbus_unregister multiple times Sorry for digressing from the original intent of the bug. On the other hand, it is somewhat related.
Created attachment 354044 [details] [review] GApplication: Assert that dbus_unregister was called before destruction
(In reply to Debarshi Ray from comment #8) > Secondly, do we expect people to guard against dbus_unregister being called > multiple times? When I added the dbus hooks (bug 675509), it was certainly my intention that they would be called only once.
Rishi, what was the outcome of your chat with Matthias about this?
(In reply to Philip Withnall from comment #12) > Rishi, what was the outcome of your chat with Matthias about this? I think he was in agreement, but he didn't formally codify it, so I might be mistaken.
Review of attachment 353421 [details] [review]: Looks good to me with that change ::: gio/gapplicationimpl-dbus.c @@ +458,3 @@ GApplicationClass *app_class = G_APPLICATION_GET_CLASS (impl->app); + if (!impl->unregistered) This looks double-negative to me. Should we have a registered boolean instead ?
Review of attachment 354044 [details] [review]: looks ok to me
Comment on attachment 354044 [details] [review] GApplication: Assert that dbus_unregister was called before destruction Pushed to master.
Created attachment 356058 [details] [review] GApplication: Don't call dbus_unregister multiple times unregistered -> registered
Review of attachment 354044 [details] [review]: ::: gio/gapplication.c @@ +1248,3 @@ + GApplication *application = G_APPLICATION (object); + + g_assert_null (application->priv->impl); We need to express this differently because it doesn't work for those (eg., evolution --quit) that use GApplication but not g_application_run. Thanks to Hussam for catching this.
Created attachment 356117 [details] [review] GApplication: Use a WARNING if dbus_unregister is called by destructor Here's another way to express it.
Review of attachment 356117 [details] [review]: ok
Review of attachment 356058 [details] [review]: ok
Pushed to master. Thanks for the reviews!
The warning brings "regression" [1] (well, it relaxes a crash, but otherwise is useless) and the commit message is inaccurate, missing the case when the application doesn't run, because it's the second running instance of it. There's no way to call g_application_run() when the application wants to quit and pass the "action" to an already running process (like described in [1]). [1] https://gitlab.gnome.org/GNOME/glib/issues/1857