Closed Bug 596370 Opened 10 years ago Closed 9 years ago

Don't delete files opened with helper apps on exit

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 9

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: dataloss, mobile)

Attachments

(3 files, 1 obsolete file)

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
Attached patch PatchSplinter Review
This pref just keeps the downloaded files around. It doesn't move them anywhere prettier (i.e. the dowloads directory).
Attachment #475226 - Flags: review?(mark.finkle)
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.
Attachment #475226 - Flags: review?(mark.finkle) → review-
Attached patch Don't use temp directories (obsolete) — Splinter Review
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.
Assignee: nobody → wjohnston
Duplicate of this bug: 597189
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 {}
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...
(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...
Attached patch No Temp dirs v2Splinter Review
Attachment #475539 - Attachment is obsolete: true
Attachment #476107 - Flags: review?(mark.finkle)
Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

Looks good to me. Adding Lassey too.
Attachment #476107 - Flags: review?(mark.finkle)
Attachment #476107 - Flags: review?(blassey.bugs)
Attachment #476107 - Flags: review+
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.
(In reply to comment #7)
> It's still unclear to me...
Can you elaborate on how it's unclear?
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.
tracking-fennec: --- → ?
Flags: in-testsuite?
Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

I still don't think this is the right thing to do.
Attachment #476107 - Flags: review?(blassey.bugs)
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).
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?
Created bug 600300.
UX guys - any thoughts on the core issue of deleting opened files or retaining them?
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.
tracking-fennec: ? → 2.0b2+
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.
(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.
Filed as bug 602535
tracking-fennec: 2.0b2+ → 2.0b3+
Comment on attachment 476107 [details] [diff] [review]
No Temp dirs v2

MAEMO?
Attachment #476107 - Flags: review+ → review-
tracking-fennec: 2.0b3+ → 2.0-
(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.
Attached patch FixSplinter Review
I think that patch was already pushed. This changes the MAEMO part.
Attachment #488365 - Flags: review?(doug.turner)
Comment on attachment 488365 [details] [diff] [review]
Fix

wes, are there tests for this?
Attachment #488365 - Flags: review?(doug.turner)
Attachment #488365 - Flags: review+
Attachment #488365 - Flags: approval2.0?
Attachment #488365 - Flags: approval2.0? → approval2.0+
(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...
Whiteboard: [approved-patches-landed]
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Actually the root of this bug was not fixed, so I am changing the resolve.
Resolution: FIXED → WONTFIX
For comparison, the stock Android browser never deletes downloaded files automatically.

See also http://forums.mozillazine.org/viewtopic.php?f=47&t=2135909
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 firefoxforandroid@mozilla.com 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
Status: RESOLVED → REOPENED
tracking-fennec: - → ?
Keywords: dataloss, mobile
Resolution: WONTFIX → ---
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.
Attachment #475226 - Flags: review- → review?(mark.finkle)
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 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.
Attachment #475226 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/250fa13ab0b6
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [approved-patches-landed] → [inbound]
Blocks: 643027
Duplicate of this bug: 643027
Blocks: 642996
http://hg.mozilla.org/mozilla-central/rev/250fa13ab0b6
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 9
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.
Attachment #475226 - Flags: approval-mozilla-aurora?
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?
(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."
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 on attachment 475226 [details] [diff] [review]
Patch

not approving, based on tarend feedback
Attachment #475226 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Status: RESOLVED → VERIFIED
Not tracking for Firefox 9, it's already fixed there.
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.