HelperApp launch is broken with GIO

RESOLVED FIXED in mozilla2.0

Status

RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: wolfiR, Unassigned)

Tracking

Trunk
mozilla2.0
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
Created attachment 513194 [details] [diff] [review]
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.
(Reporter)

Comment 2

8 years ago
Comment on attachment 513194 [details] [diff] [review]
possible patch

Karl, asking for review but as written I'm not sure which side to fix.
Attachment #513194 - Flags: review?(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.
Attachment #513194 - Flags: review?(karlt) → review+
(Reporter)

Comment 4

8 years ago
Created attachment 515658 [details] [diff] [review]
patch #2

same with two NS_ENSURE_SUCCESS checks
(Reporter)

Comment 5

8 years ago
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?
Attachment #513194 - Flags: approval2.0?
Comment on attachment 513194 [details] [diff] [review]
possible patch

a=beltzner
Attachment #513194 - Flags: approval2.0? → approval2.0+
(Reporter)

Comment 7

8 years ago
https://hg.mozilla.org/mozilla-central/rev/372eca6704c2
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
(Assignee)

Updated

2 years ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.