596370 - Don't delete files opened with helper apps on exit

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Fennec automatically opens any downloaded files it can. Then, when it closes, it deletes all of them. Since we're trying to keep users away from the file system, and we're the only lifeline to the file, we should keep it around until they intentionally delete it using the Downloads Manager.

See Bug 191239

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch

This pref just keeps the downloaded files around. It doesn't move them anywhere prettier (i.e. the dowloads directory).

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 475226 [details] [diff] [review]
Patch

Wes - I think we'd like to do more. I think we should not save files to the tmp folder at all.

I think we should add an #elif here for MAEMO:
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#476

and return the same folder we do here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1170

so for Maemo, we would save all files to $HOME/MyDocs/.documents/
for Android, we should also stop falling back to $tmp, so let's remove this:
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#471

and just return NS_ERROR_FAILURE if we have no sdcard.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Don't use temp directories

This resets download/temp directories on both platforms. This should be applied with the previous pref-patch on mobile-browser to ensure that the files aren't deleted when Fennec exits.

Haven't tested this yet, but it should work.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 475539 [details] [diff] [review]
Don't use temp directories

>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp

> #elif defined(ANDROID)
>+  // On mobile devices, we are avoiding exposing users to the file
>+  // system, and don't save downloads to temp directories
>+
>+  // On Android we only return something if we have and SD-card
>   char* sdcard = getenv("EXTERNAL_STORAGE");
>   nsresult rv;
>   if (sdcard) {
>     nsCOMPtr<nsILocalFile> ldir; 
>     rv = NS_NewNativeLocalFile(nsDependentCString(sdcard),
>                                PR_TRUE, getter_AddRefs(ldir));
>     NS_ENSURE_SUCCESS(rv, rv);

Can we append "downloads" here, like this code:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1187

Also, change "download" -> "downloads" in the above code

>     dir = ldir;
>     
>   }
>-  else {
>-    rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(dir));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-  }
>-  

>+  else
>+    return NS_ERROR_FAILURE;

wrap in {}

Comment 6

[Shawn Wilsher :sdwilsh]

This comment:
http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#411

Means that you need to keep the logic inline with what we do here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1225

Maybe we should update the comments in both locations to make this more clear...

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #6)
> This comment:
> http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#411
> 
> Means that you need to keep the logic inline with what we do here:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManager.cpp#1225
> 
> Maybe we should update the comments in both locations to make this more
> clear...

It's still unclear to me...

Comment 8

[Wesley Johnston (:wesj)]

Attachment: No Temp dirs v2

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

Looks good to me. Adding Lassey too.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

I think I disagree with the premise of this bug. It seems like we're just going to fill up the user's file system with things they've only looked at once. I guess is a fall out from the decision to open things everytime.

Perhaps it would be better to have UI for the user to say "keep this" after they've downloaded and viewed the file (a button or checkbox in the dl manager perhaps?). Expecting users to go into their download manager on a regular basis clean up their temporary downloads seems wrong.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #7)
> It's still unclear to me...
Can you elaborate on how it's unclear?

Comment 12

[Wesley Johnston (:wesj)]

Lassey, maybe we can bring this up this week for a UX or UI review/decision. I feel like its one of those guessing games that someone just has to make a decision on. Are people downloading so much from their phone that they would fill up SDCards? Is our download manager discoverable enough that they'll clean it up from time to time?

I'm not convinced people keep long lists of downloads on desktop, and "Remove" from out download manager now deletes files from the file system anyway. So maybe our natural tendency to clear out the list will help? On the other hand, I asked my wife this weekend and she didn't even know that the default Android browser had a download manager, let alone how to get to the files it had saved.

We could add a time limit to the cache for select items. If they've been in the list for more than a week/month and haven't been opened (through Fennec), delete them? Auto-delete but provide UI to re-download the items?

I think this is the simplest solution though, and it doesn't require a lot of explaining to users about what is going on, and there is almost zero risk of accidental data loss.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

I still don't think this is the right thing to do.

Comment 14

[Brian Crowder]

I agree with Brad:  We shouldn't clutter up the user's file-system covertly like this.  Let the user select to "Keep" files they want to have forever.  I have yet to download something while browsing on my phone that I intended to keep forever with the sole exception of .apk files (which are preserved by the installer, as I understand it).

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

There are two bugs here:
1. Make sure files can only be downloaded to /sdcard/downloads for both downloaders in Mozilla. If there is not a sdcard, we will not download.
2. Keep the download files until the user deletes them

#1 is the standard approach for Android. If the Android browser does it differently, than we should match them - but to my knowledge #1 is how they do it.
#2 also seem more inline with the standard Android approach and is more inline with our UX goals for Fennec. I do not believe we should keep files around forever, cleaning up should be something we do, or allow the user to configure Fennec to do.

But deleting a file as soon as possible seems like a sure way to cause the user grief.

For those following along at home, we are talking about files downloaded and opened when the user clicks a link - like PDF files and things. Files the user requests to save (Save As PDF or right-click Save Link) will be saved to the sdcard and not deleted until the user chooses to delete them.

I am perfectly happy with splitting this bug into two bugs, #1 and #2 respectively. Seems like #1 should be the new bug.

Wes, can you open a new bug for #1 and move the patch there?

Comment 16

[Wesley Johnston (:wesj)]

Created bug 600300.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

UX guys - any thoughts on the core issue of deleting opened files or retaining them?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment 15 seems to be rightthinking; we shouldn't nuke the file covertly. I don't think we want to try and get too clever here, though, with ways of having Fennec clean up.

The mobile browser isn't like the desktop browser. Users aren't going to be downloading a lot of files as they browse around. I think we should be more *overt* about the fact that we're downloading files that helper applications use, opening the download manager when those actions take place. This will help users understand that a file is being downloaded and retained on their system.

Comment 19

[Madhava Enros [:madhava]]

One minor point while I think about the bigger issue - we should change the button label in the download manager from "Remove" to "Delete."  Delete is more consistently associated with what the button actually does here (deletes the file from the filesystem);  "Remove" is a holdover from the desktop product, where the button just removes the line from the download manager.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #19)
> One minor point while I think about the bigger issue - we should change the
> button label in the download manager from "Remove" to "Delete."  Delete is more
> consistently associated with what the button actually does here (deletes the
> file from the filesystem);  "Remove" is a holdover from the desktop product,
> where the button just removes the line from the download manager.

Probably needs to be a separate bug.

Comment 21

[Wesley Johnston (:wesj)]

Filed as bug 602535

Comment 22

[Doug Turner (:dougt)]

Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

MAEMO?

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #22)
> Comment on attachment 476107 [details] [diff] [review]
> No Temp dirs v2
> 
> MAEMO?

Doug is referring to the preprocessor directive "MAEMO" used in the patch. We use MOZ_PLATFORM_MAEMO in platform code.

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Fix

I think that patch was already pushed. This changes the MAEMO part.

Comment 25

[Doug Turner (:dougt)]

Comment on attachment 488365 [details] [diff] [review]
Fix

wes, are there tests for this?

Comment 26

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/1a55741b7b27

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #26)
> http://hg.mozilla.org/mozilla-central/rev/1a55741b7b27

I just noticed that this should be on bug 600300. This bug is purely about not deleting the files downloaded via external helper app service

Oh, this patch also has the tree burning...

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Fix is on bug 600300

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

Actually the root of this bug was not fixed, so I am changing the resolve.

Comment 30

[Matt Brubeck (:mbrubeck)]

For comparison, the stock Android browser never deletes downloaded files automatically.

See also http://forums.mozillazine.org/viewtopic.php?f=47&t=2135909

Comment 31

[Matt Brubeck (:mbrubeck)]

I'm reopening and re-nominating this bug, since I've seen a few reports of it causing data loss for users, and I think the current user experience is quite bad and confusing.  Some examples, both from the past 4 days:

"Deletes files automatically after downloading it. Happened several times to me." -- email to [e-mail] on 2011-08-15

"downloaded 2 files and can see them in app as downloads but cannot find them using file manager root explorer" -- https://support.mozilla.com/en-US/questions/860719

Comment 32

[Matt Brubeck (:mbrubeck)]

Yet another one, from yesterday: https://support.mozilla.com/en-US/questions/861574

Comment 33

[Matt Brubeck (:mbrubeck)]

Comment on attachment 475226 [details] [diff] [review]
Patch

Re-requesting review on this patch, which just sets browser.helperApps.deleteTempFileOnExit to false.  The previous review comments from comment 15 were addressed by bug 600300.

The downside of this change is that users might unwittingly fill up their storage space with files that they only needed to view once.  However, this is a much more recoverable condition than the dataloss that users are currently reporting.

There may be a long-term way to free up space automatically *without* deleting files unexpectedly, but in the short term we need to stop doing this.

Comment 34

[Madhava Enros [:madhava]]

Yes - I agree. Keeping things that people have gone to the trouble and data/time expenditure to download seems the safer option. I don't want to ask people every time. Doing this would also let us get rid of the "Save Link" item from the context menu, because things would be saved anyway.

The contrary opinion has always been about filling up the SD card (or maybe non-SD storage? in which case this isn't as bad), but there are things we can do there that are less datalossy than deleting everything immediately. For example, downloads could fall off the bottom of the list after 6 months, the way history does (this just off the top of my head). Or we could only look at doing this if overall available storage went below a certain level.

Comment 35

[Matt Brubeck (:mbrubeck)]

Comment on attachment 475226 [details] [diff] [review]
Patch

Stealing review because Mark is on vacation and Madhava has approved the UX change.  Let's try this on nightly.  I'd like to request Aurora approval later if no bugs are found.

Comment 36

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/250fa13ab0b6

Comment 38

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/250fa13ab0b6

Comment 39

[Matt Brubeck (:mbrubeck)]

Comment on attachment 475226 [details] [diff] [review]
Patch

Requesting approval-mozilla-aurora.  This is not a new bug, but as Fennec's userbase grows it is showing up as a common support/feedback issue, and is causing dataloss.  The patch is a one-line, mobile-only preference change.  I think we should ship it to users sooner rather than later.

Comment 40

[Asa Dotzler [:asa]]

Thomas, can you chime in here on the reward? We can sort of discuss risk without you or a product person for Mobile, but it's hard to get the complete picture. How important is this change to you?

Comment 41

[Matt Brubeck (:mbrubeck)]

(In reply to Asa Dotzler [:asa] from comment #40)
> Thomas, can you chime in here on the reward? We can sort of discuss risk
> without you or a product person for Mobile, but it's hard to get the
> complete picture. How important is this change to you?

I'll answer for now because Thomas is out today.

The current situation is that, when Fennec downloads a file and then opens it with a helper app, the file is deleted when Fennec exits.  Some users do not expect this, and are surprised when they can't find their downloaded files later.  Comment 31 and comment 32 list some examples of recent support issues caused by this.

On the other hand, this bug has been around since before Fennec 4.0 and has not previously been such a common support issue, so perhaps this week was just an outlier.

The main reason I think this is an important fix is that it is a dataloss issue.  It feels very bad for user trust when users have to ask "Where is the file I wanted to look at?" and we have to tell them "Firefox deleted it without telling you."

Comment 42

[Thomas Arend [:tarend]]

I agree that this is important to fix, but not very urgent - despite the fact that is has been open since before Fennec 4.0. I have only received very few user reports about this. Based on our Aurora Meeting, I advise that we don't rush it into Aurora (Fx8) but instead target the fix for Fx9.

Comment 43

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 475226 [details] [diff] [review]
Patch

not approving, based on tarend feedback

Comment 44

[Cristian Nicolae (:xti)]

Saved files still exist on sdcard, even if Fennec is closed. I will mark this bug as verified fixed. 

--
Mozilla/5.0 (Android;Linux armv7l;rv:9.0a1)Gecko/20110913
Firefox/9.0a1 Fennec/9.0a1
Device: Samsung Galaxy S
OS: Android 2.2

Comment 45

[Christopher Blizzard (:blizzard)]

Not tracking for Firefox 9, it's already fixed there.