GNOME Bugzilla – Bug 658805
GtkApplication: Don't try to quit the main loop when none is running
Last modified: 2013-10-28 21:36:16 UTC
In krb5-auth-dialog I'm seeing this when exiting from a custom command line handler: (krb5-auth-dialog:25764): Gtk-CRITICAL **: gtk_main_quit: assertion `main_loops != NULL' failed The bt shows:
+ Trace 228422
This is because g_application_release was called without entering the main loop. GLib's GApplication guards this with a: if (application->priv->mainloop != NULL) g_main_loop_quit (application->priv->mainloop); we should do the same for GtkApplication in GTK. Patch attaced.
Created attachment 196245 [details] [review] GtkApplication: Don't try to quit the main loop when none is running
This seems vaguely like a GApplication bug. I'll look into it there.
As far as I understand things g_application_release can't be sure the main loop is running when the use_count drops to zero. Therefore g_application_real_quit_mainloop checks for that. Shouldn't GTK+ just do the same?
I think it was a mistake for GApplication to ever use GMainLoop or support virtual functions for doing so... Might make sense just to rewrite it in GApplication like while (held) g_main_context_iterate (); and avoid all of these issues.
Created attachment 196463 [details] [review] Don't implement GApplication mainloop Let the default GApplication implementation take care of it.
Created attachment 196464 [details] [review] GApplication: don't create a mainloop Just iterate the main context directly. Also: avoid calling the virtual functions for mainloops.
I found the main loop handling actually a nice feature of G(tk)Application. Wouldn't ripping that out change ABI?
This is not about ripping out the support -- it's merely making it so that application implementations (of which Gtk is the only one?) are incapable of overriding the default behaviour. The normal hold/release semantics (including each GtkWindow causing a hold) will be kept.
Comment on attachment 196464 [details] [review] GApplication: don't create a mainloop Committed the GLib changes, but we should not commit the Gtk changes yet since Gtk has not branched.
(In reply to comment #8) > This is not about ripping out the support -- it's merely making it so that > application implementations (of which Gtk is the only one?) are incapable of > overriding the default behaviour. > > The normal hold/release semantics (including each GtkWindow causing a hold) > will be kept. Rereading the patch this makes sense. Thanks!
With the GLib commit 43007, Empathy cannot quit anymore. Empathy uses g_application_run() with a GtkApplication child class and quits with gtk_main_quit(). With the GLib commit 43007, g_application_run() no longer runs gtk_main() through the virtual method ->run_mainloop() and gtk_main_quit() fails and print the message: (empathy:5154): Gtk-CRITICAL **: gtk_main_quit: assertion `main_loops != NULL' failed Because gtk does not have a reference to the mainloops anymore so gtk_main_quit() is broken. I could use g_application_release (my_app) in Empathy and it works. But how could gtk_main_quit() be implemented without a reference on the mainloop?
(In reply to comment #11) > With the GLib commit 43007, Empathy cannot quit anymore. Empathy uses > g_application_run() with a GtkApplication child class and quits with > gtk_main_quit(). With the GLib commit 43007, g_application_run() no longer runs > gtk_main() through the virtual method ->run_mainloop() and gtk_main_quit() > fails and print the message: This seems to also break Eye of GNOME (bug 662630) which relies on the GdkThreads locking order as shown in the first example here: http://developer.gnome.org/gdk3/stable/gdk3-Threads.html#gdk3-Threads.description If you call gdk_threads_enter() before entering the mainloop as shown you'll get a deadlock once the application runs (as the locks aren't released by gtk_main() anymore). While it might be possible that this isn't necessary anymore it breaks backwards compatibility.
I believe this change affects each and every application using GtkApplication. Not a good change, from my point of view. And yes, evolution is in the group too (bug #663122). This transfers from "things being done transparently" to "do it yourself" stuff in each application, which doesn't make sense. Furthermore, after your change in GLib the GApplicationClass::quit_mainloop and the GApplicationClass::run_mainloop are not used at all and should be removed, which will clearly indicate that the old behaviour is dropped and the application should adapt to new behaviour, which is not clear to me what is, because how can I run the gtk's main loop when there is no API for it? I mean when using g_application_run(). This seems like a very nice step to drop the GApplication, or am I the only one seeing the issue here? Before the change one was able to run his/her own mainloop, but now the application runs completely without it, and only on the default MainContext (again, with g_application_run). Is it intended? What is it good for? On the other hand, maybe I'm just missing something obvious here... Anyway, evolution luckily doesn't use g_application_run(), thus I can workaround it there (I would not call that a fix myself).
(In reply to comment #13) > I believe this change affects each and every application using GtkApplication. > Not a good change, from my point of view. And yes, evolution is in the group > too (bug #663122). This transfers from "things being done transparently" to "do > it yourself" stuff in each application, which doesn't make sense. What this change broke is the (widespread) assumption that gtk_main_quit is a universal way to stop any GTK+ application. Which was not the case before either... but I agree that this breakage is somewhat unfortunate, in particular because there is no 100% straightforward way to quit a GtkApplication. > Furthermore, after your change in GLib the GApplicationClass::quit_mainloop and > the GApplicationClass::run_mainloop are not used at all and should be removed, We need to update the documentation for these now-unused vfuncs, certainly. bug 665391 > Before the change one was able to run his/her own mainloop, but now the > application runs completely without it, and only on the default MainContext > (again, with g_application_run). Is it intended? What is it good for? Not sure what you complain about here. You can still forego g_application_run and run your own mainloop - which is what evo does anyway, as you say further down...
Indeed -- it was never intended that you should be able to quit GApplication by using gtk_main_quit().
Since we're already breaking GApplication semantics here, could we at least have it emit the "shutdown" signal from g_application_release() instead of g_application_run() when the use count drops to zero? Then applications that forego g_application_run() but still rely on the GApplication's use count (such as Evolution) could still be notified WHEN to shutdown, and they can call gtk_main_quit() themselves from their own shutdown() method if need be.
That seems rather dangerous. The application doesn't shutdown at the point that the last hold is released -- it shuts down after it returns to the mainloop after that happens. Would this be solved if GApplication had some sort of a _quit() function?
Okay, emit it from an idle callback then? The point is the way it works now makes the use count useless unless g_application_run() is called, and using g_application_run() is supposedly optional (according to comment #14).
Created attachment 202626 [details] [review] GApplication: Always emit shutdown This is what I'd like. ALWAYS emit "shutdown" when the use count hits zero, not just when using g_application_run(). As suggested the emission takes place after control returns to the main loop. Since GApplication emits a critical warning if "did_shutdown" is not set, I thought it reasonable to rely on it in g_application_run().
Created attachment 202628 [details] [review] GApplication: Always emit shutdown (revised) Some refinements and simplifications after realizing I was handling the remote case wrong.
Grr... typo: in emit_shutdown() I meant to set inactivity_timeout_id to 0.
(In reply to comment #14) > > Before the change one was able to run his/her own mainloop, but now the > > application runs completely without it, and only on the default MainContext > > (again, with g_application_run). Is it intended? What is it good for? > > Not sure what you complain about here. You can still forego g_application_run > and run your own mainloop - which is what evo does anyway, as you say further > down... I was able to manage my own main loop when using those to-be-removed vfuncs, but now I'm not. What I expect from the GApplication descendant is that I can easily use it, and I'm still able to provide my own main loop, if it's intended, which I suppose it is. If I do not provide it, then I expect it'll basically use some default, or create one itself, as I do not see any default main-loop API (in the stable doc). After the change in GLib, if I would like to do something like "Example 15. Opening files with a GApplication" from the GApplication documentation, then the g_application_run() will not create any GMainLoop, it will only iterate on the default context. If there is no difference, and no bad impact on gtk applications, then the removal of a possibility of providing my own context and/or main loop when using g_application_run() is probably OK. By the way, I never meant to say I'm expecting quitting G(tk)Application with gtk_main_quit(), it's just that it's missing there right now (bug #663122).
Moving this to glib, since this is a gapplication patch
I don't really like this approach. I think there's a bug in your patch as well -- if g_application_release() is immediately followed by a g_application_hold() then the idle will still run, causing the shutdown to happen. The trickiness of these issues is exactly what prompted this change in the first place. "while (use_count)" is a really really easy loop to write (plus a little extra for the inactivity timeout, of course). Is there a specific reason that you're trying to avoid g_application_run() in the first place but still want to use GApplication's lifecycle management? The two really go hand-in-hand in my opinion... Is it because you're trying to multiplex access to gtk_main_quit() -- either manual or GApplication-caused?
Is g_application_run() mandatory when using G[tk]Application or not? If so the API docs really need to be clarified. You and Matthias seem to have opposing answers to this, and I've always understood it to be a convenience function, mostly to aid in GApplication's way of handling command-line options. GOptionContext works fine for us, and GApplication still lacks any integration with it, so I didn't see any compelling reason to use g_application_run() over gtk_main(). I still don't, to be honest.
g_application_run() is expected when using GApplication. All of the documentation shows that. If there is some specific missing functionality that is causing problems, please open a new bug about this. This bug is getting long and has been repurposed one time too many, so I'm going to close it now.