Merge lp:~charlesk/indicator-session/lp-1256590-j-random-user into lp:indicator-session/14.04

[Charles Kerr]

Fix the phantom "J Random User" menuitem that shows up under some conditions.

This patch skips creating menuitems for users that don't have a username.

Sometimes org.freedesktop.login1 gives us a user without a username. Multiple users have reported this happens when running "Remmina Remote Desktop Client."

So we pass a NULL label to glib when we create the menuitem, and when the label property is NULL for an x-canonical-type of "indicator.user-menu-item", the default text of "J Random User" is used.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:444
http://jenkins.qa.ubuntu.com/job/indicator-session-ci/127/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-amd64-ci/23
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-armhf-ci/23
        deb: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-armhf-ci/23/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-session-ci/127/rebuild

review: Approve (continuous-integration)

[lp:~charlesk/indicator-session/lp-1256590-j-random-user]

445. By Charles Kerr on 2014-04-02

copy recoverable-problem.[ch] from url-dispatcher 0.1+14.04.20140331.1-0ubuntu1

446. By Charles Kerr on 2014-04-02

if we encounter a user for whom we can't find a name, report it to apport as a recoverable error.

[Ted Gould (ted)]

+1

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:446
http://jenkins.qa.ubuntu.com/job/indicator-session-ci/128/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-amd64-ci/24
    SUCCESS: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-armhf-ci/24
        deb: http://jenkins.qa.ubuntu.com/job/indicator-session-trusty-armhf-ci/24/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/indicator-session-ci/128/rebuild

review: Approve (continuous-integration)

=== modified file 'debian/control'
--- debian/control	2014-03-05 20:12:45 +0000
+++ debian/control	2014-04-02 15:03:28 +0000
@@ -32,6 +32,7 @@
             unity-control-center | gnome-control-center,
             unity-control-center-signon | gnome-control-center-signon
 Suggests: lightdm,
+          apport,
           zenity
 Description: indicator showing session management, status and user switching
  This indicator is designed to be placed on the right side of a panel and
 
=== modified file 'src/CMakeLists.txt'
--- src/CMakeLists.txt	2013-07-02 02:00:17 +0000
+++ src/CMakeLists.txt	2014-04-02 15:03:28 +0000
@@ -5,6 +5,8 @@
              actions.h
              guest.c
              guest.h
+             recoverable-problem.c
+             recoverable-problem.h
              service.c
              service.h
              users.c
 
=== added file 'src/recoverable-problem.c'
--- src/recoverable-problem.c	1970-01-01 00:00:00 +0000
+++ src/recoverable-problem.c	2014-04-02 15:03:28 +0000
@@ -0,0 +1,167 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+#include "recoverable-problem.h"
+#include <glib/gstdio.h>
+#include <string.h>
+#include <errno.h>
+
+/* Helpers to ensure we write nicely */
+static void 
+write_string (int          fd,
+              const gchar *string)
+{
+	int res; 
+	do
+		res = write (fd, string, strlen (string));
+	while (G_UNLIKELY (res == -1 && errno == EINTR));
+}
+
+/* Make NULLs fast and fun! */
+static void 
+write_null (int fd)
+{
+	int res; 
+	do
+		res = write (fd, "", 1);
+	while (G_UNLIKELY (res == -1 && errno == EINTR));
+}
+
+/* Child watcher */
+static gboolean
+apport_child_watch (GPid pid G_GNUC_UNUSED, gint status G_GNUC_UNUSED, gpointer user_data)
+{
+	g_main_loop_quit((GMainLoop *)user_data);
+	return FALSE;
+}
+
+static gboolean
+apport_child_timeout (gpointer user_data)
+{
+	g_warning("Recoverable Error Reporter Timeout");
+	g_main_loop_quit((GMainLoop *)user_data);
+	return FALSE;
+}
+
+/* Code to report an error */
+void
+report_recoverable_problem (const gchar * signature, GPid report_pid, gboolean wait, gchar * additional_properties[])
+{
+	GSpawnFlags flags;
+	gboolean first;
+	GError * error = NULL;
+	gint error_stdin = 0;
+	GPid pid = 0;
+	gchar * pid_str = NULL;
+	gchar ** argv = NULL;
+	gchar * argv_nopid[2] = {
+		"/usr/share/apport/recoverable_problem",
+		NULL
+	};
+	gchar * argv_pid[4] = {
+		"/usr/share/apport/recoverable_problem",
+		"-p",
+		NULL, /* put pid_str when allocated here */
+		NULL
+	};
+
+
+	argv = (gchar **)argv_nopid;
+
+	if (report_pid != 0) {
+		pid_str = g_strdup_printf("%d", report_pid);
+		argv_pid[2] = pid_str;
+		argv = (gchar**)argv_pid;
+	}
+
+	flags = G_SPAWN_STDOUT_TO_DEV_NULL | G_SPAWN_STDERR_TO_DEV_NULL;
+	if (wait) {
+		flags |= G_SPAWN_DO_NOT_REAP_CHILD;
+	}
+
+	g_spawn_async_with_pipes(NULL, /* cwd */
+		argv,
+		NULL, /* envp */
+		flags,
+		NULL, NULL, /* child setup func */
+		&pid,
+		&error_stdin,
+		NULL, /* stdout */
+		NULL, /* stderr */
+		&error);
+
+	if (error != NULL) {
+		g_warning("Unable to report a recoverable error: %s", error->message);
+		g_error_free(error);
+	}
+
+	first = TRUE;
+
+	if (error_stdin != 0 && signature != NULL) {
+		write_string(error_stdin, "DuplicateSignature");
+		write_null(error_stdin);
+		write_string(error_stdin, signature);
+
+		first = FALSE;
+	}
+
+	if (error_stdin != 0 && additional_properties != NULL) {
+		gint i;
+		for (i = 0; additional_properties[i] != NULL; i++) {
+			if (!first) {
+				write_null(error_stdin);
+			} else {
+				first = FALSE;
+			}
+
+			write_string(error_stdin, additional_properties[i]);
+		}
+	}
+
+	if (error_stdin != 0) {
+		close(error_stdin);
+	}
+
+	if (wait && pid != 0) {
+		GSource * child_source, * timeout_source;
+		GMainContext * context = g_main_context_new();
+		GMainLoop * loop = g_main_loop_new(context, FALSE);
+
+		child_source = g_child_watch_source_new(pid);
+		g_source_attach(child_source, context);
+		g_source_set_callback(child_source, (GSourceFunc)apport_child_watch, loop, NULL);
+
+		timeout_source = g_timeout_source_new_seconds(5);
+		g_source_attach(timeout_source, context);
+		g_source_set_callback(timeout_source, apport_child_timeout, loop, NULL);
+
+		g_main_loop_run(loop);
+
+		g_source_destroy(timeout_source);
+		g_source_destroy(child_source);
+		g_main_loop_unref(loop);
+		g_main_context_unref(context);
+
+		g_spawn_close_pid(pid);
+	}
+
+	g_free(pid_str);
+
+	return;
+}
 
=== added file 'src/recoverable-problem.h'
--- src/recoverable-problem.h	1970-01-01 00:00:00 +0000
+++ src/recoverable-problem.h	2014-04-02 15:03:28 +0000
@@ -0,0 +1,26 @@
+/*
+ * Copyright 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authors:
+ *     Ted Gould <[e-mail]>
+ */
+
+#include <gio/gio.h>
+
+void    report_recoverable_problem    (const gchar *   signature,
+                                       GPid            report_pid,
+                                       gboolean        wait,
+                                       gchar *         additional_properties[]);
+
 
=== modified file 'src/service.c'
--- src/service.c	2014-03-24 10:19:00 +0000
+++ src/service.c	2014-04-02 15:03:28 +0000
@@ -21,6 +21,7 @@
 #include <gio/gio.h>
 
 #include "backend.h"
+#include "recoverable-problem.h"
 #include "service.h"
 
 #define BUS_NAME "com.canonical.indicator.session"
@@ -104,6 +105,7 @@
   GSimpleAction * user_switcher_action;
   GSimpleAction * guest_switcher_action;
   GHashTable * users;
+  GHashTable * reported_users;
   guint rebuild_id;
   int rebuild_flags;
   GDBusConnection * conn;
@@ -250,15 +252,15 @@
 static void
 maybe_add_users (IndicatorSessionService * self)
 {
-  if (!show_user_list (self))
-      return;
-
-  GList * uids, * l;
-
-  uids = indicator_session_users_get_uids (self->priv->backend_users);
-  for (l=uids; l!=NULL; l=l->next)
-    add_user (self, GPOINTER_TO_UINT(l->data));
-  g_list_free (uids);
+  if (show_user_list (self))
+    {
+      GList * uids, * l;
+
+      uids = indicator_session_users_get_uids (self->priv->backend_users);
+      for (l=uids; l!=NULL; l=l->next)
+        add_user (self, GPOINTER_TO_UINT(l->data));
+      g_list_free (uids);
+    }
 }
 
 
@@ -488,6 +490,49 @@
   return serialized_icon;
 }
 
+static void
+report_unusable_user (IndicatorSessionService * self, const IndicatorSessionUser * u)
+{
+  const priv_t * const p = self->priv;
+  gpointer key;
+
+  g_return_if_fail(u != NULL);
+
+  key = GUINT_TO_POINTER(u->uid);
+
+  if (!g_hash_table_contains (p->reported_users, key))
+  {
+    gchar * uid_str;
+    GPtrArray * additional;
+    const gchar * const error_name = "indicator-session-unknown-user-error";
+
+    /* don't spam apport with duplicates */
+    g_hash_table_add (p->reported_users, key);
+
+    uid_str = g_strdup_printf("%u", u->uid);
+
+    additional = g_ptr_array_new (); /* null-terminated key/value pair strs */
+    g_ptr_array_add (additional, "uid");
+    g_ptr_array_add (additional, uid_str);
+    g_ptr_array_add (additional, "icon_file");
+    g_ptr_array_add (additional, u->icon_file ? u->icon_file : "(null)");
+    g_ptr_array_add (additional, "is_current_user");
+    g_ptr_array_add (additional, u->is_current_user ? "true" : "false");
+    g_ptr_array_add (additional, "is_logged_in");
+    g_ptr_array_add (additional, u->is_logged_in ? "true" : "false");
+    g_ptr_array_add (additional, "real_name");
+    g_ptr_array_add (additional, u->real_name ? u->real_name : "(null)");
+    g_ptr_array_add (additional, "user_name");
+    g_ptr_array_add (additional, u->user_name ? u->user_name : "(null)");
+    g_ptr_array_add (additional, NULL); /* null termination */
+    report_recoverable_problem(error_name, (GPid)0, FALSE, (gchar**)additional->pdata);
+
+    /* cleanup */
+    g_free (uid_str);
+    g_ptr_array_free (additional, TRUE);
+  }
+}
+
 static GMenuModel *
 create_switch_section (IndicatorSessionService * self, int profile)
 {
@@ -576,12 +621,23 @@
   for (i=0; i<users->len; ++i)
     {
       const IndicatorSessionUser * u = g_ptr_array_index (users, i);
+      const char * label;
       GVariant * serialized_icon;
 
       if (profile == PROFILE_LOCKSCREEN && u->is_current_user)
         continue;
 
-      item = g_menu_item_new (get_user_label (u), NULL);
+      /* Sometimes we get a user without a username? bus hiccup.
+         I can't reproduce it, but let's not confuse users with
+         a meaningless menuitem. (see bug #1263228) */
+      label = get_user_label (u);
+      if (!label || !*label)
+      {
+        report_unusable_user (self, u);
+        continue;
+      }
+
+      item = g_menu_item_new (label, NULL);
       g_menu_item_set_action_and_target (item, "indicator.switch-to-user", "s", u->user_name);
       g_menu_item_set_attribute (item, "x-canonical-type", "s", "indicator.user-menu-item");
 
@@ -1095,6 +1151,9 @@
                                     g_direct_equal,
                                     NULL,
                                     (GDestroyNotify)indicator_session_user_free);
+
+  p->reported_users = g_hash_table_new (g_direct_hash, g_direct_equal);
+
   maybe_add_users (self);
 
   init_gactions (self);
@@ -1235,6 +1294,7 @@
     }
 
   g_clear_pointer (&p->users, g_hash_table_destroy);
+  g_clear_pointer (&p->reported_users, g_hash_table_destroy);
   g_clear_object (&p->backend_users);
   g_clear_object (&p->backend_guest);
   g_clear_object (&p->backend_actions);