Merge lp:~ted/ubuntu-app-launch/fdo-application-open into lp:ubuntu-app-launch/13.10

[Ted Gould]

On second activations send a message to the FD.o application interface

This branch changes the behavior so that when we detect an application that we're managing the single instance behavior of is already running we send a DBus message using the FD.o application interface to open the URLs that we have.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:75
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/88/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-amd64-ci/90
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-armhf-ci/89
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-i386-ci/88

Click here to trigger a rebuild:
http://s-jenkins:8080/job/upstart-app-launch-ci/88/rebuild

review: Approve (continuous-integration)

[Loïc Minier (lool)]

* the variable name "connection" is confusing in get_pid_cb() and contact_app(), could we call it connection_name?
* please add a TODO in main and in parse_uris() to use something else than space for splitting URIs; in fact it might be better to just remove support for multiple uris in this new code

So IIUC, we're first scanning by matching PID (PID that we think we've launched with upstart) then comparing the DBus bus name of the target app to match the app name; I find this a bit ugly because:
a) pids are reused, even if it's unlikely that it happens quickly and at the right time, it's bad when you can't guarantee it didn't happen
b) app names are not unique, only namespace + app name is

I'd rather we use namespace + app name as the bus name we're searching for, and connect to that; seems simple and solid. But I guess this was discussed elsewhere since we have to use the same scheme in e.g. qtubuntu?

[Loïc Minier (lool)]

I forgot one thing, these constructs:
   if ! start application-click APP_ID="${APP_ID}" APP_URIS="${APP_URIS}"; then
[...]
    if ! start application-legacy APP_ID="${APP_ID}" INSTANCE_ID="" APP_URIS="${APP_URIS}" ; then
that either start an applicatin or consider an application is already running always bugged me as fragile because a real start failure would be handled as if the app is running.

Surely we can do better?

start's exit code doesn't distinguish between types of failures (exit 1 for unknown job and for already started job), so I suggest we use "status"; if it's start/running, we open another URL, if it's anything else we start it.

[Ted Gould (ted)]

On Wed, 2013-09-25 at 09:41 +0000, Loïc Minier wrote:

> * the variable name "connection" is confusing in get_pid_cb() and
> contact_app(), could we call it connection_name?

Fixed r76.

> * please add a TODO in main and in parse_uris() to use something else
> than space for splitting URIs; in fact it might be better to just
> remove support for multiple uris in this new code

Comment added r77. At this point I think it's better to be consistent.
I hope that, assuming fires get put out, to fix this later this week.

> So IIUC, we're first scanning by matching PID (PID that we think we've
> launched with upstart) then comparing the DBus bus name of the target
> app to match the app name; I find this a bit ugly because:
> a) pids are reused, even if it's unlikely that it happens quickly and
> at the right time, it's bad when you can't guarantee it didn't happen
> b) app names are not unique, only namespace + app name is
>
> I'd rather we use namespace + app name as the bus name we're searching
> for, and connect to that; seems simple and solid. But I guess this
> was discussed elsewhere since we have to use the same scheme in e.g.
> qtubuntu?

I think you might have misread the code slightly. We're not using any
well known names on DBus, those aren't allowed by confinement. We're
only using object paths on the bus. So it is only important that the
object path be unique for the connection, which it is likely that the
application name would be. Also, this is what is specified by the FD.o
application interface on where to put the object. They assume all
application names will become something like
"org.freedesktop.foo.desktop" eventually, and we handle that case, but
also the case of "foo.desktop".

The lack of well-known names (which really have their own set of race
conditions) is what makes us interested in PIDs. Unfortunately DBus
doesn't have a quick way to turn a PID into a connection. So we have to
get the list of connections and get their PIDs. Then we can know the
unique name of the PID of the Upstart job. And once we know the DBus
unique name for it, we can send it a message.

[lp:~ted/ubuntu-app-launch/fdo-application-open]

76. By Ted Gould on 2013-09-25

Changing connection variable name to be more clear

77. By Ted Gould on 2013-09-25

Adding comment on spaces so we know where to fix

[Ted Gould (ted)]

On Wed, 2013-09-25 at 11:02 +0000, Loïc Minier wrote:

> Surely we can do better?
>
> start's exit code doesn't distinguish between types of failures (exit
> 1 for unknown job and for already started job), so I suggest we use
> "status"; if it's start/running, we open another URL, if it's anything
> else we start it.

Yes, and I expect to fix this with the failure branch (which is also
needed for Unity) that I plan on starting today. Those constructs were
only meant as a quick way to unblock people so that we can more
completely handle errors.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:77
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/90/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-amd64-ci/92
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-armhf-ci/91
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-i386-ci/90

Click here to trigger a rebuild:
http://s-jenkins:8080/job/upstart-app-launch-ci/90/rebuild

review: Approve (continuous-integration)

[Loïc Minier (lool)]

So we had a quick hangout with Ted and we confirmed b) above and we're going to talk to security team about a) to see whether this race is important enough to plug and whether it's hard to plug.

[Loïc Minier (lool)]

[ Happy to top approve this once b) is done and I have tested some binaries :-) ]

[Loïc Minier (lool)]

So security team said there was no easy fix, our apparmor stuff doesn't allow that yet; they also said this wasn't severe enough (risk of leaking one URL under hard to meet conditions). The longer term fix would be to add API to dbus to find processes better and/or to upstart to find application statup timestamp.

[lp:~ted/ubuntu-app-launch/fdo-application-open]

78. By Ted Gould on 2013-09-25

Move the getting of the PID to make it less racy

79. By Ted Gould on 2013-09-25

Merge in trunk/dont-stop branch to avoid conflicts later

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:79
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/93/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-amd64-ci/95
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-armhf-ci/94
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-i386-ci/93

Click here to trigger a rebuild:
http://s-jenkins:8080/job/upstart-app-launch-ci/93/rebuild

review: Approve (continuous-integration)

[lp:~ted/ubuntu-app-launch/fdo-application-open]

80. By Ted Gould on 2013-09-25

Use nih_dbus_path() to get the app path

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:80
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/94/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-amd64-ci/96
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-armhf-ci/95
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-i386-ci/94

Click here to trigger a rebuild:
http://s-jenkins:8080/job/upstart-app-launch-ci/94/rebuild

review: Approve (continuous-integration)

[lp:~ted/ubuntu-app-launch/fdo-application-open]

81. By Ted Gould on 2013-09-27

Stealing two dbus paths from window-focus-request

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:81
http://jenkins.qa.ubuntu.com/job/upstart-app-launch-ci/102/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-amd64-ci/104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-armhf-ci/103
    SUCCESS: http://jenkins.qa.ubuntu.com/job/upstart-app-launch-saucy-i386-ci/102

Click here to trigger a rebuild:
http://s-jenkins:8080/job/upstart-app-launch-ci/102/rebuild

review: Approve (continuous-integration)

[Loïc Minier (lool)]

It seems really wrong to have the app version in there; we're trying to find an unique identifier for applications, not application + version. "appid" is a bit misleading here.

[Ted Gould (ted)]

Tested by ricmm with loicm's branch for the SDK. This is ready to land in trunk. Signing off and pulling everything in!

=== modified file 'CMakeLists.txt'
--- CMakeLists.txt	2013-09-23 20:41:53 +0000
+++ CMakeLists.txt	2013-09-27 14:11:15 +0000
@@ -157,6 +157,15 @@
 target_link_libraries(zg-report-app ${ZEITGEIST_LIBRARIES})
 install(TARGETS zg-report-app RUNTIME DESTINATION "${pkglibexecdir}")
 
+#######################
+# fdo-application-open
+#######################
+
+add_executable(fdo-application-open fdo-application-open.c)
+set_target_properties(fdo-application-open PROPERTIES OUTPUT_NAME "fdo-application-open")
+target_link_libraries(fdo-application-open helpers upstart-launcher)
+install(TARGETS fdo-application-open RUNTIME DESTINATION "${pkglibexecdir}")
+
 ####################
 # application.conf
 ####################

=== modified file 'application.conf.in'
--- application.conf.in	2013-09-25 16:31:57 +0000
+++ application.conf.in	2013-09-27 14:11:15 +0000
@@ -22,16 +22,12 @@
 
 	if [ ! -z $CLICK_DIR ] && [ -d $CLICK_DIR ] ; then
 		if ! start application-click APP_ID="${APP_ID}" APP_URIS="${APP_URIS}"; then
-			# TODO: This is pure evil.
-			stop application-click APP_ID="${APP_ID}"
-			start application-click APP_ID="${APP_ID}" APP_URIS="${APP_URIS}"
+			@pkglibexecdir@/fdo-application-open "${APP_ID}" "${APP_URIS}"
 		fi
 	else
 		if @pkglibexecdir@/desktop-single $APP_ID ; then
 			if ! start application-legacy APP_ID="${APP_ID}" INSTANCE_ID="" APP_URIS="${APP_URIS}" ; then
-				# TODO: This is a little less, but still mostly evil.
-				stop application-legacy APP_ID="${APP_ID}" INSTANCE_ID="" || true
-				start application-legacy APP_ID="${APP_ID}" INSTANCE_ID="" APP_URIS="${APP_URIS}"
+				@pkglibexecdir@/fdo-application-open "${APP_ID}" "${APP_URIS}"
 			fi
 		else
 			start application-legacy APP_ID="${APP_ID}" INSTANCE_ID=`date -u +%s` APP_URIS="${APP_URIS}"

=== added file 'fdo-application-open.c'
--- fdo-application-open.c	1970-01-01 00:00:00 +0000
+++ fdo-application-open.c	2013-09-27 14:11:15 +0000
@@ -0,0 +1,291 @@
+/*
+ * 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>
+#include <nih/alloc.h>
+#include <libnih-dbus.h>
+#include "libupstart-app-launch/upstart-app-launch.h"
+#include "helpers.h"
+
+/* Globals */
+GPid app_pid = 0;
+GMainLoop * mainloop = NULL;
+guint connections_open = 0;
+const gchar * appid = NULL;
+const gchar * input_uris = NULL;
+GVariant * app_data = NULL;
+gchar * dbus_path = NULL;
+
+/* Lower the connection count and process if it gets to zero */
+static void
+connection_count_dec (void)
+{
+	connections_open--;
+	if (connections_open == 0) {
+		g_main_loop_quit(mainloop);
+	}
+	return;
+}
+
+/* Turn the input string into something we can send to apps */
+static void
+parse_uris (void)
+{
+	if (app_data != NULL) {
+		/* Already done */
+		return;
+	}
+
+	/* TODO: Joining only with space could cause issues with breaking them
+	   back out.  We don't have any cases of more than one today.  But, this
+	   isn't good.
+	   https://bugs.launchpad.net/upstart-app-launch/+bug/1229354
+	   */
+	GVariant * uris = NULL;
+	gchar ** uri_split = g_strsplit(input_uris, " ", 0);
+	if (uri_split[0] == NULL) {
+		g_free(uri_split);
+		uris = g_variant_new_array(G_VARIANT_TYPE_STRING, NULL, 0);
+	} else {
+		GVariantBuilder builder;
+		g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);
+
+		int i;
+		for (i = 0; uri_split[i] != NULL; i++) {
+			g_variant_builder_add_value(&builder, g_variant_new_take_string(uri_split[i]));
+		}
+		g_free(uri_split);
+
+		uris = g_variant_builder_end(&builder);
+	}
+
+	GVariant * platform = g_variant_new_array(G_VARIANT_TYPE("{sv}"), NULL, 0);
+
+	GVariantBuilder tuple;
+	g_variant_builder_init(&tuple, G_VARIANT_TYPE_TUPLE);
+	g_variant_builder_add_value(&tuple, uris);
+	g_variant_builder_add_value(&tuple, platform);
+
+	app_data = g_variant_builder_end(&tuple);
+	g_variant_ref_sink(app_data);
+
+	return;
+}
+
+/* Finds us our dbus path to use.  Basically this is the name
+   of the application with dots replaced by / and a / tacted on
+   the front.  This is recommended here:
+
+   http://standards.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus   
+*/
+static void
+app_id_to_dbus_path (void)
+{
+	if (dbus_path != NULL) {
+		return;
+	}
+
+	dbus_path = nih_dbus_path(NULL, "", appid, NULL);
+	g_debug("DBus Path: %s", dbus_path);
+
+	return;
+}
+
+/* Finish the send and decrement the counter */
+static void
+send_open_cb (GObject * object, GAsyncResult * res, gpointer user_data)
+{
+	GError * error = NULL;
+
+	g_dbus_connection_call_finish(G_DBUS_CONNECTION(object), res, &error);
+
+	if (error != NULL) {
+		/* Mostly just to free the error, but printing for debugging */
+		g_debug("Unable to send Open: %s", error->message);
+		g_error_free(error);
+	}
+
+	connection_count_dec();
+	return;
+}
+
+/* Sends the Open message to the connection with the URIs we were given */
+static void
+contact_app (GDBusConnection * bus, const gchar * dbus_name)
+{
+	parse_uris();
+	app_id_to_dbus_path();
+
+	/* Using the FD.o Application interface */
+	g_dbus_connection_call(bus,
+		dbus_name,
+		dbus_path,
+		"org.freedesktop.Application",
+		"Open",
+		app_data,
+		NULL,
+		G_DBUS_CALL_FLAGS_NONE,
+		-1,
+		NULL,
+		send_open_cb, NULL);
+
+	g_debug("Sending Open request to: %s", dbus_name);
+
+	return;
+}
+
+/* Gets the PID for a connection, and if it matches the one we're looking
+   for then it tries to send a message to that connection */
+static void
+get_pid_cb (GObject * object, GAsyncResult * res, gpointer user_data)
+{
+	gchar * dbus_name = (gchar *)user_data;
+	GError * error = NULL;
+	GVariant * vpid = NULL;
+
+	vpid = g_dbus_connection_call_finish(G_DBUS_CONNECTION(object), res, &error);
+
+	if (error != NULL) {
+		g_warning("Unable to query PID for dbus name '%s': %s", dbus_name, error->message);
+		g_error_free(error);
+		g_free(dbus_name);
+
+		/* Lowering the connection count, this one is terminal, even if in error */
+		connection_count_dec();
+		return;
+	}
+
+	guint pid = 0;
+	g_variant_get(vpid, "(u)", &pid);
+	g_variant_unref(vpid);
+
+	if (pid == app_pid) {
+		/* Trying to send a message to the connection */
+		contact_app(G_DBUS_CONNECTION(object), dbus_name);
+	} else {
+		/* See if we can quit now */
+		connection_count_dec();
+	}
+
+	g_free(dbus_name);
+
+	return;
+}
+
+int
+main (int argc, char * argv[])
+{
+	if (argc != 3) {
+		g_error("Should be called as: %s <app_id> <uri list>", argv[0]);
+		return 1;
+	}
+
+	appid = argv[1];
+	input_uris = argv[2];
+
+	/* DBus tell us! */
+	GError * error = NULL;
+	GDBusConnection * session = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error);
+	if (error != NULL) {
+		g_error("Unable to get session bus");
+		g_error_free(error);
+		return 1;
+	}
+
+	/* List all the connections on dbus.  This sucks that we have to do
+	   this, but in the future we should add DBus API to do this lookup
+	   instead of having to do it with a bunch of requests */
+	GVariant * listnames = g_dbus_connection_call_sync(session,
+		"org.freedesktop.DBus",
+		"/",
+		"org.freedesktop.DBus",
+		"ListNames",
+		NULL,
+		G_VARIANT_TYPE("(as)"),
+		G_DBUS_CALL_FLAGS_NONE,
+		-1,
+		NULL,
+		&error);
+
+	if (error != NULL) {
+		g_warning("Unable to get list of names from DBus: %s", error->message);
+		g_error_free(error);
+		return 1;
+	}
+
+	/* Next figure out what we're looking for (and if there is something to look for) */
+	/* NOTE: We're getting the PID *after* the list of connections so
+	   that some new process can't come in, be the same PID as it's
+	   connection will not be in teh list we just got. */
+	app_pid = upstart_app_launch_get_primary_pid(appid);
+	if (app_pid == 0) {
+		g_warning("Unable to find pid for app id '%s'", argv[1]);
+		return 1;
+	}
+
+	/* Allocate the mainloop now as we know we're going async */
+	mainloop = g_main_loop_new(NULL, FALSE);
+
+	GVariant * names = g_variant_get_child_value(listnames, 0);
+	GVariantIter iter;
+	g_variant_iter_init(&iter, names);
+	gchar * name = NULL;
+
+	while (g_variant_iter_loop(&iter, "s", &name)) {
+		/* We only want to ask each connection once, this makes that so */
+		if (!g_dbus_is_unique_name(name)) {
+			continue;
+		}
+
+		/* Get the PIDs */
+		g_dbus_connection_call(session,
+			"org.freedesktop.DBus",
+			"/",
+			"org.freedesktop.DBus",
+			"GetConnectionUnixProcessID",
+			g_variant_new("(s)", name),
+			G_VARIANT_TYPE("(u)"),
+			G_DBUS_CALL_FLAGS_NONE,
+			-1,
+			NULL,
+			get_pid_cb, g_strdup(name));
+
+		connections_open++;
+	}
+
+	g_variant_unref(names);
+	g_variant_unref(listnames);
+
+	if (connections_open != 0) {
+		g_main_loop_run(mainloop);
+	}
+
+	if (app_data != NULL) {
+		g_variant_unref(app_data);
+	}
+
+	g_main_loop_unref(mainloop);
+	g_object_unref(session);
+
+	if (dbus_path != NULL) {
+		nih_free(dbus_path);
+	}
+
+	return 0;
+}