634942 - HelperApp launch is broken with GIO

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

If GIO (instead of GnomeVFS) is used for HelperApp handling the Launch() method fails to do the right thing.

https://mxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsGIOService.cpp#135
this method is using g_app_info_launch_uris() which expects a list of URIs (not files).
But 
https://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#137
simply calls the method with a string containing the native local path.

The matching GnomeVFS method converts the path to a uri apparently:
https://mxr.mozilla.org/mozilla-central/source/toolkit/system/gnome/nsGnomeVFSService.cpp#105

I haven't found a fitting gio function to do a similar thing.
I have a candidate patch but it's currently changing the caller instead of the callee. I'm not sure what's the correct way to fix it.
The interface description suggests it should be called via uri and not file path but then it has been wrong for years because the callee fixed it silently.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Attachment: possible patch

This one fixes the caller as described and it isn't very intrusive leaving the old non-gio code alone. 
As I said I'm unsure on which side to fix it in the end.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 513194 [details] [diff] [review]
possible patch

Karl, asking for review but as written I'm not sure which side to fix.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 513194 [details] [diff] [review]
possible patch

This makes sense to me.

>+    nsCOMPtr<nsIIOService> ioservice = do_GetService(NS_IOSERVICE_CONTRACTID);
>+    nsCOMPtr<nsIURI> uri;
>+    ioservice->NewFileURI(aFile, getter_AddRefs(uri));

Should null check ioservice, I think.

>+    uri->GetSpec(uriSpec);

I saw another NewFileURI use where uri was not null-checked, so I don't know whether a null check on uri is necessary or not.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

Attachment: patch #2

same with two NS_ENSURE_SUCCESS checks

Comment 5

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 513194 [details] [diff] [review]
possible patch

That one is basically NPOTB (unless --enable-gio is used) and the original code just does not work. Not sure if I need approval in that case?

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 513194 [details] [diff] [review]
possible patch

a=beltzner

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

https://hg.mozilla.org/mozilla-central/rev/372eca6704c2