Merge lp:~ev/compiz/call-apport-on-hangs into lp:compiz/0.9.8

[Evan]

This branch adds a USE_APPORT flag. If set, when faced with a hanging application, compiz will call apport with the --hanging option (in apport trunk; I'll be releasing shortly), rather than show its own dialog. It will also automatically show the dialog if the fade plugin triggers.

By calling into apport in this way, we can track application hangs as errors on http://errors.ubuntu.com

I'll happily take care of coordinating the upload to Ubuntu, if the branch is accepted and no one else beats me to it. :)

[lp:~ev/compiz/call-apport-on-hangs]

3274. By Evan on 2012-07-04

Report the error if we cannot spawn apport-gtk.

[Sam Spilsbury (smspillaz)]

this looks okay, although the design feels a bit weird to me.

145 +#ifdef USE_APPORT
146 + Time serverTime;
147 +
148 + if (!window->alive () && !forceQuitTriggered) {
149 + serverTime = screen->getCurrentTime ();
150 + screen->toolkitAction (Atoms::toolkitActionForceQuitDialog, serverTime, window->id (), true, 0, 0);
151 + forceQuitTriggered = true;
152 + }
153 +#endif
154 + if (window->alive ())
155 + forceQuitTriggered = false;
156
157 value = fScreen->optionGetUnresponsiveBrightness ();
158 if (value != 100)
159 @@ -361,7 +375,8 @@
160 fadeTime (0),
161 opacityDiff (0),
162 brightnessDiff (0),
163 - saturationDiff (0)
164 + saturationDiff (0),
165 + forceQuitTriggered (false)

Currently it pops up the apport dialog as soon as the application becomes unresponsive. There are quite a few cases where applications can become temporarily unresponsive and it isn't a bug because the system is just under really heavy load and the UI thread of every application is starved. It doesn't make sense to report those.

I noticed that because of this, you need to have checks in both gtk-window-decorator and in the fade plugin to see if the apport dialog has already come up.

What I'd suggest is just showing the dialog when the force quit dialog normally comes up - except it might be better to add a checkbox which says "report this unresponsive application"

Then you can make the code slightly more modular too by delegating the call through a standard interface that all reporters might accept.

This way you only have check if the apport dialog has appeared once, because the user will be closing and reporting the hang at the same time.

Also, some indentation notes:

72 + app = wnck_window_get_application (win);
73 + if (!app)
74 + return FALSE;

You need a one-tab indent for the return FALSE;

76 + pid = wnck_application_get_pid (app);
77 + if (d->apport_dialog) {
78 + /* Is Apport already running? */
79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
80 + return TRUE;
81 + }

pid is indented incorrectly, the brace should fall on the next line, return TRUE needs an indent (4 spaces)

85 + if (error) {
86 + g_printerr ("Could not spawn apport-gtk: %s\n", error->message);
87 + g_error_free (error);
88 + }

brace needs to fall on the next line.

[Daniel van Vugt (vanvugt)]

As above, please fix the formatting. Please also rethink as Sam says:

"Currently it pops up the apport dialog as soon as the application becomes unresponsive. There are quite a few cases where applications can become temporarily unresponsive and it isn't a bug because the system is just under really heavy load and the UI thread of every application is starved. It doesn't make sense to report those."

review: Needs Fixing

[Evan (ev)]

Matthew Paul Thomas is working on addressing this in the UI. I'll update the merge when we have something concrete.

[jenkins (martin-mrazik+qa)]

FAILED: Continuous integration, rev:3274
http://jenkins.qa.ubuntu.com/job/compiz-ci/2/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=amd64/2/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-ci/./distribution=quantal,flavor=i386/2/console

review: Needs Fixing (continuous-integration)

[Martin Mrazik (mrazik)]

The jenkins failure above seems to be be related to us.archive.ubuntu.com connectivity issues :-/

[Evan (ev)]

On Wed, Jul 4, 2012 at 8:41 PM, Sam Spilsbury
<email address hidden> wrote:
> Also, some indentation notes:
>
> 72 + app = wnck_window_get_application (win);
> 73 + if (!app)
> 74 + return FALSE;
>
> You need a one-tab indent for the return FALSE;
>
> 76 + pid = wnck_application_get_pid (app);
> 77 + if (d->apport_dialog) {
> 78 + /* Is Apport already running? */
> 79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
> 80 + return TRUE;
> 81 + }
>
> pid is indented incorrectly, the brace should fall on the next line, return TRUE needs an indent (4 spaces)

Does compiz mix tabs and spaces? If so, why?

Also, is there a codified coding style to be followed?

[Sam Spilsbury (smspillaz)]

> On Wed, Jul 4, 2012 at 8:41 PM, Sam Spilsbury
> <email address hidden> wrote:
> > Also, some indentation notes:
> >
> > 72 + app = wnck_window_get_application (win);
> > 73 + if (!app)
> > 74 + return FALSE;
> >
> > You need a one-tab indent for the return FALSE;
> >
> > 76 + pid = wnck_application_get_pid (app);
> > 77 + if (d->apport_dialog) {
> > 78 + /* Is Apport already running? */
> > 79 + if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
> > 80 + return TRUE;
> > 81 + }
> >
> > pid is indented incorrectly, the brace should fall on the next line, return
> TRUE needs an indent (4 spaces)
>
> Does compiz mix tabs and spaces? If so, why?

Yes, history (its how X11 does it, davidr was an X11 developer, that coding style got followed in compiz). Yes, its stupid. No we're not changing it (mucks up bzr histories etc, better to just be consistent).

>
> Also, is there a codified coding style to be followed?

I would point you to the compiz wiki, but it seems to be down again :(

[Evan (ev)]

On Fri, Jul 13, 2012 at 9:59 AM, Sam Spilsbury
<email address hidden> wrote:
>> Does compiz mix tabs and spaces? If so, why?
>
> Yes, history (its how X11 does it, davidr was an X11 developer, that coding style got followed in compiz). Yes, its stupid. No we're not changing it (mucks up bzr histories etc, better to just be consistent).

I'm sure "lets change it!" is the normal response, but I'm all for
staying consistent.
>>
>> Also, is there a codified coding style to be followed?
>
> I would point you to the compiz wiki, but it seems to be down again :(

I've fetched it out of Google's cache. Thanks!

[Evan (ev)]

Matthew has finished updating the specification to account for temporarily unresponsive applications:

https://wiki.ubuntu.com/ErrorTracker#app-hang

I'll get started on this now, but if there are concerns do let me know.

[Tim Penhey (thumper)]

Evan, I agree with you on the "change it" for mixed tabs and spaces (my emacs hates it). Personally I think history for that sake is more of a hindrance, and not something that couldn't be worked around when necessary, but hey, it isn't my project.

[Francis Ginther (fginther)]

Review was claimed by accident, please ignore.

review: Abstain

[Daniel van Vugt (vanvugt)]

The branches have changed. Please resubmit targeting lp:compiz

[Daniel van Vugt (vanvugt)]

review: Needs Resubmitting

[Sam Spilsbury (smspillaz)]

Is this still being worked on or can we drop it from the merge queue?

review: Needs Information

[Evan (ev)]

It's being worked on - slowly :)

Unmerged revisions

3274. By Evan on 2012-07-04

Report the error if we cannot spawn apport-gtk.

3273. By Evan on 2012-07-04

Put USE_APPORT in the right place so that forcequit.c picks it up.

3272. By Evan on 2012-07-04

Make apport dialog optional.

3271. By Evan on 2012-07-03

Initial commit of calling apport when applications hang under compiz.

=== modified file 'cmake/base.cmake'
--- cmake/base.cmake	2012-05-24 00:55:17 +0000
+++ cmake/base.cmake	2012-07-04 17:10:24 +0000
@@ -1,3 +1,5 @@
+option (BUILD_APPORT "Use apport instead of the built-in application hang dialog" ON)
+compiz_set (USE_APPORT ${BUILD_APPORT})
 set (USE_GCONF 1 CACHE BOOL "Install core GConf schemas")
 
 if (USE_GCONF)
@@ -29,6 +31,7 @@
     compiz_print_result_message ("gconf schemas" USE_GCONF)
     compiz_print_result_message ("gnome" USE_GNOME)
     compiz_print_result_message ("kde4 window decorator" USE_KDE4)
+    compiz_print_result_message ("apport hang dialog" USE_APPORT)
 
     compiz_print_result_message ("protocol buffers" USE_PROTOBUF)
     compiz_print_result_message ("file system change notifications" HAVE_INOTIFY)
 
=== modified file 'config.h.core.in'
--- config.h.core.in	2012-05-16 17:40:13 +0000
+++ config.h.core.in	2012-07-04 17:10:24 +0000
@@ -3,3 +3,6 @@
 #define PACKAGE "@_compiz_package@"
 
 #define GETTEXT_PACKAGE "@GETTEXT_PACKAGE@"
+
+/* Define to 1 if Apport support is enabled */
+#cmakedefine USE_APPORT 1
 
=== modified file 'gtk/config.h.gtk.in'
--- gtk/config.h.gtk.in	2008-10-14 10:27:55 +0000
+++ gtk/config.h.gtk.in	2012-07-04 17:10:24 +0000
@@ -4,6 +4,9 @@
 /* Define to 1 if Gconf support is enabled */
 #cmakedefine USE_GCONF 1
 
+/* Define to 1 if Apport support is enabled */
+#cmakedefine USE_APPORT 1
+
 /* Define to 1 if you have the `wnck_window_has_name' function. */
 #cmakedefine HAVE_WNCK_WINDOW_HAS_NAME 1
 
 
=== modified file 'gtk/window-decorator/forcequit.c'
--- gtk/window-decorator/forcequit.c	2011-02-21 09:53:08 +0000
+++ gtk/window-decorator/forcequit.c	2012-07-04 17:10:24 +0000
@@ -23,6 +23,9 @@
  *        Authored By: Sam Spilsbury <[e-mail]>
  */
 
+#ifdef HAVE_CONFIG_H
+#include  <config.h>
+#endif
 #include "gtk-window-decorator.h"
 
 static char *
@@ -127,6 +130,37 @@
     }
 }
 
+gboolean
+show_apport_dialog (WnckWindow *win)
+{
+    WnckApplication *app;
+    GError *error = NULL;
+    gulong flags = G_SPAWN_SEARCH_PATH | G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
+    decor_t *d = g_object_get_data (G_OBJECT (win), "decor");
+    int pid;
+    char pid_s[16];
+    gchar *argv[] = { "/usr/share/apport/apport-gtk", "--hanging", NULL, NULL };
+
+    app = wnck_window_get_application (win);
+    if (!app)
+    return FALSE;
+
+	pid = wnck_application_get_pid (app);
+    if (d->apport_dialog) {
+        /* Is Apport already running? */
+        if (kill (d->apport_dialog, 0) >= 0 && errno != ESRCH)
+        return TRUE;
+    }
+    sprintf (pid_s, "%d", pid);
+    argv[2] = pid_s;
+    g_spawn_async (NULL, argv, NULL, flags, NULL, NULL, &(d->apport_dialog), &error);
+    if (error) {
+        g_printerr ("Could not spawn apport-gtk: %s\n", error->message);
+        g_error_free (error);
+    }
+    return TRUE;
+}
+
 void
 show_force_quit_dialog (WnckWindow *win,
 			Time        timestamp)
@@ -135,6 +169,11 @@
     GtkWidget *dialog;
     gchar     *str, *tmp;
 
+#ifdef USE_APPORT
+    if (show_apport_dialog(win))
+    return;
+#endif
+
     if (d->force_quit_dialog)
 	return;
 
 
=== modified file 'gtk/window-decorator/gtk-window-decorator.h'
--- gtk/window-decorator/gtk-window-decorator.h	2012-05-27 04:32:55 +0000
+++ gtk/window-decorator/gtk-window-decorator.h	2012-07-04 17:10:24 +0000
@@ -100,6 +100,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <signal.h>
+#include <errno.h>
 
 #include <libintl.h>
 #define _(x)  gettext (x)
@@ -464,6 +465,7 @@
     WnckWindowActions actions;
     XID		      prop_xid;
     GtkWidget	      *force_quit_dialog;
+    int		 apport_dialog;
     Bool	      created;
     void	      (*draw) (struct _decor *d);
 } decor_t;
 
=== modified file 'plugins/fade/src/fade.cpp'
--- plugins/fade/src/fade.cpp	2011-06-27 13:32:58 +0000
+++ plugins/fade/src/fade.cpp	2012-07-04 17:10:24 +0000
@@ -23,6 +23,9 @@
  * Author: David Reveman <[e-mail]>
  */
 
+#ifdef HAVE_CONFIG_H
+#  include <config.h>
+#endif
 #include "fade.h"
 #include <core/atoms.h>
 
@@ -195,6 +198,17 @@
 	fScreen->optionGetDimUnresponsive ())
     {
 	GLuint value;
+#ifdef USE_APPORT
+    Time serverTime;
+
+    if (!window->alive () && !forceQuitTriggered) {
+    serverTime = screen->getCurrentTime ();
+    screen->toolkitAction (Atoms::toolkitActionForceQuitDialog, serverTime, window->id (), true, 0, 0);
+    forceQuitTriggered = true;
+    }
+#endif
+    if (window->alive ())
+    forceQuitTriggered = false;
 
 	value = fScreen->optionGetUnresponsiveBrightness ();
 	if (value != 100)
@@ -361,7 +375,8 @@
     fadeTime (0),
     opacityDiff (0),
     brightnessDiff (0),
-    saturationDiff (0)
+    saturationDiff (0),
+    forceQuitTriggered (false)
 {
     if (window->isViewable ())
 	addDisplayModal ();
 
=== modified file 'plugins/fade/src/fade.h'
--- plugins/fade/src/fade.h	2012-01-18 16:26:45 +0000
+++ plugins/fade/src/fade.h	2012-07-04 17:10:24 +0000
@@ -94,6 +94,8 @@
 	int opacityDiff;
 	int brightnessDiff;
 	int saturationDiff;
+
+    bool forceQuitTriggered;
 };
 
 class FadePluginVTable :