73757 - Local files not handled by moz should not be copied to temp directory

Status: RESOLVED FIXED

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

Description

[Pabs]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; 0.8.1)
BuildID:    2001032704

When files in local drives/dirs of file types that moz doesn't handle are
accessed they to get put in the disk cache. They should not!
Copying files from local hard disks to the disk cache is an utter waste of
resources. Why make a copy when there is already one at the same level? In other
words there is no time saving from caching any item back to where is is located
(in terms of relative load time)

Reproducible: Always
Steps to Reproduce:
1.Pick any file on your local drive(s) that mozilla doesn't handle (exe,zip,etc)
2.Give its path to mozilla's address box & load it
3.Open it using any program (ie text/hex editor) that will allow you to find out
the file name **and path**

Actual Results:  You will find that the file has some munged name and is in the
in the disk cache directory (ie c:\windows\temp\, /tmp etc)

Expected Results:  1.Not copied the file to the disk cache
2.Used the requested file instead of the cache one

NOTE: File types moz does handle (text,html,images etc) don't go to the disk
cache. What's up with that?

Comment 1

[neeti]

-->gordon

Comment 2

[gordon]

The file: protocol does not use the cache, and the temp directory is not the 
cache directory.

Gagan, who is the owner for file: protocol?

Comment 3

[Gagan]

This actually has nothing to do with cache. The URILoader is the culprit here. 
When we download a file for an external handler-- we temporarily put it in the 
temp directory which is why you are seeing it there. I agree that we should not 
be doing this copy for local/file: URLs. Over to mscott! mscott: let me know if 
you need help with this...

Comment 4

[Pabs]

I think I tracked down the prob using lxr
see
http://lx.mozilla.org/seamonkey/sourcer/uriloader/exthandler/nsExternalHelperAppService.cpp#792
where nsExternalAppHandler::OnStartRequest is located
notice >>802   nsresult rv = SetUpTempFile(aChannel);
nsExternalAppHandler::SetUpTempFile is found on line 698
I propose the fix to be located in nsExternalAppHandler::OnStartRequest
replacing line 802 and be something like:

nsresult rv;
aChannel->GetURI(getter_AddRefs(mSourceUrl));
nsCOMPtr<nsIURL> url = do_QueryInterface(mSourceUrl);
if( ?? IsLocal (url) ?? )
{
  rv = ?? fix up the file name ??
}
else
{
  rv = SetUpTempFile(aChannel);
}

Sorry I don't know much about coding the ?? parts

Comment 5

[Keyser Sose]

Marking NEW.

Comment 6

[Jesse Ruderman]

*** Bug 94628 has been marked as a duplicate of this bug. ***

Comment 7

[Jesse Ruderman]

*** Bug 120999 has been marked as a duplicate of this bug. ***

Comment 8

[Boris Zbarsky [:bzbarsky]]

We can test whether a url is local by qi-ing to nsIFileURL and then getting the
nsIFile from it and testing whether it's null.

At this point we also have the nsIFile, so no need to fixup anything.

Marking dependency on bug 86640 which will be touching some of this code and
will change how it works.... once that lands, I may take a stab at this.

Comment 9

[Adam Hauner]

Adding keywords from duplicated bug and nominating for mozilla1.0.1 as
duplicated bug has targeted to this milestone. 

Comment 10

[Wesha]

*** Bug 158458 has been marked as a duplicate of this bug. ***

Comment 11

[imran.geriskovan]

I see that everybody involved below mention about the performance or other
aspects. They asked "Why do we make a copy while we have the original?"

But the need for editing/manuplating the original file with the helper app may
be another incentive for fixing this.

Copying the dupe:

While viewing a local file (ie. file://bla/bla.html) in browser I can edit it
with composer in its place using "File/Edit Page" menu item.

However this is not the case for documents of external apps. For example when I
click on a link file://bla/bla.xls, first the file is
copied to temp directory and then excel is launched for the copied file.

This makes editing of local docs impossible while using Mozilla as a shell.
I think Mozilla should launch the application on the original file.
Now you may wonder what my intention is...
Well... I would like to use Mozilla as a shell and replacement for
Windows/Internet Explorer. :) And it seems it is very likely. :)

Comment 12

[benc]

-> file w/ defaults
+neeti b/c she made some changes here.
does anyone mind? gordon isn't qa, and mscott hasn't responded...

Comment 13

[Boris Zbarsky [:bzbarsky]]

So what we should in fact do is that OnStartRequest should simply check whether
the url is a file url; if it is we should just get the nsIFile from it and use
that instead of constructing one of our own (we'll probably want to Cancel() the
request too).  I may be able to get to this sometime... but not for a few months.

Comment 14

[benc]

-> file handling

Comment 15

[Boris Zbarsky [:bzbarsky]]

I may as well take this...

Comment 16

[Boris Zbarsky [:bzbarsky]]

*** Bug 176804 has been marked as a duplicate of this bug. ***

Comment 17

[Matthias Versen [:Matti]]

*** Bug 170913 has been marked as a duplicate of this bug. ***

Comment 18

[Matthias Versen [:Matti]]

*** Bug 185720 has been marked as a duplicate of this bug. ***

Comment 19

[Michael Lefevre]

*** Bug 170914 has been marked as a duplicate of this bug. ***

Comment 20

[Matthieu MOY]

There is something more :

When I click on an executable file on a local filesystem, I expect the file to
be executed. Instead, I get the download window, and as mozilla doesn't know
what to do, (type application/octet-stream), It suggests me to open it with less.

We have a real problem with that on the gnuwin project (http://gnuwin.epfl.ch) :
We use HTML pages as an interface to install software, so, the user clicks links
untill he gets a link to the installation file (eg: setup.exe).

In MSIE, the file is executed, the application is installed, ...

Currently, we can find no way to do this with mozilla, so, we have to use MSIE,
which is a pity.

Comment 21

[Jason Bassford]

I believe a decision was made that EXEs would never be executed, only
downloaded.  This was for security reasons.  However, I see no reason for
*local* EXEs to be subject to this restriction...

Comment 22

[imran.geriskovan]

> I believe a decision was made that EXEs would never be executed, only
> downloaded.  This was for security reasons.  However, I see no reason for
> *local* EXEs to be subject to this restriction...

I agree..
And there is more:
This way, Mozilla can never able to act as a operating system shell.
(Like almost_merged_Internet/Windows Explorer.)

I always hear from "Mozilla people" that "Mozilla" is never intended to be a
shell or filemanager. I may understand scarcity of their resources.
But I can never understand their _attitude_ for reducing the options for the use
of Mozilla.

Comment 23

[Boris Zbarsky [:bzbarsky]]

1)  If you want a feature, write it (you say you understand lack of resources;
    I fail to see you understanding it based on your comment -- scarcity of
    resources implies not having resources for major projects like writing a
    file manager; if someone writes it, it could well be accepted into the
    tree).
2)  Comment 20 through comment 22 have _nothing_ to do with this bug.  Please
    take them elsewhere (newsgroups would be a good place for that sort of
    discussion).  This bug is about a real problem -- the fact that we
    incorrectly launch local files.  Whether we should launch local executables
    is a whole different kettle of fish and will require changes to totally
    different code.

Comment 24

[imran.geriskovan]

Those comments are closely related with the bug, if a well defined
framework/approach/policy is to be developed for "Local File Handling".

You'd better keep them in your mind, even if you don't want to!

Comment 25

[Boris Zbarsky [:bzbarsky]]

I've been keeping it in mind for about a year now.  Bug I have no plans to
develop such a framework for two reasons: lack of resources (time) and lack of
need (I never plan to use Mozilla as a file launcher).  Feel free to step up and
do something useful if you _do_ want it to be a file launcher.  And stop
threatening people -- it's rude.

Comment 26

[imran.geriskovan]

No ruder than "go_somewhere_else" attitude OR declaring comments as
invalid/nonsense when you don't agree...

It is clear that handling of local executables is also related with this bug.
But you deny it. Why? Do you always expect ideally isolated problems?

Comment 27

[Jason Bassford]

To be fair, the issue of local executables is NOT strictly on topic for this bug
(although it's related), which is about the storing of files downloaded from the
local filesystem to a temp directory (rather than simply using them where they are).

I may have inadvertantly contributed to this off-topic discussion by answering
the original question in comment 20 - I apologize for the thread it's now
generated.  I hadn't thought, at the time, that a one comment clarification
would have been so momentous. <grin>

To address issues of executing *local* EXEs a separate bug should be filed; to
address issues of local file handling in general, a separate tracking bug should
be filed, which would mark this bug (and newly filed local EXE bug) as depedencies.

Further discussion of local EXE execution and local filesystem handling in
general (as well as, most especially, personal comments from all parties
concerned) should be taken elsewhere.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Where do you see a "go somewhere else" attitude?  The only attitude I see being 
expressed is "this is not a priority at the moment; if it's important to you to 
see this soon you should try to find the resources to do it".

I never said the comments were invalid or nonsense.  Just that they belonged in 
a separate bug so that they would not get lost when this bug is fixed.  Please 
do find that bug (and it exists, I note) and put the comments in there (if they 
are not already present, which they are).

I don't always expect ideally isolated problems, but I can guarantee to you 
that this bug and the local executable problem are very well isolated indeed 
and can be fixed quite independently of each other (the code involved lives not 
only in wo different files but mostly in two different _modules_ right now).

Comment 29

[Jason Bassford]

> Please do find that bug (and it exists, I note)

Do you have a bug number?  A search on exe and local didn't produce anything I
could see.  Short of that, I'll just file a new bug myself (which I wouldn't
object to having duped) if for no other reason than to get this discussion off
of the radar here...

Comment 30

[Boris Zbarsky [:bzbarsky]]

bug 59932 (see also bug 59096).

Comment 31

[Jason Bassford]

Aha!  Wrong keyword search on my part.  Okay, back to the temp directory
issue... <grin>

Comment 32

[Erik]

*** Bug 205313 has been marked as a duplicate of this bug. ***

Comment 33

[Jo Hermans]

*** Bug 210722 has been marked as a duplicate of this bug. ***

Comment 34

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

taking, I hope bz doesn't mind

Comment 35

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

Attachment: patch

includes a little bit of unrelated cleanup

Comment 36

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

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

bz, what do you think?

I tested this on linux, seems to work fine, will do win32 testing as well.

Comment 37

[Boris Zbarsky [:bzbarsky]]

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

>Index: nsExternalHelperAppService.cpp

>     nsExternalAppHandler * handler = CreateNewExternalHandler(mimeInfo, fileExtension.get(), aWindowContext);
>-    handler->QueryInterface(NS_GET_IID(nsIStreamListener), (void **) aStreamListener);
>+    CallQueryInterface(handler, aStreamListener);

While you're here, return NS_ERROR_OUT_OF_MEMORY if handler is null?

>+      nsCOMPtr<nsPIExternalAppLauncher> helperAppService =
>+                         do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID);

Can't you just QI our weak mHelperAppService pointer?  Same thing in the place
you copied this code from....

>+      if (helperAppService) {
>+        nsresult rv = helperAppService->LaunchAppWithTempFile(mMimeInfo, file);
>+        if (NS_SUCCEEDED(rv)) {
>+          Cancel();
>+          return NS_OK;

>+        }

What if it failed?  Then we go through all the normal rigmarole?  Wouldn't it
make more sense to report an error and so forth like the code in
OpenWithApplication() ?  (I'm not quite sure which is better; just trying to
decide...)

Looks great other than those nits!

Comment 38

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

Attachment: patch v2

this should address bz's comments

Comment 39

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126756 [details] [diff] [review]
patch v2

r=bzbarsky

Comment 40

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

Comment on attachment 126756 [details] [diff] [review]
patch v2

err, re-marking bz's review+, I put darin into the wrong field...

Comment 41

[Darin Fisher]

Comment on attachment 126756 [details] [diff] [review]
patch v2

>Index: nsExternalHelperAppService.cpp

>     nsExternalAppHandler * handler = CreateNewExternalHandler(mimeInfo, fileExtension.get(), aWindowContext);
>+    if (!handler)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    CallQueryInterface(handler, aStreamListener);

no need for a QI.  |handler| is a |nsIStreamListener|.

      *aStreamListener = handler;


>+  // Now check if the file is local, in which case we won't bother with saving
>+  // it to a temporary directory and just launch it from where it is
>+  nsCOMPtr<nsIFileURL> fileUrl(do_QueryInterface(mSourceUrl));
>+  if (fileUrl)
>+  {

nit: inconsistent brace style.	others places put opening brace on same
line as conditional.


>+      if (mHelperAppService) {
>+        rv = mHelperAppService->LaunchAppWithTempFile(mMimeInfo, file);

hmm... you use mHelperAppService elsewhere without null checking it.
why do that here?


>+    if (file)
>+      file->GetPath(path);

nit:
	rv = file->GetPath(path)

Comment 42

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

Attachment: patch v3

Comment 43

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

Comment on attachment 126771 [details] [diff] [review]
patch v3

transferring r=bzbarsky

darin: it seems to me like the most common style in that file is { on its own
line, so I left it like that in this patch

Comment 44

[Darin Fisher]

Comment on attachment 126771 [details] [diff] [review]
patch v3

sr=darin

Comment 45

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

checked in, thank for the reviews
Checking in nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <-- 
nsExternalHelperAppService.cpp
new revision: 1.194; previous revision: 1.193
done
Checking in nsExternalHelperAppService.h;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.h,v  <-- 
nsExternalHelperAppService.h
new revision: 1.44; previous revision: 1.43
done

Comment 46

[Matthias Versen [:Matti]]

*** Bug 218470 has been marked as a duplicate of this bug. ***

Comment 47

[Nian Liu(n/a in a long time)]

Boris, Christian

What if the file not handled by mozilla is an attachment of a local eml file?
Shoud we create a file under /tmp?

Comment 48

[Boris Zbarsky [:bzbarsky]]

I don't see how we can avoid that in that case.

Comment 49

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

(In reply to comment #47)
> What if the file not handled by mozilla is an attachment of a local eml file?
> Shoud we create a file under /tmp?

I believe we do, don't we? hm... I wonder if it wouldn't have been better to QI
the channel instead of the uri.

Comment 50

[Nian Liu(n/a in a long time)]

(In reply to comment #49)
> (In reply to comment #47)
> > What if the file not handled by mozilla is an attachment of a local eml file?
> > Shoud we create a file under /tmp?
> 
> I believe we do, don't we? hm... I wonder if it wouldn't have been better to QI
> the channel instead of the uri.

I agree with QI the channel. The issue is that in LaunchWithApplication, we have
no way to access the channel. Should we add new member variable?

Comment 51

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

> in LaunchWithApplication, we have no way to access the channel

hm? why do you need that?

Comment 52

[Nian Liu(n/a in a long time)]

(In reply to comment #51)
> > in LaunchWithApplication, we have no way to access the channel
> 
> hm? why do you need that?

because the logic of QI uri is in LaunchWithApplication.

Comment 53

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

oh right, I misremembered the code. isn't there mChannel? (not valid after
onStopRequest though... hm...)

Comment 54

[Nian Liu(n/a in a long time)]

Attachment: patch for attachment of local file

Comment 55

[Nian Liu(n/a in a long time)]

Attachment: patch for attachment of local file

How about this one?

Comment 56

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

can we discuss that patch in a new bug, i.e. in one that's not been marked
resolved for a long time now?

Comment 57

[Nian Liu(n/a in a long time)]

(In reply to comment #56)
> can we discuss that patch in a new bug, i.e. in one that's not been marked
> resolved for a long time now?

pls. see 309241