Closed Bug 73757 Opened 23 years ago Closed 21 years ago

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

Categories

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

Product:
Core Graveyard
Component:
File Handling
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: pabs3, Assigned: Biesinger)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: arch, perf)

Attachments

(2 files, 3 obsolete files)

patch v3
8.17 KB, patch
Biesinger
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
patch for attachment of local file
3.04 KB, patch
Details | Diff | Splinter Review 
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?
-->gordon
Assignee: neeti → 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?
Assignee: gordon → 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...
Assignee: gagan → mscott
Component: Networking: Cache → Networking
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

Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
*** Bug 94628 has been marked as a duplicate of this bug. ***
Summary: Local files not handled by moz should not go to disk cache → Local files not handled by moz should not be copied to temp directory
*** Bug 120999 has been marked as a duplicate of this bug. ***
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.
Blocks: 59932
Depends on: 86640
OS: Windows 98 → All
Hardware: PC → All
Adding keywords from duplicated bug and nominating for mozilla1.0.1 as
duplicated bug has targeted to this milestone.
Keywords: arch, mozilla1.0.1, perf
*** Bug 158458 has been marked as a duplicate of this bug. ***
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. :)
-> file w/ defaults
+neeti b/c she made some changes here.
does anyone mind? gordon isn't qa, and mscott hasn't responded...
Assignee: mscott → new-network-bugs
QA Contact: gordon → benc
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.
-> file handling
Assignee: new-network-bugs → law
Component: Networking → File Handling
QA Contact: benc → sairuh
I may as well take this...
Assignee: law → bzbarsky
QA Contact: sairuh → petersen
*** Bug 176804 has been marked as a duplicate of this bug. ***
*** Bug 170913 has been marked as a duplicate of this bug. ***
*** Bug 185720 has been marked as a duplicate of this bug. ***
*** Bug 170914 has been marked as a duplicate of this bug. ***
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.
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 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.
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.


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!
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.
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?
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.
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).
> 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...
Aha!  Wrong keyword search on my part.  Okay, back to the temp directory
issue... <grin>
Priority: -- → P2
Target Milestone: --- → mozilla1.5beta
*** Bug 205313 has been marked as a duplicate of this bug. ***
*** Bug 210722 has been marked as a duplicate of this bug. ***
taking, I hope bz doesn't mind
Assignee: bzbarsky → cbiesinger
Target Milestone: mozilla1.5beta → mozilla1.5alpha
Attached patch patch (obsolete) — — Splinter Review 
includes a little bit of unrelated cleanup
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.
Attachment #126739 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
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!
Attachment #126739 - Flags: review?(bzbarsky) → review-
Attached patch patch v2 (obsolete) — — Splinter Review 
this should address bz's comments
Attachment #126739 - Attachment is obsolete: true
Attachment #126756 - Flags: review?(bzbarsky)
Comment on attachment 126756 [details] [diff] [review]
patch v2

r=bzbarsky
Attachment #126756 - Flags: review?(bzbarsky) → review+
Attachment #126756 - Flags: review+ → review?(darin)
Comment on attachment 126756 [details] [diff] [review]
patch v2

err, re-marking bz's review+, I put darin into the wrong field...
Attachment #126756 - Flags: superreview?(darin)
Attachment #126756 - Flags: review?(darin)
Attachment #126756 - Flags: review+
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)
Attachment #126756 - Flags: superreview?(darin) → superreview-
Attachment #126756 - Attachment is obsolete: true
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
Attachment #126771 - Flags: superreview?(darin)
Attachment #126771 - Flags: review+
Comment on attachment 126771 [details] [diff] [review]
patch v3

sr=darin
Attachment #126771 - Flags: superreview?(darin) → superreview+
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
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
*** Bug 218470 has been marked as a duplicate of this bug. ***
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?
I don't see how we can avoid that in that case.
(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.
(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?
> in LaunchWithApplication, we have no way to access the channel

hm? why do you need that?
(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.


oh right, I misremembered the code. isn't there mChannel? (not valid after
onStopRequest though... hm...)
Attachment #196626 - Attachment is obsolete: true
can we discuss that patch in a new bug, i.e. in one that's not been marked
resolved for a long time now?
(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
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: