If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

opening a file with the default helper app can fail by using the wrong app in the end

RESOLVED FIXED in mozilla1.9beta5

Status

Core Graveyard
File Handling
P2
normal
RESOLVED FIXED
10 years ago
a year ago

People

(Reporter: wolfiR, Assigned: glandium)

Tracking

({regression})

Trunk
mozilla1.9beta5
x86
Linux
regression
Bug Flags:
blocking1.9 +
in-testsuite ?
in-litmus ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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

10 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

10 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

10 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

10 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

10 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.
Component: OS Integration → File Handling
Product: Firefox → Core
QA Contact: os.integration → file-handling
Flags: blocking1.9?
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

10 years ago
Created attachment 308440 [details] [diff] [review]
maybe patch

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

10 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

10 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

10 years ago
I can confirm that my testcase works correctly again.
I haven't tried a default helper using arguments though.
(Assignee)

Comment 10

10 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

10 years ago
Created attachment 308570 [details] [diff] [review]
patch

So, this should be okay
Attachment #308440 - Attachment is obsolete: true
Attachment #308570 - Flags: review?(cbiesinger)
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

10 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

10 years ago
Created attachment 308736 [details] [diff] [review]
alternative patch

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 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++
(Assignee)

Comment 17

10 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 ?

yeah, I'm talking about the space
(Assignee)

Comment 19

10 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?
yeah
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
Attachment #308736 - Flags: approval1.9?
Is a testcase possible for this code?
Flags: in-testsuite?
Keywords: checkin-needed
(Assignee)

Comment 24

10 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.
(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

10 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.
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
Last Resolved: 10 years ago
Keywords: checkin-needed
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.
Flags: in-litmus?
(Reporter)

Comment 29

10 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.
(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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.