Comment on attachment 297463 [details]
The file bug.cpp, see comments inside on how to compile.

>// COMPILE AS:
>//
>// g++ -DWORKAROUND=0 `pkg-config --cflags gtkmm-3.0` `pkg-config --cflags gtk+-3.0` bug.cpp `pkg-config --libs gtk+-3.0` `pkg-config --libs gtkmm-3.0` -Wl,--export-dynamic
>//
>
>// FIX THIS PATH.
>char const* glade_path = "/home/carlo/projects/speech/speech/res/speechUI.glade";
>
>#ifndef WORKAROUND
>#error Use -DWORKAROUND=0 or -DWORKAROUND=1
>#endif
>
>#include <iostream>
>#include <gtkmm.h>
>#include <cassert>
>
>class UIWindow : public Gtk::Window
>{
>  private:
>    Glib::RefPtr<Gtk::Application> m_refApp;
>
>    static GtkWindow* read_glade(std::string const& glade_path, char const* window_name);
>
>  public:
>    UIWindow(Glib::RefPtr<Gtk::Application> const& refApp, std::string const& glade_path, char const* window_name);
>    virtual ~UIWindow();
>
>  protected:
>    // Signal handlers.
>    void on_hide(void);
>};
>
>//static
>GtkWindow* UIWindow::read_glade(std::string const& glade_path, char const* window_name)
>{
>  // This block is C code because we need a plain GtkWindow instead of a Gtk::Window.
>  // Otherwise we get a warning when wrapping the GtkWindow* in a Gtk::Window below,
>  // saying that is already wrapped.
>
>  GtkBuilder* builder = gtk_builder_new_from_file(glade_path.c_str());
>  GtkWindow* window = GTK_WINDOW(gtk_builder_get_object(builder, window_name));
>  // Do not connect signals C-style though, because we need them to be connected
>  // C++-style, which is done in the constructor of UIWindow.
>  g_object_unref(G_OBJECT(builder));	// Delete the builder.
>
>  if (!window) std::cerr << "The glade file does not contain a window called '" << window_name << "'." << std::endl;
>  assert(window);
>  return window;
>}
>
>UIWindow::UIWindow(Glib::RefPtr<Gtk::Application> const& refApp, std::string const& glade_path, char const* window_name) :
>    Gtk::Window(read_glade(glade_path, window_name)), m_refApp(refApp)
>{
>  // Do not leave application until the destructor was called.
>  refApp->hold();
>
>  // Connect signals.
>
>  // Call the on_hide() method when the UIWindow is hidden.
>  signal_hide().connect(sigc::mem_fun(this, &UIWindow::on_hide));
>}
>
>UIWindow::~UIWindow()
>{
>  std::cout << "UIWindow::~UIWindow()" << std::endl;
>  // Do not terminate the application immediately because otherwise
>  // the window is not removed from the screen and we get an annoying
>  // pop-up from kwin saying `You tried to close window "Speech recognition"
>  // from application "speech" (Process ID: 27656) but the application is not
>  // responding. Do you want to terminate this application?`.
>#if WORKAROUND
>  m_refApp->set_inactivity_timeout(10);
>#endif
>  m_refApp->release();
>}
>
>void UIWindow::on_hide(void)
>{
>  std::cout << "UIWindow::on_hide()" << std::endl;
>}
>
>int main (void)
>{
>  Glib::RefPtr<Gtk::Application> refApp = Gtk::Application::create("org.gnome.gtkbugexample");
>  // Store refApp in UIWindow in order to Keep the application running (not return from run()), until
>  // the window is destructed (as result of the hide signal; ie, when the user clicks the close button).
>  UIWindow* pUIWindow = new UIWindow(refApp, glade_path, "window1");
>
>  std::cout << "Entering run()" << std::endl;
>  refApp->run(*pUIWindow);
>  std::cout << "Returned from run()" << std::endl;
>
>  // Keep application running, do not actually leave main().
>  sleep(-1);
>  return 0;
>}

Created attachment 297498 [details] [review]
GApplication: let the main loop drain on shutdown

After ::shutdown, run the mainloop until all pending activity is
handled, before returning from run().

Among other things, this gives a chance for destroyed windows to be
properly withdrawn from the windowing system.

hi Carlo,

Thanks for the report.

The attached patch fixes the issue for me and it will be applied, pending review.

If you are able to, can you test a patched version of glib (with the patch above) to see if this addresses the issue as you've reported it?

Hi Ryan,

Thanks for rapid response.

Yes this fixes the issue completely for me.

Review of attachment 297498 [details] [review]:

Wow, I wonder why we didn't catch this earlier. Thanks!

Attachment 297498 [details] pushed as 2844f23 - GApplication: let the main loop drain on shutdown

This patch has one problem: now if you call g_application_quit() the application won't quit.

(In reply to Giovanni Campagna from comment #7)
> This patch has one problem: now if you call g_application_quit() the
> application won't quit.

I don't understand how that could be the case.

Can you attach a small demo program that demonstrates the issue?

Well, if you call g_application_quit() you will not destroy windows, so you will have events to process.

Arguably, because you don't block in iteration(NULL), you need a busy loop, most likely some kind of GL app that doesn't keep up with the display and countinously redraws, so a small demo program would be hard to produce.

That's true, but it would only happen in the case that the app is pinning the CPU at 100% all the time without a single break...

(In reply to Ryan Lortie (desrt) from comment #10)
> That's true, but it would only happen in the case that the app is pinning
> the CPU at 100% all the time without a single break...

Again, if it's a GL app, most of the time it would be blocked in eglSwapBuffers (while pinning the GPU at 100% and dropping frames, or just waiting for vblank if swapinterval is configured), even if the CPU usage is reasonable. It just doesn't block in the poll() from GMainContext.

It's probably possible to trigger this from clutter if you have an animation running (and at the same time use the proprietary drivers that block in eglSwapBuffers instead of giving a frame completion XEvent).

It might sound an edge case, but in practice running the main loop without the app asking for it is a bad idea.
See eg. bug 742065, when you get a deadlock if you g_main_context_iteration() without the property gtk+/clutter threading.

I'm not an expert of this code, but I'd suggest to only run this extra loop when we get here because the last window was closed - I think the only other reason to get here is by calling g_application_quit(), if that is done without first closing all windows (so that any deadlock might occur) then it is probably best to NOT run g_main_context_iteration(). So, I'd suggest detect (again) if all windows are closed and if so drain the mainloop, otherwise you could opt to run it 10 times? Not sure if I understand if these calls could possibly block (in which case a limit on the number of loops here would not help). In my case I saw it loop six times before it was done.

Created attachment 298083 [details] [review]
GApplication: don't iterate further on _quit()

If someone explicitly calls g_application_quit() then don't attempt to
drain the mainloop of remaining sources.

This allows applications with 100% CPU utilisation to quit reliably.

Giovanni: looks good?

Yes, this restores the documented behavior of "quit now" and looks good to me.

Attachment 298083 [details] pushed as 0cb75bf - GApplication: don't iterate further on _quit()