299839 - If I use the option 'reset camino' it removes downloaded files out of my download location.

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[MT]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050705 Camino/0.9a1+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b2) Gecko/20050705 Camino/0.9a1+

When I download files (applications, images, etc) and then select the option to
'reset camino' my downloaded files will be (presumably) deleted.  For example; I
downloaded VLC to my 'documents' folder (the default set in my preferences) - I
then selected 'Camino -> Reset Camino' - then when going to my Documents the
file was deleted.  This happened with other applications and images.  I have to
move the file out of that folder before I can reset Camino.  I haven't verified
that the same thing happens if I save it to my Desktop.

Reproducible: Always

Steps to Reproduce:
1. Download files
2. Choose to reset Camino
3. 

Actual Results:  
File is no longer there.

Expected Results:  
Not to remove downloaded files.

OSX 10.3.9

Comment 1

[Mike Pinkerton (not reading bugmail)]

this shouldn't be happening. we don't delete any download files.

Comment 2

[Sparky D Clown]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8b2) Gecko/20050705 Camino/0.9a1+
> Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US;
rv:1.8b2) Gecko/20050705 Camino/0.9a1+
> 
> When I download files (applications, images, etc) and then select the option to
> 'reset camino' my downloaded files will be (presumably) deleted.  For example; I
> downloaded VLC to my 'documents' folder (the default set in my preferences) - I
> then selected 'Camino -> Reset Camino' - then when going to my Documents the
> file was deleted.  This happened with other applications and images.  I have to
> move the file out of that folder before I can reset Camino.  I haven't verified
> that the same thing happens if I save it to my Desktop.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Download files
> 2. Choose to reset Camino
> 3. 
> 
> Actual Results:  
> File is no longer there.
> 
> Expected Results:  
> Not to remove downloaded files.
> 
> OSX 10.3.9


I have also downloaded 0.9a1 and I had the same experience. Download anything
anywhere and leave them in the default download folder. Reset Camino and the
reset deletes the downloaded files.
I took the insight of this tech to heart and told my girlfriend that my sack was
itching and she said the same thing "This shoudnt be happening" but she wasnt so
nice telling me that and I am now looking for a new home and job so the gist of
that is never say "This shouldnt be happening" and by the way my sack still
itches. You got any recommendations for that by chance Sparky?

Comment 3

[Torben]

I can reproduce this too, confirming and requesting blocking for 0.9.

Comment 4

[Mike Pinkerton (not reading bugmail)]

i suggest gold bond medicated itch powder.

looks like we need to look into this.

Comment 5

[Torben]

This only happens for files that are in the download manager (whether the window
is shown or not), the easy fix is probably to automatically do a "Remove all
inactive downloads" before the reset.

Comment 6

[Torben]

Attachment: Quick fix, remove all inactive downloads before the reset

As suggested in comment 5 (tested and working). Other possible fixes could be
to either only cancel active downloads here or check for mDownloadDone in the
ProgressViewController's cancel.

Comment 7

[Mike Pinkerton (not reading bugmail)]

as i told kreeger on irc, before we let this go in i'd like to know *why* this
workaround fixes the problem.

Comment 8

[Torben]

(In reply to comment #7)
> as i told kreeger on irc, before we let this go in i'd like to know *why* this
> workaround fixes the problem.

From ProgressDlgController.mm (line 193):

  // the ProgressViewController method "cancel:" has a sanity check, so its
     ok to call on anything

This sanity check only checks that the download is not already canceled, I guess
cancelling a finnished download also makes necko delete the file. As suggested
above, checking for mDownloadDone probably also works - I will check this at once.

Comment 9

[Josh Aas]

try replacing:
cacheServ->EvictEntries(nsICache::STORE_ANYWHERE);

with:
cacheServ->EvictEntries(nsICache::STORE_ON_DISK);
cacheServ->EvictEntries(nsICache::STORE_IN_MEMORY);

everywhere it's used. Using STORE_ON_DISK_AS_FILE, which is included in
STORE_ANYWHERE, may be the problem.

Comment 10

[Simon Fraser [no longer active]]

(In reply to comment #9)
> try replacing:
> cacheServ->EvictEntries(nsICache::STORE_ANYWHERE);
> 
> with:
> cacheServ->EvictEntries(nsICache::STORE_ON_DISK);
> cacheServ->EvictEntries(nsICache::STORE_IN_MEMORY);
> 
> everywhere it's used. Using STORE_ON_DISK_AS_FILE, which is included in
> STORE_ANYWHERE, may be the problem.

I think it's related to cancelling completed downloads, not cache eviction flags.

Comment 11

[Torben]

Attachment: Alternative patch, don't cancel finnished downloads

This works too. Only one of the patches (or none if Josh's idea is better, but
see below) should be used, please remove review request and obsolete the
unwanted patch(es). 

(In reply to comment #9)
> try replacing:
> cacheServ->EvictEntries(nsICache::STORE_ANYWHERE);
> 
> with:
> cacheServ->EvictEntries(nsICache::STORE_ON_DISK);
> cacheServ->EvictEntries(nsICache::STORE_IN_MEMORY);
> 
> everywhere it's used. Using STORE_ON_DISK_AS_FILE, which is included in
> STORE_ANYWHERE, may be the problem.

Wouldn't this also mean that the temporary (.part) file isn't deleted?

Comment 12

[Josh Aas]

About the cache eviction flags, I was just throwing that out there for
consideration.

Comment 13

[Torben]

Comment on attachment 189190 [details] [diff] [review]
Quick fix, remove all inactive downloads before the reset

Forget about this one, the other patch is the right thing to do.

AFAICT tell this is what happens: (ProgressDlgController) clearAllDownloads
calls (ProgressViewController) cancel which calls
nsDownloadListener::CancelDownload() which again calls
nsDownloadListener::DownloadDone(NS_BINDING_ABORTED). When DownloadDone is
called with an error result the downloaded file _will_ be deleted
<URL:http://lxr.mozilla.org/mozilla/source/camino/src/download/nsDownloadListen
er.mm#351>.

So we have two possible solutions: either we check for active downloads in
(ProgressDlgController) clearAllDownloads and only cancel these, or we never
allow canceling of finnished downloads (as in attachment #189206 [details] [diff] [review]). I can't
think of any situation where cancelling a finnished download makes sense, in
the download window the button is greyed out, so IMHO the last solution is the
coorect one (_if_ we ever want to be able to delete files directly from the
download manager we should create a function for this and not depend of some
strange behaviour from the cancel function).

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #13)
> (_if_ we ever want to be able to delete files directly from the
> download manager we should create a function for this and not depend of some
> strange behaviour from the cancel function).

The decision seemed to be only moving them to the trash, see patches/discussion
in bug 230320.

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 189206 [details] [diff] [review]
Alternative patch, don't cancel finnished downloads

sr=pink. i prefer this as well.

Comment 16

[Simon Fraser [no longer active]]

Patch checked in.

Comment 17

[Torben]

Removing blocking request since this is fixed.