This is because shell_app_info_launch_full() is passing G_SPAWN_DO_NOT_REAP_CHILD, and then not setting up a child watch.

Colin, why is it passing that flag?

(In reply to comment #1)
> This is because shell_app_info_launch_full() is passing
> G_SPAWN_DO_NOT_REAP_CHILD, and then not setting up a child watch.
> 
> Colin, why is it passing that flag?

Because we were expecting to find a ShellApp instance, and then _gather_pid_callback would eventually tell ShellWindowTracker to call g_child_watch_add.  But in this case we weren't finding the app, so it was NULL, and would cause a g_return_if_fail, and thus we'd never set up the watch.

Created attachment 183086 [details] [review]
placeDisplay: Remove network places

It hasn't existed in nautilus for a while now, and we don't
have places at all.

Created attachment 183087 [details] [review]
Remove broken ShellAppSystem API and all consumers

In commit 9bd22dc0, I introduced an API to load an arbitrary
.desktop file, not necessarily from the menu path.  It turns
out this function was broken because it created ShellApp instances
that were *different* from ones that were cached normally.

As far as I can tell, we didn't initially use it.  Then later
Util.spawnDesktop was created which used this function.

Remove this broken function and all callers; if we're loading
.desktop files from *outside* the menu path, we can look at
readding.

This patch also kills off Util.spawnDesktop in favor of callers
talking to ShellAppSystem directly - I'd much prefer we move
to reducing the number of layers here.

For example - application launching errors should really be handled
both more generically and better; the code should not live in util.js
to create a system notification. For now, simply logging an exception
to stderr (~/.xsession-errors) is probably fine, actually better even.

Giving this to danw to review, don't really understand the logic of the utils.spawnDesktop removal. It was added to have a consistent way of launching and error handling throughout the code, and then we go back and don't have such a way? This doesn't seem to me to be progress. If we have some better way we think it should be done, then we should do it now, or live with the current code until we have time to do it. It also seems inappropriately in this patch - the patch looks like it could be split in two trivially.

*** Bug 644548 has been marked as a duplicate of this bug. ***

*** Bug 644685 has been marked as a duplicate of this bug. ***

Comment on attachment 183087 [details] [review]
Remove broken ShellAppSystem API and all consumers

(In reply to comment #5)
> Giving this to danw to review, don't really understand the logic of the
> utils.spawnDesktop removal. It was added to have a consistent way of launching
> and error handling throughout the code, and then we go back and don't have such
> a way? This doesn't seem to me to be progress.

I agree. I made the same comment when Colin posted this to IRC.

>-        Util.spawnDesktop('gnome-datetime-panel');
>+        var app = Shell.AppSystem.get_default().get_app('gnome-datetime-panel.desktop');
>+        app.activate(-1);

also, it seems to me that even if it wasn't going to do error checking, it's still nicer to keep the utility method. Just change what spawnDesktop does internally. (If not, the 'var' should be 'let'.)

>     _onOpenCalendarActivate: function() {
>         this.menu.close();
>         // TODO: pass '-c calendar' (to force the calendar at startup)
>         // TODO: pass the selected day
>-        Util.spawnDesktop('evolution');

not sure why this hasn't been fixed to just use spawn() anyway...

Comment on attachment 183086 [details] [review]
placeDisplay: Remove network places

Attachment 183086 [details] pushed as f079501 - placeDisplay: Remove network places

Created attachment 183475 [details] [review]
Add shell_global_report_async_nonfatal()

Move the "system notification error" handling out of
util.js, and add it to ShellGlobal so we can start
calling it from across the codebase better (including
C).

Created attachment 183476 [details] [review]
ShellApp: Report error when we fail to run an app

Created attachment 183477 [details] [review]
Remove broken ShellAppSystem API and all consumers

In commit 9bd22dc0, I introduced an API to load an arbitrary
.desktop file, not necessarily from the menu path.  It turns
out this function was broken because it created ShellApp instances
that were *different* from ones that were cached normally.

As far as I can tell, we didn't initially use it.  Then later
Util.spawnDesktop was created which used this function.

Remove this broken function and all callers; if we're loading
.desktop files from *outside* the menu path, we can look at
readding.

This patch also kills off Util.spawnDesktop in favor of callers
talking to ShellAppSystem directly, now that the latter reports
errors.

(In reply to comment #8)
>
> >     _onOpenCalendarActivate: function() {
> >         this.menu.close();
> >         // TODO: pass '-c calendar' (to force the calendar at startup)
> >         // TODO: pass the selected day
> >-        Util.spawnDesktop('evolution');
> 
> not sure why this hasn't been fixed to just use spawn() anyway...

I thought we decided elsewhere that all uses of directly executing apps[1] should be via .desktop files, so in this case evolution should have (probably a NoDisplay=true) .desktop file to launch the calendar?

[1] By app I mean apps, not components like "gvfs-open"

Comment on attachment 183475 [details] [review]
Add shell_global_report_async_nonfatal()

I like the code, I don't like the name. "Async nonfatal what?" I'd call the method shell_global_report_error() and the signal report-error. Or "warning" if "error" implies "fatal" to you.

Comment on attachment 183476 [details] [review]
ShellApp: Report error when we fail to run an app

>+              msg = g_strdup_printf (_("Failed to launch '%s'"), shell_app_get_name (app));

freeze break and you need to update POTFILES.in

Comment on attachment 183477 [details] [review]
Remove broken ShellAppSystem API and all consumers

>-        Util.spawnDesktop('gnome-datetime-panel');
>+        var app = Shell.AppSystem.get_default().get_app('gnome-datetime-panel.desktop');
>+        app.activate(-1);

what I said before. It would be simpler to just change spawnDesktop() to do that, rather than adding all that boilerplate. But if you really must remove spawnDesktop(), fix the "var"s to be "let"s.

(In reply to comment #14)
> (From update of attachment 183475 [details] [review])
> I like the code, I don't like the name. "Async nonfatal what?" I'd call the
> method shell_global_report_error() and the signal report-error. Or "warning" if
> "error" implies "fatal" to you.

Well...it's tricky because "warning" and "error" are both overloaded with the programming level ones (g_warning, g_error).  Practically speaking those end up as random crud in ~/.xsession-errors, and I want to distinguish this method from those.  How about _report_async_nonfatal_problem?

"async" is also pretty overloaded, and I don't think we need it in the name of this method. (I'd leave out "nonfatal" too... I think it should generally be assumed that most methods *won't* exit/abort/etc.)

"notify" might be better than "report" to distinguish from "dump into .xsession-errors where no one will ever see it". (And since it does actually use the notification mechanism.)

Attachment 183476 [details] pushed as 4bf1df0 - ShellApp: Report error when we fail to run an app
Attachment 183477 [details] pushed as fea8b6d - Remove broken ShellAppSystem API and all consumers

Whats left here ?

nothing