435799 - download does not resume where it left off if firefox crashes

Status: NEW

(Toolkit :: Downloads API, defect)

Description

[juan becerra [:juanb]]

Fx3rc1, Mac OS X 10.4.11, Litmus test case 4992. If you simulate a crash by force quitting Firefox while it is downloading a file, the download will not resume where it left off after restarting the browser.

Steps:
1. Install Fx3rc1 and start to download a big file, like the ubuntu installer
2. Using the Activity Monitor or cmd-option-esc force quit Firefox while the download is in progress
3. Restart firefox and opt to restore session when prompted

Expected: Download manager window should be open and the download should resume where it left off.

Actual: Download manager is closed and when you open it you can see the download is starting from zero.

Comment 1

[Simon Bünzli]

Happens on Windows as well. IMO this is no Session Restore bug, though, as we just tell the DM to get things going again.

(In reply to comment #0)
> Expected: Download manager window should be open

If you want that to happen, please file a different bug. Note that we already do tell the DM to resume its job - and it actually does so as well, just restarting instead of resuming active downloads.

On a further note: Paused downloads can be resumed after a crash without having to start over.

Comment 2

[ray]

Another issue is the following: Some failed Downloads are somehow wrong marked as 'finished' and so you can not resume or restart it because the menu does not show. This problem is old, and it still is in the recent versions.

How can i resume a failed dl which is at 0 bytes size and still regarded as finished download by firefox 3?

Comment 3

[Brian Ewins]

comment #2 - ray, that is bug 237623.

comment #1 - Simon, is it actually intended downloads should restart, not resume, after a crash? Any idea why its not done?

I had a look at the code, and it appears that the only reason downloads don't resume after a crash is that their state isn't set to 'Paused'. When you quit the browser, nsDownloadManager::PauseAllDownloads is called, which sets the auto-resume flag and then pauses every active download. When the browser resumes after a crash, nsDownloadManager::RestoreDatabaseState gets called, which sets all non-paused active downloads into an auto-resume state as well - but this always causes a restart, because nsDownload::Resume says:

  if (!IsPaused() || !IsResumable())
    return NS_ERROR_UNEXPECTED;

Since the crashed downloads weren't paused, it doesn't even attempt to resume them. Looks like a bug to me? It seems that one way of fixing this would be to add this in nsDownloadManager::RestoreActiveDownloads:

  // Try to resume only the downloads that should auto-resume
+ // autoresumable downloads that are active need to be paused first
+ rv = PauseAllDownloads(PR_FALSE);
  rv = ResumeAllDownloads(PR_FALSE);

alternatively, actually reset to a sane state in nsDownloadManager::ResetDatabaseState:

  // Convert supposedly-active downloads into downloads that should auto-resume
  rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
    "UPDATE moz_downloads "
-   "SET autoResume = ?1 "
-   "WHERE state = ?2 "
-     "OR state = ?3 "
-     "OR state = ?4"), getter_AddRefs(stmt));
+   "SET autoResume = ?1, state = ?2 "
+   "WHERE state = ?3 "
+     "OR state = ?4 "
+     "OR state = ?5"), getter_AddRefs(stmt));
  NS_ENSURE_SUCCESS(rv, rv);

  i = 0;
  rv = stmt->BindInt32Parameter(i++, nsDownload::AUTO_RESUME);
  NS_ENSURE_SUCCESS(rv, rv);
+ rv = stmt->BindInt32Parameter(i++, nsIDownloadManager::DOWNLOAD_PAUSED);
+ NS_ENSURE_SUCCESS(rv, rv);
  rv = stmt->BindInt32Parameter(i++, nsIDownloadManager::DOWNLOAD_NOTSTARTED);
  NS_ENSURE_SUCCESS(rv, rv);
  rv = stmt->BindInt32Parameter(i++, nsIDownloadManager::DOWNLOAD_QUEUED);
  NS_ENSURE_SUCCESS(rv, rv);
  rv = stmt->BindInt32Parameter(i++, nsIDownloadManager::DOWNLOAD_DOWNLOADING);
  NS_ENSURE_SUCCESS(rv, rv);

  rv = stmt->Execute();
  NS_ENSURE_SUCCESS(rv, rv);

... which ought to act pretty much as if PauseAllDownloads had been called just before the crash. Not tried this yet, ymmv; interested because a 6-hour download just restarted on me after 4 :(

Comment 4

[Brian Ewins]

Ah, found the bug explaining the problem, bug 410289. It seems that at the very least my second suggestion won't work (since you can't pause queued downloads). Still not sure if the first suggestion (calling PauseAllDownloads) is ok - at least it goes through all the checks to see if the download can be paused before pausing and resuming.

Comment 5

[Brian Ewins]

Attachment: change 'downloading' to 'paused+autoresume' when resetting db state

the patch fixes the problem by resetting the db state for 'downloading' downloads to 'paused' with the autoresume flag set. This sends the download through the usual autoresume checks, which will fall back to the old 'restart' behaviour if they fail. Tested on Leopard, seems to restart the download ok both after a normal quit and a force quit (to simulate a crash).

Comment 6

[Shawn Wilsher :sdwilsh]

Could you make a unit test too so we don't break this in the future?

Comment 7

[Brian Ewins]

I'll have a go - not tried this before. Looks like I can just adapt this one?
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/test/unit/test_bug_401582.js

Comment 8

[Shawn Wilsher :sdwilsh]

yeah, it'll be a lot like that, but you will likely have to generate that database in the test since file paths are treated differently on each OS.  GetDownloadFromDB might fail, but I'm not sure.

Comment 13

[Wayne Mery (:wsmwk)]

Do you still see this issue?

Comment 14

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:mak, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 16

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.