Closed Bug 421879 Opened 13 years ago Closed 13 years ago
opening a file with the default helper app can fail by using the wrong app in the end
I just noticed the following which is a bit hard to explain: (I think it's not trunk only but was problematic in the past as well) I "download" a file and get a default application offered to open that file based on gnome-vfs information (HelperAppService). If I choose to open that file with the default application, Firefox starts to download the file and afterwards tries to open it but uses another application at that point. I figured out that gnome-vfs chooses the right application by using the extension or mimetype given for the file but operating gnome-open (gnome_url_show) on the local saved file opens another application. So my guess is that Firefox displays the right application as default handler based on mimetypes and tries to start the default application by just using gnome_url_show() with the file as parameter. But gnome-vfs has a different opinion what's the default application since it does content sniffing for local files and has no access to the mime-type information from the server anymore. I haven't found the actual location in the code yet to prove my guess though. (Some more information here: https://bugzilla.novell.com/show_bug.cgi?id=368238 )
Summary: opening a file with the default helper app can fail → opening a file with the default helper app can fail by using the wrong app in the end
That seems to be a regression to Firefox 2 actually. Using the same testcase with Firefox 2.0 doesn't reveal that issue.
I'm a bit confused still to find the correct execution path for opening those applications. Probably it ends up here: http://mxr.mozilla.org/seamonkey/source/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp#64 and http://mxr.mozilla.org/seamonkey/source/toolkit/system/gnome/nsGnomeVFSService.cpp#249 That would explain why gnome_url_show() is used in the end when using the default handler. Firefox 2 doesn't have nsMIMEInfoUnix::LaunchDefaultWithFile AFAICT.
This regression is most likely caused by bug 273524 I tried a nightly from 2008-02-25 (before the checkin) and it works correctly.
Your analysis sounds correct. Replacing vfs->ShowURIForInput(nativePath) with something that would launch the nsIGnomeVFSMimeApp should be enough to fix this, but that will need some additions to the interface.
Component: OS Integration → File Handling
Product: Firefox → Core
QA Contact: os.integration → file-handling
We need to fix this (as in, this should block). Treating the data as something other than the server MIME type is a security risk.
I only had enough time to come up with a theorical patch. This is untested (I don't even know if it builds). I unfortunately won't be able to test soon, which is why i'm throwing the patch now so that someone can ;) Note it also fixes a bad uuid bump that happened on nsIGnomeVFSService.idl (on nsIGnomeVFSMimeApp, while the change happened on nsIGnomeVFSService)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Comment on attachment 308440 [details] [diff] [review] maybe patch >diff --git a/toolkit/system/gnome/nsGnomeVFSService.cpp b/toolkit/system/gnome/nsGnomeVFSService.cpp >index 0d866ce..6c19e0e 100644 >--- a/toolkit/system/gnome/nsGnomeVFSService.cpp >+++ b/toolkit/system/gnome/nsGnomeVFSService.cpp >@@ -103,6 +103,32 @@ nsGnomeVFSMimeApp::GetExpectsURIs(PRInt32* aExpects) > return NS_OK; > } > >+NS_IMETHODIMP >+nsGnomeVFSService::Launch(const nsACString &aUri) nsGnomeVFSMimeApp::Launch With that change it compiles at least. I still need to test if it works correctly.
(In reply to comment #7) > >+NS_IMETHODIMP > >+nsGnomeVFSService::Launch(const nsACString &aUri) > > nsGnomeVFSMimeApp::Launch Damn copy/paste ;) Please tell me if it works.
I can confirm that my testcase works correctly again. I haven't tried a default helper using arguments though.
(In reply to comment #9) > I can confirm that my testcase works correctly again. > I haven't tried a default helper using arguments though. It shouldn't be a problem.
So, this should be okay
Comment on attachment 308570 [details] [diff] [review] patch + [noscript] void launch(in ACString uri); why noscript? and why not AUTF8String?
Attachment #308570 - Flags: review?(cbiesinger) → review+
(In reply to comment #12) > (From update of attachment 308570 [details] [diff] [review]) > + [noscript] void launch(in ACString uri); > > why noscript? and why not AUTF8String? I must say i copied that from ShowURIForInput in the same file. I thought ACStrings were not available in scripts.
Again, not had time to test if this builds or works. Am I right assuming changing the type in the .idl file is enough ? (it seems so, looking at the other functions)
Comment on attachment 308736 [details] [diff] [review] alternative patch + if (! uri) I assume that's the style of the file?
Attachment #308736 - Flags: review?(cbiesinger) → review+
and yeah, in AUTF8String gets mapped to const nsACString& in C++
(In reply to comment #15) > (From update of attachment 308736 [details] [diff] [review]) > + if (! uri) > > I assume that's the style of the file? There is no other NULL pointer check in the file actually. but I thought it was not in mozilla style to explicitely test for NULL pointers... or are you talking about the space ?
yeah, I'm talking about the space
Comment on attachment 308736 [details] [diff] [review] alternative patch > yeah, I'm talking about the space That can surely be fixed when committing, can't it?
Can we include a test here? Waiting on approval until answered.
OK, marking as blocking, so, please check in. However, please include a test case as well.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Is a testcase possible for this code?
What kind of testcase ? We can probably attach here some $somemimetype file with $othermimetype forced in the Content-Type, if that's what you want.
(In reply to comment #24) > What kind of testcase ? We can probably attach here some $somemimetype file > with $othermimetype forced in the Content-Type, if that's what you want. Want an automated testcase that can be run by our unit test machines: http://developer.mozilla.org/en/docs/Mozilla_automated_testing
I have absolutely no idea how automated tests can check whether the proper program has been launched, nor how one can ensure some proper gnome config in order to do the proper testing (if the setup happens to have the same program for both mime types from the testcase, that's not going to be useful). Moreover, I don't have immediate time for that, and won't before beta 5 freeze, that's for sure.
Checking in toolkit/system/gnome/nsGnomeVFSService.cpp; /cvsroot/mozilla/toolkit/system/gnome/nsGnomeVFSService.cpp,v <-- nsGnomeVFSService.cpp new revision: 1.6; previous revision: 1.5 done Checking in uriloader/exthandler/unix/nsMIMEInfoUnix.cpp; /cvsroot/mozilla/uriloader/exthandler/unix/nsMIMEInfoUnix.cpp,v <-- nsMIMEInfoUnix.cpp new revision: 1.4; previous revision: 1.3 done Checking in xpcom/system/nsIGnomeVFSService.idl; /cvsroot/mozilla/xpcom/system/nsIGnomeVFSService.idl,v <-- nsIGnomeVFSService.idl new revision: 1.4; previous revision: 1.3 done
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
(In reply to comment #26) > I have absolutely no idea how automated tests can check whether the proper > program has been launched, nor how one can ensure some proper gnome config in > order to do the proper testing (if the setup happens to have the same program > for both mime types from the testcase, that's not going to be useful). If you can provide exact steps-to-reproduce for a manual testcase, maybe just a Litmus test will suffice.
This special bug is caused by the fact that we get different default applications from gnome-vfs: - when we search with the server provided mime-type we get one application - when gnome-vfs is looking at the file (using gnome_url_show() via content sniffing) it uses a different one The example how I found it was an XML file which was provided with a certain non-standard mime-type from the server. So Firefox found the right application from the mime database but ended up not using this information but just handed the file over to libgnome what was sniffing the file and found that it's XML (and just opened Firefox with the local file as parameter). That's the background. It should be possible to create a testcase out of it.
(In reply to comment #29) > That's the background. It should be possible to create a testcase out of it. > Would be really great to get one. Helps out the project in the long run. Thanks for the background.
You need to log in before you can comment on or make changes to this bug.