Open Bug 435799 Opened 14 years ago Updated 1 year ago

download does not resume where it left off if firefox crashes

Categories

(Toolkit :: Downloads API, defect)

1.9.0 Branch
defect
Not set
normal

Tracking

()

People

(Reporter: jbecerra, Unassigned)

References

()

Details

(Whiteboard: [notacrash])

Attachments

(1 file)

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.
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.
Component: Session Restore → Download Manager
OS: Mac OS X → All
QA Contact: session.restore → download.manager
Hardware: Macintosh → All
Product: Firefox → Toolkit
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 #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 :(
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.
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).
Could you make a unit test too so we don't break this in the future?
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
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.
Whiteboard: [notacrash]
Duplicate of this bug: 506648
Duplicate of this bug: 569343
Duplicate of this bug: 569208
Duplicate of this bug: 576273
Do you still see this issue?
Flags: needinfo?(timwi)
Flags: needinfo?(deepak.vasudevan)

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

Flags: needinfo?(timwi)
Flags: needinfo?(deepak.vasudevan)
You need to log in before you can comment on or make changes to this bug.