Merge lp:~ted/ubuntu-app-launch/lp1495871-unref-context into lp:ubuntu-app-launch/16.04

[Ted Gould]

Ensure all pending events on the context are complete before unref'ing it.

So while we create a context to ensure that we can capture all the events to the CGManager connection it seems that some of those events can get stuck on the context, and they need to get processed to complete. Otherwise they hold onto references of things, and that actually results in a g_spawn getting left around because it never gets free'd. (actually of the context itself, which is a bit confusing, but eh, refcounting is fun)

Also brought in the pid tools branch because it made testing this much, much easier. You can use the ubuntu-app-list-pids tool to cause the FD to be leaked and then see it fixed with something like (assuming you've done ubuntu-app-launch firefox):

valgrind --track-fds=yes ./ubuntu-app-list-pids firefox

Which on trunk will have 6 FDs left over, but with this branch only 5. (which come from globals that we don't control)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:214
http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-ci/18/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-amd64-ci/18
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-armhf-ci/18
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-i386-ci/18

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-app-launch-ci/18/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:214
http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-ci/22/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-amd64-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-armhf-ci/22/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-i386-ci/22/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-app-launch-ci/22/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:215
http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-ci/27/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-amd64-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-armhf-ci/27
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-app-launch-wily-i386-ci/27

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-app-launch-ci/27/rebuild

review: Approve (continuous-integration)

[Charles Kerr (charlesk)]

The code looks fine to me.

IIUC, the while(pending){iterate();} snippet is the main fix, while the rest of it is either sugar (eg qdata instead of data) or the pid tools branch, right?

Optional, I'd suggest extracting while(pending){iterate();} into an inline function to improve the readability a bit, eg flush_pending_events(GMainContext*). IMO this would also address thomas' comment on the readability of the /* may block */ comment

review: Approve

=== modified file 'debian/libubuntu-app-launch2.symbols'
--- debian/libubuntu-app-launch2.symbols	2015-08-17 21:38:34 +0000
+++ debian/libubuntu-app-launch2.symbols	2015-12-07 17:03:12 +0000
@@ -3,6 +3,7 @@
  ubuntu_app_launch_application_info@Base 0.5+15.10.20150817
  ubuntu_app_launch_application_log_path@Base 0.4
  ubuntu_app_launch_get_primary_pid@Base 0.4
+ ubuntu_app_launch_get_pids@Base 0replaceme
  ubuntu_app_launch_helper_set_exec@Base 0.5+15.10.20150604
  ubuntu_app_launch_list_helper_instances@Base 0.4
  ubuntu_app_launch_list_helpers@Base 0.4

=== modified file 'helpers-shared.c'
--- helpers-shared.c	2015-07-15 02:32:54 +0000
+++ helpers-shared.c	2015-12-07 17:03:12 +0000
@@ -145,6 +145,8 @@
 	cgroup_manager_connection_core_cb(g_dbus_connection_new_for_address_finish, res, (cgm_connection_t *)data);
 }
 
+G_DEFINE_QUARK(CGMANAGER_CONTEXT, cgmanager_context);
+
 /* Get the connection to the cgroup manager */
 GDBusConnection *
 cgroup_manager_connection (void)
@@ -191,7 +193,9 @@
 	g_main_context_pop_thread_default(context);
 
 	if (!use_session_bus && connection.con != NULL) {
-		g_object_set_data(G_OBJECT(connection.con), "cgmanager-context", context);
+		g_object_set_qdata(G_OBJECT(connection.con),
+		                   cgmanager_context_quark(),
+		                   context);
 	} else {
 		g_main_context_unref(context);
 	}
@@ -213,7 +217,7 @@
 	if (cgmanager == NULL)
 		return;
 
-	GMainContext * creationcontext = g_object_get_data(G_OBJECT(cgmanager), "cgmanager-context");
+	GMainContext * creationcontext = g_object_get_qdata(G_OBJECT(cgmanager), cgmanager_context_quark());
 	if (creationcontext == NULL) {
 		g_object_unref(cgmanager);
 		return;
@@ -228,6 +232,11 @@
 	}
 
 	g_object_unref(cgmanager);
+
+	while (g_main_context_pending(creationcontext)) {
+		g_main_context_iteration(creationcontext, TRUE /* may block */);
+	}
+
 	g_main_context_unref(creationcontext);
 }
 

=== modified file 'libubuntu-app-launch/ubuntu-app-launch.c'
--- libubuntu-app-launch/ubuntu-app-launch.c	2015-12-07 17:03:12 +0000
+++ libubuntu-app-launch/ubuntu-app-launch.c	2015-12-07 17:03:12 +0000
@@ -38,7 +38,6 @@
 
 static void apps_for_job (GDBusConnection * con, const gchar * name, GArray * apps, gboolean truncate_legacy);
 static void free_helper (gpointer value);
-static GList * pids_for_appid (const gchar * appid);
 int kill (pid_t pid, int signal);
 static gchar * escape_dbus_string (const gchar * input);
 
@@ -588,7 +587,7 @@
 
 	do {
 		hash_table_size = g_hash_table_size(pidssignaled);
-		GList * pidlist = pids_for_appid(appid);
+		GList * pidlist = ubuntu_app_launch_get_pids(appid);
 		GList * iter;
 
 		if (pidlist == NULL) {
@@ -1506,9 +1505,10 @@
 /* Get the PIDs for an AppID. If it's click or legacy single instance that's
    a simple call to the helper. But if it's not, we have to make a call for
    each instance of the app that we have running. */
-static GList *
-pids_for_appid (const gchar * appid)
+GList *
+ubuntu_app_launch_get_pids (const gchar * appid)
 {
+	g_return_val_if_fail(appid != NULL, NULL);
 	ual_tracepoint(pids_list_start, appid);
 
 	GDBusConnection * cgmanager = cgroup_manager_connection();
@@ -1572,7 +1572,7 @@
 		return FALSE;
 	}
 
-	GList * pidlist = pids_for_appid(appid);
+	GList * pidlist = ubuntu_app_launch_get_pids(appid);
 	GList * head;
 
 	for (head = pidlist; head != NULL; head = g_list_next(head)) {

=== modified file 'libubuntu-app-launch/ubuntu-app-launch.h'
--- libubuntu-app-launch/ubuntu-app-launch.h	2015-08-10 14:19:05 +0000
+++ libubuntu-app-launch/ubuntu-app-launch.h	2015-12-07 17:03:12 +0000
@@ -388,6 +388,18 @@
 GPid       ubuntu_app_launch_get_primary_pid           (const gchar *                     appid);
 
 /**
+ * ubuntu_app_launch_get_pids:
+ * @appid: ID of the application to look for
+ *
+ * Checks to see if an application is running and returns
+ * the PIDs associated with it.
+ *
+ * Return Value: (transfer full) (element-type GLib.Pid): A list
+ *   of PIDs associated with @appid, empty if not running.
+ */
+GList *     ubuntu_app_launch_get_pids                 (const gchar *                     appid);
+
+/**
  * ubuntu_app_launch_pid_in_app_id:
  * @pid: Process ID to check on
  * @appid: ID of the application to look in

=== modified file 'tools/CMakeLists.txt'
--- tools/CMakeLists.txt	2014-09-10 15:01:22 +0000
+++ tools/CMakeLists.txt	2015-12-07 17:03:12 +0000
@@ -9,6 +9,15 @@
 install(TARGETS ubuntu-app-list RUNTIME DESTINATION "${CMAKE_INSTALL_FULL_BINDIR}")
 
 ########################
+# ubuntu-app-list-pids
+########################
+
+add_executable(ubuntu-app-list-pids ubuntu-app-list-pids.c)
+set_target_properties(ubuntu-app-list-pids PROPERTIES OUTPUT_NAME "ubuntu-app-list-pids")
+target_link_libraries(ubuntu-app-list-pids ubuntu-launcher)
+install(TARGETS ubuntu-app-list-pids RUNTIME DESTINATION "${CMAKE_INSTALL_FULL_BINDIR}")
+
+########################
 # ubuntu-app-launch
 ########################
 

=== added file 'tools/ubuntu-app-list-pids.c'
--- tools/ubuntu-app-list-pids.c	1970-01-01 00:00:00 +0000
+++ tools/ubuntu-app-list-pids.c	2015-12-07 17:03:12 +0000
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2015 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 "libubuntu-app-launch/ubuntu-app-launch.h"
+
+int
+main (int argc, char * argv[])
+{
+	GList * pids;
+	GList * iter;
+
+	if (argc != 2) {
+		g_printerr("Usage: %s <app id>\n", argv[0]);
+		return 1;
+	}
+
+	pids = ubuntu_app_launch_get_pids(argv[1]);
+	if (pids == NULL) {
+		return 1;
+	}
+
+	for (iter = pids; iter != NULL; iter = g_list_next(iter)) {
+		g_print("%d\n", (GPid)GPOINTER_TO_INT(iter->data));
+	}
+
+	g_list_free(pids);
+
+	return 0;
+}
+