Closed
Bug 421879
Opened 16 years ago
Closed 16 years ago
opening a file with the default helper app can fail by using the wrong app in the end
Categories
(Core Graveyard :: File Handling, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9beta5
People
(Reporter: wolfiR, Assigned: glandium)
References
Details
(Keywords: regression)
Attachments
(1 file, 2 obsolete files)
2.63 KB,
patch
|
Biesinger
:
review+
|
Details | Diff | Splinter Review |
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 )
Reporter | ||
Updated•16 years ago
|
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
Reporter | ||
Comment 1•16 years ago
|
||
That seems to be a regression to Firefox 2 actually. Using the same testcase with Firefox 2.0 doesn't reveal that issue.
Keywords: regression
Reporter | ||
Comment 2•16 years ago
|
||
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.
Reporter | ||
Comment 3•16 years ago
|
||
This regression is most likely caused by bug 273524 I tried a nightly from 2008-02-25 (before the checkin) and it works correctly.
Assignee | ||
Comment 4•16 years ago
|
||
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.
Updated•16 years ago
|
Component: OS Integration → File Handling
Product: Firefox → Core
QA Contact: os.integration → file-handling
Updated•16 years ago
|
Flags: blocking1.9?
![]() |
||
Comment 5•16 years ago
|
||
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.
Blocks: 273524
Assignee | ||
Comment 6•16 years ago
|
||
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
Reporter | ||
Comment 7•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
(In reply to comment #7) > >+NS_IMETHODIMP > >+nsGnomeVFSService::Launch(const nsACString &aUri) > > nsGnomeVFSMimeApp::Launch Damn copy/paste ;) Please tell me if it works.
Reporter | ||
Comment 9•16 years ago
|
||
I can confirm that my testcase works correctly again. I haven't tried a default helper using arguments though.
Assignee | ||
Comment 10•16 years ago
|
||
(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.
Assignee | ||
Comment 11•16 years ago
|
||
So, this should be okay
Attachment #308440 -
Attachment is obsolete: true
Attachment #308570 -
Flags: review?(cbiesinger)
Comment 12•16 years ago
|
||
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+
Assignee | ||
Comment 13•16 years ago
|
||
(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.
Assignee | ||
Comment 14•16 years ago
|
||
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)
Attachment #308570 -
Attachment is obsolete: true
Attachment #308736 -
Flags: review?(cbiesinger)
Comment 15•16 years ago
|
||
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+
Comment 16•16 years ago
|
||
and yeah, in AUTF8String gets mapped to const nsACString& in C++
Assignee | ||
Comment 17•16 years ago
|
||
(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 ?
Comment 18•16 years ago
|
||
yeah, I'm talking about the space
Assignee | ||
Comment 19•16 years ago
|
||
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?
Attachment #308736 -
Flags: approval1.9?
Comment 20•16 years ago
|
||
yeah
Comment 21•16 years ago
|
||
Can we include a test here? Waiting on approval until answered.
Comment 22•16 years ago
|
||
OK, marking as blocking, so, please check in. However, please include a test case as well.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Updated•16 years ago
|
Attachment #308736 -
Flags: approval1.9?
Comment 23•16 years ago
|
||
Is a testcase possible for this code?
Flags: in-testsuite?
Keywords: checkin-needed
Assignee | ||
Comment 24•16 years ago
|
||
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.
Comment 25•16 years ago
|
||
(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
Assignee | ||
Comment 26•16 years ago
|
||
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.
Comment 27•16 years ago
|
||
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: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9beta5
Comment 28•16 years ago
|
||
(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.
Flags: in-litmus?
Reporter | ||
Comment 29•16 years ago
|
||
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.
Comment 30•16 years ago
|
||
(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.
Updated•7 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•