Closed Bug 391980 Opened 17 years ago Closed 17 years ago

New Download manager automatically associates all types of files including folders with one file type

Categories

(Firefox :: File Handling, defect, P1)

Product:
Firefox
Component:
File Handling
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 beta2

People

(Reporter: ht990332, Assigned: ventnor.bugzilla)

References

Details

Attachments

(1 file, 8 obsolete files)

Plug leak
5.24 KB, patch
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081222 Firefox/3.0a8pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a8pre) Gecko/2007081222 Firefox/3.0a8pre

New Download manager automatically associates all types of files including folders with one file type.
I downloaded an image file. I right clicked on the downloaded image file in the download manager, I clicked 'Open', it gave me a dialog and I pointed it to /usr/bin/gthumb (an image
viewer) and selected 'remember my choice for file links' and it successfully
opened the image file using the image viewer.

However, when I clicked 'open containing folder', firefox attempted to open the folder using  the image viewer instead of opening my download folder using nautilus. 

I downloaded a .doc file. when I open it in the download manager, it still tries to open it in the image viewer I associated the image file with.

Reproducible: Always
The problem is that the DM uses nsIExternalProtocalHandler to launch a file:// uri.  When you associated the image with the program, you actually associated the file:// protocol to it. :(
The easiest fix for this specific bug is to change the download manager to use the MIME-type for launching rather than the file: protocol handler.

A slightly broader fix would to simply blacklist file: to never allow the user to choose a protocol handler.  From a user experience standpoint, this seems like the right thing to do, since I can't think of any valid use case.  It would provide us with defense-in-depth, for the case in which someone figures out a way to trick the front-end into allowing users to register internally-handled protocols.  Though this isn't a terrible risk, given that protocols are always passed by reference, never by value.

More generally, it's worth wondering if file: is really unique here.  One could imagine this being a problem with other URI schemes that are simply file accessors  (jar:, sftp:, ...).  Thoughts?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
Two questions:

1)  What changed to break this?
2)  Would it make sense to treat URIs that we can get a local file from as files
    instead?
(In reply to comment #3)
> Two questions:
> 
> 1)  What changed to break this?

The fact that, by default, except for blacklisted protocols, we now offer a
dialog for protocol handlers that we don't handle internally.

> 2)  Would it make sense to treat URIs that we can get a local file from as
> files instead?

In this specific case, I think so.  What actually happens is that we try to use nsILocalFile.reveal to show the folder, but since that always fails on Linux, we fall back to using the protocol handler in that case.  Fixing to fall back to use the nsIMIMEInfo launch method on the file itself would be a

If you're asking about the more general case of all handoff to the local OS, I don't really have a good feel for this.  At the very least, we'd want to do a some research to see where in the tree this is done, and test it on the various OSes to understand how the user-experience will change (if at all).
> The fact that, by default, except for blacklisted protocols, we now offer a
> dialog for protocol handlers that we don't handle internally.

Er... Except we _do_ handle file:/// internally.

In any case, what I meant was "why did this code work before the dialog changes?"  Or did it not work?

> If you're asking about the more general case of all handoff to the local OS

I meant the case of file:// loads being done through the helper app service.  Note that we already do just that for the helper app case there.
(In reply to comment #5)
> > The fact that, by default, except for blacklisted protocols, we now offer a
> > dialog for protocol handlers that we don't handle internally.
> 
> Er... Except we _do_ handle file:/// internally.

You're right, my characterization was wrong.  It's really any URI scheme that anyone asks nsIExternalProtocolHandlerService (i.e. the external helper app service) about and for which the service doesn't have RDF indicating that it should be handled in some other way.

> In any case, what I meant was "why did this code work before the dialog
> changes?"  Or did it not work?

Because we always used to use the default system handler, and never offer to let the user select another one.  Now offering to select is the default.

> > If you're asking about the more general case of all handoff to the local OS
> 
> I meant the case of file:// loads being done through the helper app service. 
> Note that we already do just that for the helper app case there.

That sounds reasonable to me.
Component: Download Manager → File Handling
QA Contact: download.manager → file.handling
Version: unspecified → Trunk
OS: Linux → All
I haven't really given this enough thought yet, but bug 392403 makes me wonder if, rather than special casing file: URIs here, we want to do something more general involving the "expose" preferences.
(In reply to comment #1)
> The problem is that the DM uses nsIExternalProtocalHandler to launch a file://
> uri.  When you associated the image with the program, you actually associated
> the file:// protocol to it. :(

You should stop doing that...

I'd recommend getting a MIME info for the file and using launchWithFile.


nevermind, I didn't realize that you did this to show the folder. not sure what the best solution for that is.
(In reply to comment #9)
> (In reply to comment #1)
> > The problem is that the DM uses nsIExternalProtocalHandler to launch a file://
> > uri.  When you associated the image with the program, you actually associated
> > the file:// protocol to it. :(
> 
> You should stop doing that...
> 
> I'd recommend getting a MIME info for the file and using launchWithFile.
> 

Yes, that should fix it, correct? This worked fine in firefox2. It would open each downloaded file with the correct program using the system's mine info/file associations.

As for the second part of the bug, use the desktop's file manager to open folder locations.  i.e nautilus on gnome.

(In reply to comment #10)
> nevermind, I didn't realize that you did this to show the folder. not sure what
> the best solution for that is.
> 
For clarification, it doesn't matter whether it is a folder or not. When I associated the downloaded image with the image viewer, then downloaded a .doc file, it also attempted to open the .doc file with the image viewer. 
As Shawn Wilsher mentioned, it's incorrectly associating the protocol file:// and not the file type with the program.
On a similar note, if you Click on a file to download, firefox will offer to either save it or open it with the correct program. This mean that this is is broken only from inside the DM window.
At some level, the best solution would be to fix nsLocalFileUnix::Reveal to do the right thing on GNOME rather than just returning NS_ERROR_FAILURE.  mwu: any idea how much work this would be?

A possible workaround would be to do what SeaMonkey does, and open a browser window rooted at the parent directory: 

http://lxr.mozilla.org/seamonkey/source/xpfe/components/download-manager/resources/downloadmanager.js#277
Target Milestone: --- → Firefox 3 M8
If there is a problem figuring out what application opens folders, I think there are the mimetypes 'inode/directory' or 'x-directory'. both open with nautilus.
Litmus Triage Team: ctalbert or stephend, which one of you will handle this test case (crossover areas?).
As a workaround, I associated file:// with /usr/bin/xdg-open in firefox.
xdg-open itself will handle both protocols ( file:// , http:// ...... ) and mimetypes and file will open with correct program everytime. This is because xdg-open will handle both protocol and mimetype at the same time even if we are actually using if for the protocol file://
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 M8 → ---
Assignee: nobody → dmose
Target Milestone: --- → Firefox 3 M9
Priority: -- → P1
This is a Unix-only bug; fixing nsILocalFile.Launch (and maybe .Reveal) should be sufficient to fix it.  My understanding is that that's not too hard.
OS: All → Linux
(In reply to comment #17)
> Litmus Triage Team: ctalbert or stephend, which one of you will handle this
> test case (crossover areas?).

Me.
Target Milestone: Firefox 3 M9 → Firefox 3 M10
Whiteboard: [proto]
The GNOME VFS API's might be able to handle this...
Whats the difference between Launch and Reveal?
For a PDF, say, Launch will start a PDF viewer, while Reveal will open the parent dir in the user's file manager.
This is definitely fixable with the GNOME VFS functions, the problem is they are purely C and extern "C" doesn't seem to work on them. I have code which compiles but doesn't build, I keep getting "undefined reference" errors. Hopefully dmose has better luck than me.
Michael: can you attach a patch and as well paste in the link errors you're seeing?  I can try and poke at that...
Attached patch Patch with problems (obsolete) — — Splinter Review 
This patch passes the compiling stage so I know I've included all the headers I need. But then it reaches the linking stage and I get this:

../../staticlib/libxpcom_core.a(nsLocalFileUnix.o): In function `nsLocalFile::Launch()':
nsLocalFileUnix.cpp:(.text+0xaa9): undefined reference to `gnome_vfs_get_file_mime_type'
nsLocalFileUnix.cpp:(.text+0xab1): undefined reference to `gnome_vfs_mime_can_be_executable'
nsLocalFileUnix.cpp:(.text+0xb1d): undefined reference to `gnome_vfs_make_uri_from_input'
nsLocalFileUnix.cpp:(.text+0xb27): undefined reference to `gnome_vfs_url_show'
collect2: ld returned 1 exit status
make[3]: *** [libxul.so] Error 1
You need to change EXTRA_DSO_LDOPTS or some such?  Add $(MOZ_GCONF_LIBS) and $(GLIB_LIBS) maybe?  Not sure the exact magic incantation...
(In reply to comment #27)
> You need to change EXTRA_DSO_LDOPTS or some such?  Add $(MOZ_GCONF_LIBS) and
> $(GLIB_LIBS) maybe?  Not sure the exact magic incantation...

I've tried it before but it doesn't make a difference. I copied that rule from the Makefile of a directory where we do successfully use it but it still doesn't work.
Attached patch xpcom/build/Makefile.in (obsolete) — — Splinter Review 
Adding these changes on top of your patch makes things link on my Ubuntu box.  

Also, one thing bsmedberg pointed out to me is that for XPCOM to accept this dependency, whatever APIs we use need to be supported on versions of GNOME at least old as those listed on <http://wiki.mozilla.org/Linux/Runtime_Requirements>.
(In reply to comment #29)
> Created an attachment (id=288080) [details]
> xpcom/build/Makefile.in
> 
> Adding these changes on top of your patch makes things link on my Ubuntu box.  
> 
> Also, one thing bsmedberg pointed out to me is that for XPCOM to accept this
> dependency, whatever APIs we use need to be supported on versions of GNOME at
> least old as those listed on
> <http://wiki.mozilla.org/Linux/Runtime_Requirements>.

It doesn't work for me. Does the patch work as expected for opening files on your machine?


Can you please attach a patch with EVERYTHING, including my stuff, that was changed to get this to build? I'm getting very fed up with the build system here...
Why don't you add that function to nsIGnomeVFSService and implement it in toolkit/system/gnome/nsGnomeVFSService.cpp instead of making xpcom directly depend on gnome-vfs ?
(In reply to comment #32)
> Why don't you add that function to nsIGnomeVFSService and implement it in
> toolkit/system/gnome/nsGnomeVFSService.cpp instead of making xpcom directly
> depend on gnome-vfs ?
> 

That would require XPCOM to depend on toolkit, resulting in a circular dependency and hell for embedders.

I'd like to know if dmose did anything else to make my patch compile.
Actually your approach is hell for embedders which want to replace gnome-vfs (with GIO in my case). Only the interface would need to live in xpcom (nsISystemVFSService ?), the implementation could be external.
If you or someone has a better approach then please go ahead and take this bug. I'm having too much trouble with this myself and I'm not working on this bug anymore.
Attached patch complete merged patch (v2) (obsolete) — — Splinter Review 
Michael: sorry the build system is misbehaving for you.  Here's the complete patch; it does indeed work nicely.  This is an up-to-date trunk build running on Gutsy Gibbon.
Attachment #288060 - Attachment is obsolete: true
Attachment #288080 - Attachment is obsolete: true
bsmedberg: as module owner, what's your opinion about which sort of dependency we want here (cf comments 32-34)?
So the more important question now is does this patch work as expected? :-)
It does indeed work as expected; I verified with a couple of Components.utils.reportErrors that launch was really being used from the download manager and was doing the right thing.
I works for me too (ArchLinux / Gnome 2.20.1 / gcc 4.2.2)
I downloaded a tar.gz file and it opened correctly in file-roller from the download manager.

But it causes a crash when you open a plain text file from the download manager instead of opening them with gedit . http://pastebin.com/d10cabad1 (the crash doesn't happen with other files, only plain text files)
Attached patch Patch 2 (obsolete) — — Splinter Review 
I still can't get it to build, you might be using a different build method to me or something. I think I might wipe my tree and start over to see if that makes any difference...

Here's my attempt at reveal. Please test and report.
Attachment #288248 - Attachment is obsolete: true
PS I think I fixed Hussam's problem, it looked like a double-free. Please test this again, and also the new reveal?
Comment on attachment 288262 [details] [diff] [review]
Patch 2

Couldn't you cut down the function duplication by doing something like this:

NS_IMETHODIMP
nsLocalFile::Reveal()
{
#ifndef MOZ_WIDGET_GTK2
    return NS_ERROR_FAILURE;
#endif

    PRBool isDirectory;
    if (NS_FAILED(IsDirectory(&isDirectory)))
        return NS_ERROR_FAILURE;
...

Seems like that would make the code more readable. :)
I suppose I could do that, but first of all I want to know if the two functions even work correctly :-)
Removing the double free from Launch gets rid of the crash with text files, but it still doesn't do the right thing in that case (ie ends up returning NS_ERROR_FAILURE).  Note that other types (eg PDF) do and did work.

Reveal appears to work nicely.

Thanks for continuing to poke at this!
Attached patch Patch 3 (obsolete) — — Splinter Review 
Its obvious that VFS thinks your text file is an executable and is trying to execute it as an actual process. That path doesn't work so this patch removes it, thus relegating as much work as I possibly can to the GNOME libraries. If something doesn't work right with this patch then its likely to be GNOME's fault.

I think this patch is ready for review though I wouldn't have a clue who to request it from. dmose? bsmedberg?
Attachment #288262 - Attachment is obsolete: true
(In reply to comment #44)
> I suppose I could do that, but first of all I want to know if the two functions
> even work correctly :-)

Well, in your latest patch, you got it backwards.

Do it as an #ifndef instead of an #ifdef. It makes the code much more readable, and it's what we do in other code.

See comment #43 for an example of what I mean.

(In reply to comment #46)
> I think this patch is ready for review though I wouldn't have a clue who to
> request it from. dmose? bsmedberg?

http://www.mozilla.org/owners.html#xpcom says bsmedberg owns XPCOM, so he might be a good reviewer?
Assignee: dmose → ventnor.bugzilla
(In reply to comment #47)
> (In reply to comment #44)
> > I suppose I could do that, but first of all I want to know if the two functions
> > even work correctly :-)
> 
> Well, in your latest patch, you got it backwards.
> 
> Do it as an #ifndef instead of an #ifdef. It makes the code much more readable,
> and it's what we do in other code.
> 
> See comment #43 for an example of what I mean.

But the non-GTK platforms will get compile errors since the compiler is still exposed to the code that uses GNOME functions, won't it? Thats why we need that #else.


(In reply to comment #48)
> But the non-GTK platforms will get compile errors since the compiler is still
> exposed to the code that uses GNOME functions, won't it? Thats why we need that
> #else.

Ah, duh. Sorry, my bad. Silly me this evening. :(
Attached patch New COM-based approach (obsolete) — — Splinter Review 
PCOM seems to not complain with this dependency when compiled, and the IDL is in the xpcom directory so it should be OK. This moves the library functions to nsGnomeVFSService which provides three benefits:

a) It makes it easier for embedders to provide their own implementation, including Epiphany's (or us!) switch to Gio when it is ready for prime-time.
b) XPCOM is no longer directly dependent on these Gnome libraries.
c) It actually compiles on my machine! This makes me unbelievably relieved.

Initially I was worried when the VFS service code was in the toolkit directory, but then I noticed the IDL in the xpcom directory so things should work out fine now.
Attachment #288277 - Attachment is obsolete: true
Sorry about before, Christian. I thought nsGnomeVFSService was a toolkit component.
Attachment #288280 - Flags: superreview?(benjamin)
Attachment #288280 - Flags: review?(benjamin)
Comment on attachment 288280 [details] [diff] [review]
New COM-based approach

>Index: xpcom/system/nsIGnomeVFSService.idl
>===================================================================
...

I think you need to revv the IID here since you're changing something.
Attached patch Change the UUID (obsolete) — — Splinter Review 
Right, I always forget that...
Attachment #288280 - Attachment is obsolete: true
Attachment #288281 - Flags: superreview?(benjamin)
Attachment #288281 - Flags: review?(benjamin)
Attachment #288280 - Flags: superreview?(benjamin)
Attachment #288280 - Flags: review?(benjamin)
(In reply to comment #53)
> Created an attachment (id=288281) [details]
> Change the UUID
> 
> Right, I always forget that...
> 

Works brilliantly regardless of file type (it fixes the crash above). 'Open containing folder' also opens the parent folder in nautilus. Excellent :-)
Whiteboard: [proto] → [has patch][needs review bsmedberg]
Why is that a noscript function? Also, you need to g_free the result from gnome_vfs_make_uri_from_input, don't you?
Comment on attachment 288281 [details] [diff] [review]
Change the UUID

I can't see any reason for the method on nsIGnomeVFSService not to be scriptable, but: it is documented to take a UTF-8 string, and yet we're passing it mPath of the localfile, which is in the native charset. Is there some external guarantee that the native charset is UTF8, or is the method misdocumented and should use ACString instead?
Attachment #288281 - Flags: superreview?(benjamin)
Attachment #288281 - Flags: review?(benjamin)
Attachment #288281 - Flags: review-
Attached patch Fix IDL (obsolete) — — Splinter Review 
Is this what you mean?

The reason I made it noscript is because I'd rather not allow extensions to pass any old string directly to a system library, they should go through nsIURI and all the sanitations it provides.
Attachment #288281 - Attachment is obsolete: true
Attachment #289641 - Flags: review?(benjamin)
That seems like a good reason to make it noscript; I'd suggest adding a doxygen comment to the IDL explaining that so that someone doesn't come along later and undo that.

That doesn't sound like a good reason to me... extensions can already do pretty much anything... why shouldn't they be allowed to pass arbitrary strings to a system library?
Because it makes it hard for the extension developer to naively do the wrong thing.  Unless there's some specific use case we want to support here...
(In reply to comment #59)
> That doesn't sound like a good reason to me... extensions can already do pretty
> much anything... why shouldn't they be allowed to pass arbitrary strings to a
> system library?
> 

Because there should be no reason for them to. They can make an nsIURI and thus go through all the safety checks that nsIURI does when one is created. showURIForInput is for our purposes only because we can trust mPath, we can't automatically trust any string that extensions may pass.
Attachment #289641 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
Whiteboard: [has patch][needs review bsmedberg] → [has patch][has review]
Attached patch Plug leak — — Splinter Review 
I couldn't use g_free before because I set it as const char, but this time it seems plain ol' char works too.
Attachment #289641 - Attachment is obsolete: true
bsmedberg said over IRC that the changes in the new patch are trivial enough that the review can be carried over.
Checking in xpcom/io/nsLocalFileUnix.cpp;
/cvsroot/mozilla/xpcom/io/nsLocalFileUnix.cpp,v  <--  nsLocalFileUnix.cpp
new revision: 1.137; previous revision: 1.136
done
Checking in xpcom/system/nsIGnomeVFSService.idl;
/cvsroot/mozilla/xpcom/system/nsIGnomeVFSService.idl,v  <--  nsIGnomeVFSService.idl
new revision: 1.3; previous revision: 1.2
done
Checking in toolkit/system/gnome/nsGnomeVFSService.cpp;
/cvsroot/mozilla/toolkit/system/gnome/nsGnomeVFSService.cpp,v  <--  nsGnomeVFSService.cpp
new revision: 1.4; previous revision: 1.3
done
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review]
I think this broke trunk on my x86_64 machine:
/usr/bin/ld: nsGnomeVFSService.o: relocation R_X86_64_PC32 against `gnome_vfs_make_uri_from_input' can not be used when making a shared object; recompile with -fPIC

Clobbering didn't help.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: