Last Comment Bug 408605 - Preference to control cross-session download behaviour
: Preference to control cross-session download behaviour
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Download Manager (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.9beta3
Assigned To: Graeme McCutcheon [:graememcc]
:
:
Mentors:
Depends on: 939512
Blocks:
  Show dependency treegraph
 
Reported: 2007-12-16 14:05 PST by Graeme McCutcheon [:graememcc]
Modified: 2013-11-28 01:03 PST (History)
10 users (show)
mconnor: blocking1.9-
sdwilsh: in‑testsuite-
sdwilsh: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 - Adds about:config preference (7.03 KB, patch)
2008-01-03 20:24 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
Patch v2 - fixes a couple of nits, better commented (7.54 KB, patch)
2008-01-04 16:35 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
Patch v3 - unbitrotted (7.47 KB, patch)
2008-01-22 08:57 PST, Graeme McCutcheon [:graememcc]
sdwilsh: review-
Details | Diff | Splinter Review
Patch v4 - new patch for review (7.59 KB, patch)
2008-01-22 15:52 PST, Graeme McCutcheon [:graememcc]
no flags Details | Diff | Splinter Review
Patch v4a - new patch for review (7.59 KB, patch)
2008-01-22 16:16 PST, Graeme McCutcheon [:graememcc]
sdwilsh: review-
Details | Diff | Splinter Review
Patch v5 - tri-state preference (7.71 KB, patch)
2008-01-25 14:33 PST, Graeme McCutcheon [:graememcc]
sdwilsh: review-
Details | Diff | Splinter Review
Patch v6 - tri-state patch 2nd attempt (8.26 KB, patch)
2008-01-25 16:27 PST, Graeme McCutcheon [:graememcc]
sdwilsh: review+
Details | Diff | Splinter Review
Patch v7 - final version for check-in (8.19 KB, patch)
2008-01-26 08:54 PST, Graeme McCutcheon [:graememcc]
graeme.mccutcheon: review+
mtschrep: approval1.9+
Details | Diff | Splinter Review

Description Graeme McCutcheon [:graememcc] 2007-12-16 14:05:16 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121605 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2007121605 Minefield/3.0b3pre

As per bug 408555, although cross-session download resumption has been implemented, there is no way to control this behaviour, and any active/paused downloads will automatically resume next time Minefield is opened. For some users, this may pose a privacy risk. 

Bug 256620, comment 1 suggested one possible way of implementing this, although if the user has multiple tabs open which trigger the save tabs dialog, this may cause user fatigue. An option could also be provided in either Tools->Options->Main under downloads, or Tools-Options->Privacy, perhaps as part of the "Clear private data on exit" options.


Reproducible: Always

Steps to Reproduce:
1. Start downloading a file that would cause you personal or professional embarrassment if anyone else ever saw that you had downloaded it. 
2. Exit and restart Firefox.
3. Download resumes.
Actual Results:  
On restart the download resumes. Embarrassment ensues.

Expected Results:  
The download should not resume if user wishes for unfinished downloads to cancel at end of browsing sessions.
Comment 1 James Darpinian 2007-12-16 16:12:22 PST
Bug 401172 is taking care of notifying the user on exit, so that behavior should be discussed there instead.  

I think it is pretty clear that this belongs in the "clear private data" feature, perhaps as a checkbox labeled "Paused Downloads" (or "Active Downloads", and it could then cancel *all* downloads, which I think is appropriate).
Comment 2 Shawn Wilsher :sdwilsh 2007-12-16 16:19:32 PST
I agree - this is not a DM bug.  Not sure of the right component for Privacy...
Comment 3 Stephen Donner [:stephend] 2007-12-16 16:53:19 PST
(In reply to comment #0)

> As per bug 408555, although cross-session download resumption has been
> implemented, there is no way to control this behaviour, and any active/paused
> downloads will automatically resume next time Minefield is opened. For some
> users, this may pose a privacy risk. 

This is not a true assertion; paused downloads do not auto-resume upon app restart; if they do, that's a bug (I just tested, and this is still true).
Comment 4 Graeme McCutcheon [:graememcc] 2007-12-17 03:58:08 PST
(In reply to comment #3)

> This is not a true assertion; paused downloads do not auto-resume upon app
> restart; if they do, that's a bug (I just tested, and this is still true).
> 

Correct, I wasn't thinking when I typed that. Paused downloads will persist in the download manager between sessions, allowing the download to be resumed in a later browsing session, which may be a privacy issue for some users.
Comment 5 Mike Connor [:mconnor] 2007-12-17 18:24:24 PST
I don't think this is a blocker.  It makes some sense to have at least a hidden pref for internet cafes and the like, but we persist cancelled downloads across sessions as well, so the info on the download is still there even if we don't resume.  What you want is a global pref for remembering downloads, period, this is not on its own some sort of new potential local privacy leak.

If the user is embarassed, they should cancel and remove it from the list.
Comment 6 James Darpinian 2007-12-18 01:02:37 PST
(In reply to comment #5)
> we persist cancelled downloads across sessions as well, so the info on the 
> download is still there even if we don't resume

Not if you are using the "Clear Private Data" feature on exit.  This is really a bug in the "Clear Private Data" feature.

> this is not on its own some sort of new potential local privacy leak.

It is, when the user has Clear Private Data on exit enabled with all the boxes checked.  Firefox 2 remembered nothing about the previous session.  Firefox 3 now saves *new* private data between sessions, namely paused downloads.  Without the ability to erase this new private data, the Clear Private Data feature is incomplete and therefore misleading.
Comment 7 Graeme McCutcheon [:graememcc] 2007-12-20 16:25:08 PST
(In reply to comment #5)
> What you want is a global pref for remembering downloads, period, this
> is not on its own some sort of new potential local privacy leak.
> 

This makes sense, and would probably make sense to have it located in Clear Private Data.

A user who does not want any in progress/paused downloads to be remembered in the next session would just need to select the "downloads" option in the clear private data dialog and select "clear private data when I close Firefox" in options. The sanitizer code appears to be called after observerService has sent quit-application notifications to its observers - in particular, this means that download manager will have paused all the resumable downloads by the time the sanitizer runs. The sanitizer can then simply query the database for all downloads whose status is DOWNLOAD_PAUSED, and cancel/remove them.

This approach brings up a few points:
  
- What text would we use to describe this option, to make it clear to the user that this performs more than FF2 "clear download history" functionality

- When the option is selected from the clear private data dialog in a non-shutdown situation, it may take users by surprise when paused downloads disappear, as this will be very different from FF2 behaviour...

- ...unless we opted not to remove paused downloads if the application is not quitting. However, having one control with two different possible behaviours is probably unwholesome and confusing

- This would eliminate some of the possibilities being discussed in bug 401172. If the user has set the prompt on shutdown feature for clear private data, the dialog appears after quit dialogs have been issued. Thus, quit dialogs would not  be able to make any claims as to whether or not downloads will be resumed, as that will be dictated by the user's responses in clear private data.



Comment 8 Graeme McCutcheon [:graememcc] 2008-01-03 20:24:04 PST
Created attachment 295328 [details] [diff] [review]
Patch v1 - Adds about:config preference

This patch performs the following:

- Adds a preference browser.download.manager.rememberActiveDownloads with default value of true to control cross-session download behaviour

- Adds backend code to remove active/paused downloads on exit if this pref is set to false

I haven't exposed the pref in the UI anywhere apart from about:config, as I think more debate is needed on the right place, if any.

A user who wants all downloads to be cleared on exit can set this pref to false, and either set privacy.item.downloads to true, or set browser.download.manager.retention to 1
Comment 9 Graeme McCutcheon [:graememcc] 2008-01-03 20:25:12 PST
Sorry for bugspam. Changing to what is, I think, correct component.

Does someone with editbugs want to assign this to me?
Comment 10 Graeme McCutcheon [:graememcc] 2008-01-04 16:35:14 PST
Created attachment 295452 [details] [diff] [review]
Patch v2 - fixes a couple of nits, better commented

This patch fixes a couple of nits I spotted (whitespace/indents etc)

sdwilsh: After I reassigned to FF->Preferences, reed has reassigned back to DM, so we are back at the component I originally filed it against! I threw away the approach I was talking about in the email, realising that I wouldn't be able to access all the info I needed through the Download Manager's API, and in any case was simpler and more sensible.
Comment 11 Shawn Wilsher :sdwilsh 2008-01-06 10:25:35 PST
mconnor - is this at least wanted-firefox3?
Comment 12 Shawn Wilsher :sdwilsh 2008-01-21 22:26:08 PST
I keep forgetting to ask mconnor this when I have time - should be be shifting to a toolkit.download.manager.* preference system when we can?  the DM isn't exactly browser specific...
Comment 13 Graeme McCutcheon [:graememcc] 2008-01-22 08:57:09 PST
Created attachment 298479 [details] [diff] [review]
Patch v3 - unbitrotted

Minor update to original patch now that bug 410289 has landed.

Will wait for mconnor's answer on the pref branch, but for the moment have given it a slightly more meaningful name.

Incidentally, this should help with bug 401172 - as it gives the conditions for displaying the quit dialog with its current strings - if persistActiveDownloads pref is false or there are unresumable downloads, downloads will indeed be cancelled on quit.
Comment 14 Mike Connor [:mconnor] 2008-01-22 12:43:50 PST
I think that toolkit.* is the right thing, but let's punt until next cycle, its a touch late to be making changes that affect toolkit consumers in a non-trivial way...
Comment 15 Shawn Wilsher :sdwilsh 2008-01-22 12:59:14 PST
Comment on attachment 298479 [details] [diff] [review]
Patch v3 - unbitrotted

>-    if (dl->IsPaused())
>+    if (GetCrossSessionBehavior() && dl->IsPaused())
Switch the order here - we don't need to check the pref if the download isn't paused (pause check is cheaper than the cross session behavior check).

>+PRBool
>+nsDownloadManager::GetCrossSessionBehavior()
>+{
>+  // We use true as the default, which is "remember and resume the download"
no you don't - I think you want NS_ENSURE_SUCCESS(rv, PR_TRUE)

>+  nsresult rv;
>+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, 0);
>+
>+  PRBool val;
>+  rv = pref->GetBoolPref(PREF_BDM_CROSSSESSIONRETENTION, &val);
>+  NS_ENSURE_SUCCESS(rv, 0);
>+
>+  return val;
>+}

>+  PRBool GetCrossSessionBehavior();
javadoc style comment
Let's also call it something different that explains what it is a bit better.  How about |PRBool ResumeActiveDownloads();|
Comment 16 Graeme McCutcheon [:graememcc] 2008-01-22 15:52:55 PST
Created attachment 298566 [details] [diff] [review]
Patch v4 - new patch for review

This new patch incorporates points from comment# 15.

Also, I've removed:
+      // Remove download if user does not want it persisting across sessions
+    if (!GetCrossSessionBehavior())
+      result = RemoveDownload(dl->mID);

so that setting the pref to false now cancels the download, but does not remove it completely - I think this still satisifies the privacy issue, and allows for another use case. Any user who wants their download history cleared on exit will already have clear private data set to do this, so we only need to cancel the download, and clear private data will mop up behind us. In addition, this allows for users who do not want downloads cleared, but who don't want them resuming automatically either.

Of course, in this case they will still lose their progress. In fact, there's a  case for making the pref a tri-value pref to allow for the following behaviours:

 * Continue downloads next session (current behaviour)
 * Pause incomplete downloads, do not auto-resume
 * Cancel incomplete downloads

sdwilsh, would this be better? If so, feel free to deny r on this patch, and I'll whip up the above.
Comment 17 Graeme McCutcheon [:graememcc] 2008-01-22 16:16:47 PST
Created attachment 298582 [details] [diff] [review]
Patch v4a - new patch for review

Sorry for bugspam. Found a nit in the above.
Comment 18 Shawn Wilsher :sdwilsh 2008-01-25 10:42:19 PST
Comment on attachment 298582 [details] [diff] [review]
Patch v4a - new patch for review

Actually, yes, I think tri-state is better. We'll need a better name for the pref though.
Comment 19 Ed Lee :Mardak 2008-01-25 12:18:27 PST
browser.download.manager.quitBehavior sounds good.

I haven't been following the bug.. but how will this interact with the download dialog for bug 401172? The actions there lets the user "quit and autoresume" or "quit and no-autoresume". Would the "do not ask next time" make use of this preference?

quad-state:
1) ask (default)
2) pause and autoresume
3) pause and don't autoresume
4) cancel

The dialog in bug 401172 only allows selecting options 1-3 from the quit dialog UI.
Comment 20 Graeme McCutcheon [:graememcc] 2008-01-25 12:44:22 PST
browser.download.manager.quitBehavior it is then.

> quad-state:
> 1) ask (default)
> 2) pause and autoresume
> 3) pause and don't autoresume
> 4) cancel

Or tri-state, but with a seperate pref analagous to browser.tabs.warnOnClose to control the ask behavior? This would keep seperate the behavior from the UI controlling it.


Comment 21 Shawn Wilsher :sdwilsh 2008-01-25 14:13:28 PST
(In reply to comment #19)
> I haven't been following the bug.. but how will this interact with the download
> dialog for bug 401172? The actions there lets the user "quit and autoresume" or
> "quit and no-autoresume". Would the "do not ask next time" make use of this
> preference?
I'm not sure that's going to make firefox 3 (I was talking to mconnor today about dropping it).
Comment 22 Graeme McCutcheon [:graememcc] 2008-01-25 14:33:18 PST
Created attachment 299298 [details] [diff] [review]
Patch v5 - tri-state preference

This patch adds the preference browser.download.manager.quitBehavior

The meaning of the value of this preference is as follows:
0 - incomplete downloads will auto-resume next session (unless they are paused by user) [DEFAULT]

1 - incomplete downloads will pause on exit, and can be resumed manually

2 - incomplete downloads will be cancelled on exit

Any other values will be treated as case 0.

This will support use cases such as those concerned about their privacy (in conjunction with their existing clear private data settings). It will also help users who switch between unmetered and metered internet access, and don't want downloads restarting automatically when they are paying by the Mb, but equally don't want their progress to be lost, so they can continue them when back on an unmetered connection.
Comment 23 Shawn Wilsher :sdwilsh 2008-01-25 14:51:38 PST
Comment on attachment 299298 [details] [diff] [review]
Patch v5 - tri-state preference

>+PRInt32
>+nsDownloadManager::GetQuitBehavior()
>+{
>+  // We use 0 as the default, which is "remember and resume the download"
>+  nsresult rv;
>+  nsCOMPtr<nsIPrefBranch> pref = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, 0);
>+
>+  PRBool val;
>+  rv = pref->GetIntPref(PREF_BDM_QUITBEHAVIOR, &val);
>+  NS_ENSURE_SUCCESS(rv, 0);
>+
>+  return val;
>+}
A few things:
1) This doesn't handle the case where it is set to an invalid value (I suggest using a switch statement)
2) Let's make an enum representing the states here

>+  /**
>+   * Indicates behavior for active downloads across sessions,
>+   *
>+   * @return value of user-set pref for active download behavior
>+   *         Valid values are:
>+   *          0 - downloads should be auto-resumed,
>+   *          1 - downloads should be paused
>+   *          2 - downloads should be cancelled
>+   */
>+  PRInt32 GetQuitBehavior();
Let's have the enum values here, and let's have it look something like this:
enum QuitBehavior { QUIT_AND_RESUME = 0, QUIT_AND_PAUSE = 1, QUIT_AND_CANCEL = 2};
We should also return the enum, not an int.
Comment 24 Graeme McCutcheon [:graememcc] 2008-01-25 16:27:12 PST
Created attachment 299331 [details] [diff] [review]
Patch v6 - tri-state patch 2nd attempt

Per comments in comment# 23.

A bug should probably be filed to fix up GetRetentionBehavior() in a similar manner, as it also has 3 states, and like my earlier version, does not explicitly catch invalid values.
Comment 25 Shawn Wilsher :sdwilsh 2008-01-25 20:54:43 PST
Comment on attachment 299331 [details] [diff] [review]
Patch v6 - tri-state patch 2nd attempt

>+    if (dl->IsPaused() && GetQuitBehavior() != QUIT_AND_CANCEL)
nit: if (dl->IsPaused() && QUIT_AND_CANCEL != GetQuitBehavior())

>+nsDownloadManager::QuitBehavior
>+nsDownloadManager::GetQuitBehavior()
nit: enum nsDownloadManager::QuitBehavior
I have a strange style regarding enums, I know...

>+  QuitBehavior result;
>+  
>+  switch (val) {
>+    case 1: 
>+      result = QUIT_AND_PAUSE;
>+      break;
>+    case 2:
>+      result = QUIT_AND_CANCEL;
>+      break;
>+    default:
>+      result = QUIT_AND_RESUME;
>+  }
>+
>+  return result;
just return the results in the switch statement - no need to store it until the end.

>+    // Try to pause all downloads and, if appropriate, mark them as auto-resume
>+    // unless user has specified that downloads should be canceled
>+    enum QuitBehavior behavior = GetQuitBehavior();
>+    if (behavior != QUIT_AND_CANCEL)
nit: if (QUIT_AND_CANCEL != behavior)

>+      (void)PauseAllDownloads(PRBool(behavior != QUIT_AND_PAUSE));
nit: swap behavior and QUIT_AND_PAUSE

>+   *  QUIT_AND_RESUME  - downloads should be auto-resumed,
>+   *  QUIT_AND_PAUSE   - downloads should be paused
>+   *  QUIT_AND_CANCEL  - downloads should be cancelled
nit: no comma at the end

>+  /**
>+   * Indicates user-set behavior for active downloads across sessions,
>+   *
>+   * @return value of user-set pref for active download behavior
>+   */
>+  QuitBehavior GetQuitBehavior();
nit: enum QuitBehavior

Looks great otherwise!

r=sdwilsh with comments addressed.
Comment 26 Graeme McCutcheon [:graememcc] 2008-01-26 08:54:55 PST
Created attachment 299425 [details] [diff] [review]
Patch v7 - final version for check-in

Addresses comments in comment 25. Carrying forward r=sdwilsh.
Comment 27 Shawn Wilsher :sdwilsh 2008-01-26 13:13:08 PST
After arguing about it in #developers, I changed my mind about the nit's on the ordering of the boolean conditions.  I fixed that before checkin.

Thanks for the patch!

Checking in browser/app/profile/firefox.js;
new revision: 1.257; previous revision: 1.256
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.156; previous revision: 1.155
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.61; previous revision: 1.60
Comment 28 Jean Jordaan 2009-08-29 01:55:50 PDT
This isn't only a privacy issue. If you were downloading something that requires login, or that times out, and download resumes, then the incomplete download (say 45MB) is stomped by an HTML "login required" page (say 1.8KB). This happens as a result of hibernate/wake up as well -- so I'm not sure whether cross-session applies. I'm changing it to 2 now, so I'll find out ..

Note You need to log in before you can comment on or make changes to this bug.