Closed Bug 634942 Opened 9 years ago Closed 9 years ago

HelperApp launch is broken with GIO

Categories

(Core Graveyard :: File Handling, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla2.0

People

(Reporter: wolfiR, Unassigned)

Details

Attachments

(2 files)

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.
Attached patch possible patchSplinter Review
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 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+
Attached patch patch #2Splinter Review
same with two NS_ENSURE_SUCCESS checks
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+
https://hg.mozilla.org/mozilla-central/rev/372eca6704c2
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.