Closed
Bug 73757
Opened 24 years ago
Closed 22 years ago
Local files not handled by moz should not be copied to temp directory
Categories
(Core Graveyard :: File Handling, defect, P2)
Core Graveyard
File Handling
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.5alpha
People
(Reporter: pabs3, Assigned: Biesinger)
References
(Blocks 2 open bugs, )
Details
(Keywords: arch, perf)
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
Biesinger
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
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?
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
Updated•23 years ago
|
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
Comment 8•23 years ago
|
||
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•23 years ago
|
||
Adding keywords from duplicated bug and nominating for mozilla1.0.1 as duplicated bug has targeted to this milestone.
Comment 11•22 years ago
|
||
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•22 years ago
|
||
-> 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
Comment 13•22 years ago
|
||
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•22 years ago
|
||
-> file handling
Assignee: new-network-bugs → law
Component: Networking → File Handling
QA Contact: benc → sairuh
Updated•22 years ago
|
QA Contact: sairuh → petersen
Comment 20•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
> 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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
> 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 31•22 years ago
|
||
Aha! Wrong keyword search on my part. Okay, back to the temp directory issue... <grin>
Updated•22 years ago
|
Priority: -- → P2
Target Milestone: --- → mozilla1.5beta
Updated•22 years ago
|
Blocks: ExternalViewSource
Assignee | ||
Comment 34•22 years ago
|
||
taking, I hope bz doesn't mind
Assignee: bzbarsky → cbiesinger
Target Milestone: mozilla1.5beta → mozilla1.5alpha
Assignee | ||
Comment 36•22 years ago
|
||
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)
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Comment 37•22 years ago
|
||
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-
Assignee | ||
Comment 38•22 years ago
|
||
this should address bz's comments
Attachment #126739 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #126756 -
Flags: review?(bzbarsky)
Comment 39•22 years ago
|
||
Comment on attachment 126756 [details] [diff] [review] patch v2 r=bzbarsky
Attachment #126756 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Updated•22 years ago
|
Attachment #126756 -
Flags: review+ → review?(darin)
Assignee | ||
Comment 40•22 years ago
|
||
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 41•22 years ago
|
||
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-
Assignee | ||
Comment 42•22 years ago
|
||
Assignee | ||
Updated•22 years ago
|
Attachment #126756 -
Attachment is obsolete: true
Assignee | ||
Comment 43•22 years ago
|
||
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 44•22 years ago
|
||
Comment on attachment 126771 [details] [diff] [review] patch v3 sr=darin
Attachment #126771 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 45•22 years ago
|
||
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: 22 years ago
Resolution: --- → FIXED
Comment 47•19 years ago
|
||
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?
Assignee | ||
Comment 49•19 years ago
|
||
(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•19 years ago
|
||
(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?
Assignee | ||
Comment 51•19 years ago
|
||
> in LaunchWithApplication, we have no way to access the channel
hm? why do you need that?
Comment 52•19 years ago
|
||
(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.
Assignee | ||
Comment 53•19 years ago
|
||
oh right, I misremembered the code. isn't there mChannel? (not valid after onStopRequest though... hm...)
Comment 54•19 years ago
|
||
Updated•19 years ago
|
Attachment #196626 -
Attachment is obsolete: true
Assignee | ||
Comment 56•19 years ago
|
||
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•19 years ago
|
||
(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
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•