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

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Wolfgang Rosenauer [:wolfiR]]

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 )

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

That seems to be a regression to Firefox 2 actually. Using the same testcase with Firefox 2.0 doesn't reveal that issue.

Comment 2

[Wolfgang Rosenauer [:wolfiR]]

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.

Comment 3

[Wolfgang Rosenauer [:wolfiR]]

This regression is most likely caused by bug 273524

I tried a nightly from 2008-02-25 (before the checkin) and it works correctly.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[Mike Hommey [:glandium]]

Attachment: 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)

Comment 7

[Wolfgang Rosenauer [:wolfiR]]

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.

Comment 8

[Mike Hommey [:glandium]]

(In reply to comment #7)
> >+NS_IMETHODIMP
> >+nsGnomeVFSService::Launch(const nsACString &aUri)
> 
> nsGnomeVFSMimeApp::Launch

Damn copy/paste ;) Please tell me if it works.

Comment 9

[Wolfgang Rosenauer [:wolfiR]]

I can confirm that my testcase works correctly again.
I haven't tried a default helper using arguments though.

Comment 10

[Mike Hommey [:glandium]]

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

Comment 11

[Mike Hommey [:glandium]]

Attachment: patch

So, this should be okay

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 308570 [details] [diff] [review]
patch

+  [noscript] void launch(in ACString uri);

why noscript? and why not AUTF8String?

Comment 13

[Mike Hommey [:glandium]]

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

Comment 14

[Mike Hommey [:glandium]]

Attachment: 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)

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 308736 [details] [diff] [review]
alternative patch

+  if (! uri)

I assume that's the style of the file?

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

and yeah, in AUTF8String gets mapped to const nsACString& in C++

Comment 17

[Mike Hommey [:glandium]]

(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

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, I'm talking about the space

Comment 19

[Mike Hommey [:glandium]]

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?

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah

Comment 21

[Damon Sicore (:damons)]

Can we include a test here?  Waiting on approval until answered.

Comment 22

[Damon Sicore (:damons)]

OK, marking as blocking, so, please check in.  However, please include a test case as well.

Comment 23

[Reed Loden [:reed]]

Is a testcase possible for this code?

Comment 24

[Mike Hommey [:glandium]]

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

[Reed Loden [:reed]]

(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

Comment 26

[Mike Hommey [:glandium]]

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

[Reed Loden [:reed]]

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

Comment 28

[Reed Loden [:reed]]

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

Comment 29

[Wolfgang Rosenauer [:wolfiR]]

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

[Damon Sicore (:damons)]

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