Closed Bug 384810 Opened 18 years ago Closed 18 years ago

DM should restore the database state gracefully after a crash

Categories

(Toolkit :: Downloads API, defect)

Product:
Toolkit
Component:
Downloads API
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9beta1

People

(Reporter: sdwilsh, Assigned: Mardak)

References

Details

Attachments

(3 files, 8 obsolete files)

Stack trace
2.24 KB, text/plain
Details
Some context
844 bytes, text/plain
Details
as checked in
2.55 KB, patch
Details | Diff | Splinter Review
Currently, Session Restore handles this, but if the user doesn't decide to restore the crash, we end up with downloads in the UI that are apparently downloading, but they get no progress updates (because it isn't downloading). Here's how I think we should fix this: 1) We need to initialize the download manager service at startup. 2) Remove the session restore code (see discussion in Bug 382513) 3) At startup (in the Init method), retry all downloads that have the state nsIDownloadManager::DOWNLOAD_DOWNLOADING. This should happen regardless if the user actually wanted to restore the session. When we get Bug 377243 working, we should try to resume first, then retry.
What happens if a download can't be successfully restarted? Will it automatically change to DOWNLOAD_FAILED?
Yes, as far as I can tell with the code, the nsIWebBrowserPersist will handle this gracefully with nsDownload.
Flags: blocking-firefox3?
Attached patch v0.1 (obsolete) — — Splinter Review
Not yet done, but until I get some concrete decisions as to when this should run, this is all I can do.
Target Milestone: --- → Firefox 3 beta1
Depends on: 386605
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Blocks: 382513
Attached patch v1.0 (obsolete) — — Splinter Review
Until we can figure out when the "right" time to run this is, this will have to do for now.
Attachment #268878 - Attachment is obsolete: true
Attachment #277181 - Flags: review?(mano)
Whiteboard: [need review Mano]
Attached patch v1.1 (obsolete) — — Splinter Review
Updated to trunk.
Attachment #277181 - Attachment is obsolete: true
Attachment #278417 - Flags: review?(mano)
Attachment #277181 - Flags: review?(mano)
Attached patch v1.2 (obsolete) — — Splinter Review
Let's get all the files...
Attachment #278417 - Attachment is obsolete: true
Attachment #278418 - Flags: review?(mano)
Attachment #278417 - Flags: review?(mano)
Comment on attachment 278418 [details] [diff] [review] v1.2 r=mano
Attachment #278418 - Flags: review?(mano) → review+
Whiteboard: [need review Mano]
Comment on attachment 278418 [details] [diff] [review] v1.2 ><HTML><HEAD><script xmlns="http://www.w3.org/1999/xhtml" src="chrome://skype_ff_toolbar_win/content/injection_graph_func.js" id="injection_graph_func" charset="utf-8"/></HEAD><BODY><PRE> >+ rv = stmt->BindInt32Parameter(0, nsIDownloadManager::DOWNLOAD_FAILED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(1, nsIDownloadManager::DOWNLOAD_NOTSTARTED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(2, nsIDownloadManager::DOWNLOAD_QUEUED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(3, nsIDownloadManager::DOWNLOAD_DOWNLOADING); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Finally, let's retry all of those downloads >+ for (nsTArray<PRUint32>::size_type i = 0; i < ids.Length(); i++) >+ (void)RetryDownload(ids[i]); >+ >+ return NS_OK; >+} >+ For downloads of the type DOWNLOAD_DOWNLOADING shouldn't we try resuming first, instead of directly going for Retry? Only then we can have something like Cross-Session resumable downloads. And if the resume failed then we should probably inform the user before going for a Retry. (May be use the nsIAlertService for a sliding notification with clickable text). From the discussion at bug 377243 we should notify the user on auto-restart also. Here it is: https://bugzilla.mozilla.org/show_bug.cgi?id=377243#c26
(In reply to comment #11) > For downloads of the type DOWNLOAD_DOWNLOADING shouldn't we try resuming first, > instead of directly going for Retry? Only then we can have something like > Cross-Session resumable downloads. > > And if the resume failed then we should probably inform the user before going > for a Retry. (May be use the nsIAlertService for a sliding notification with > clickable text). From the discussion at bug 377243 we should notify the user on > auto-restart also. Here it is: > https://bugzilla.mozilla.org/show_bug.cgi?id=377243#c26 Sure, but we don't have support landed for resumable downloads yet, so we couldn't do that here.
Whiteboard: [patch has review][checkin needed?]
I was having issues with the tests this morning but a clean build cleared the issues up. This is ready to land.
Flags: in-testsuite?
Flags: in-litmus?
Keywords: checkin-needed
Whiteboard: [patch has review][checkin needed?]
Checking in browser/components/sessionstore/src/nsSessionStore.js; new revision: 1.74; previous revision: 1.73 Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; new revision: 1.111; previous revision: 1.110 Checking in toolkit/components/downloads/src/nsDownloadManager.h; new revision: 1.37; previous revision: 1.36
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Shawn asked me to reopen this, and I'm not seeing it working properly either. Copying comments over from bug 382513: I'm reopening; we've have several reports of it not working on our Testday (which featured Download Manager), and I just tried the steps in comment 0 with the first ISO file on http://centos.mirror.nac.net/4.5/isos/i386/, using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090415 Minefield/3.0a8pre. When I force quit, Restore Session, and open the Download Manager, there's no trace of the file.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
OK, so I am no longer getting the file sitting there in my download manager, but it seems as if it isn't actually retrying the download :/. Retargeting for M9...
Target Milestone: Firefox 3 M8 → Firefox 3 M9
http://litmus.mozilla.org/show_test.cgi?id=4642 Not convinced this testcase of mine is good; Shawn/Edward, can you please review it before I mark it in-litmus+ ? Thanks!
Comment on attachment 278418 [details] [diff] [review] v1.2 [snip] >+ // Third, change all of those downloads to a failed state so we will be able >+ // to retry them. >+ rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING( >+ "UPDATE moz_downloads " >+ "SET state = ?1 " >+ "WHERE state = ?2 " >+ "OR state = ?3 " >+ "OR state = ?4"), getter_AddRefs(stmt)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = stmt->BindInt32Parameter(0, nsIDownloadManager::DOWNLOAD_FAILED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(1, nsIDownloadManager::DOWNLOAD_NOTSTARTED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(2, nsIDownloadManager::DOWNLOAD_QUEUED); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = stmt->BindInt32Parameter(3, nsIDownloadManager::DOWNLOAD_DOWNLOADING); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // Finally, let's retry all of those downloads >+ for (nsTArray<PRUint32>::size_type i = 0; i < ids.Length(); i++) >+ (void)RetryDownload(ids[i]); >+ >+ return NS_OK; >+} Pretty surprising it has been overlooked. The SQL statement is actually never executed.
Attached patch myPatch v0.1 (obsolete) — — Splinter Review
Ok. As per my previous comment, the SQL statement was not being executed at all. I have added that -- considering that the execution was not left on purpose. Apart from that the patch has code for RealResuming() downloads which had the DOWNLOAD_DOWNLOADING state and had a proper entityID. The code and the comments has the rest of the explanation. I discussed with stephend as to what actually the problem was. I will try and test more tomorrow and make sure this thing actually works. Regards, Brahmana
Attachment #282609 - Flags: review?(comrade693+bmo)
Attached patch v2.0 (obsolete) — — Splinter Review
I'm much more inclined to get the original bug fixed, and then closed. Let's do a follow-up for the behavior change (I'm not sure if the behavior change is blocking, but the original issue certainly was).
Attachment #282609 - Attachment is obsolete: true
Attachment #282766 - Flags: review?(mano)
Attachment #282609 - Flags: review?(comrade693+bmo)
Attachment #282766 - Flags: review?(mano) → review+
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
still targeting M9 - I just need to test this.
Target Milestone: Firefox 3 M10 → Firefox 3 M9
I have had a "zombie" download stuck in my download manager for a few weeks now--would this solve that issue? As best as I can recall, this is what happened: 1. Downloaded large (several hundred MB) file 2. When it reached 100%, the download manager hung for several minutes. I could not switch to that window and the hard drive was grinding like crazy 3. Killed firefox process in task manager 4. Ever since, I have an "active" download of that file that cannot be paused or canceled. It shows an empty progress bar.
(In reply to comment #23) > I have had a "zombie" download stuck in my download manager for a few weeks > now--would this solve that issue? As best as I can recall, this is what > happened: > > 1. Downloaded large (several hundred MB) file > 2. When it reached 100%, the download manager hung for several minutes. I could > not switch to that window and the hard drive was grinding like crazy > 3. Killed firefox process in task manager > 4. Ever since, I have an "active" download of that file that cannot be paused > or canceled. It shows an empty progress bar. Yes, according to both Shawn and Edward in bug 398234. For the moment, I'm leaving that bug open since it's cleaner to test against.
I haven't had an opportunity to test this patch before I check it in (and now it needs approval). Would it help if we get a test build to see if it fixes the issue for you?
(In reply to comment #25) > I haven't had an opportunity to test this patch before I check it in (and now > it needs approval). Would it help if we get a test build to see if it fixes > the issue for you? > I have a build environment set up. I can do a build with this patch applied and see if that zombie entry goes away, if that will help you all out.
Yes, it would. I've been very busy with school, which is why I haven't tested it, so your help will be greatly appreciated!
I did a build this morning with this patch applied. I got the following results: 1. After beginning a download, opening the download manager crashed Firefox (wasn't a debug build, I don't have a stack for you) 2. After restarting Firefox, the corrupt download is gone from the download manager. I didn't think to backup my downloads.sqlite file beforehand, so I can't readily test this again. Doh!
I can upload my downloads.sqllite file if someone wants it to test the patch against. I won't delete the file until some patches land just to make sure they actually get deleted and the patches are working correctly.
Attached file Stack trace —
I was able to reproduce the crash in a debug build. I've attached the stack, to my untrained eye it looks like the "start time" on the download is null?
Hrm - can you show me what line 2288 is in your version of nsDownloadManager.cpp is please (with some context too!).
If I had to guess, it's mUpdateDownloadStatement which doesn't get set until /after/ we do RestoreDatabaseState. ;)
Attached file Some context —
Here ya go...the line it crashes on is about midway down the page.
Attached patch v3 (obsolete) — — Splinter Review
Explicitly do things from a PostInit... (p.s.... don't do an UPDATE moz_downloads SET state = 0 with a bunch of downloads then open the download manager...... You might get some mysterious files come from the grave from months ago ;))
Assignee: comrade693+bmo → edilee
Attachment #278418 - Attachment is obsolete: true
Attachment #282766 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #283754 - Flags: review?(comrade693+bmo)
Comment on attachment 283754 [details] [diff] [review] v3 I don't like the extra method. Just place a comment above them saying something like "OK, everything has been initialized, so we can do work if we have to now" or something worded better. Also, returning the result of that function violates the comment above that block of code (about how it needs to be the last thing called)
Attachment #283754 - Flags: review?(comrade693+bmo) → review-
Attached patch v4 (obsolete) — — Splinter Review
Un-function-ify.
Attachment #283754 - Attachment is obsolete: true
Attachment #283847 - Flags: review?(comrade693+bmo)
Comment on attachment 283847 [details] [diff] [review] v4 >+ // Do things *after* initializing various download manager properties such as >+ // restoring downloads to a consistent state >+ rv = RestoreDatabaseState(); >+ NS_ENSURE_SUCCESS(rv, rv); nit: newline after this please. >+ rv = RestoreActiveDownloads(); > NS_ENSURE_SUCCESS(rv, rv); r=sdwilsh. This patch is low risk (high gain too!).
Attachment #283847 - Flags: review?(comrade693+bmo)
Attachment #283847 - Flags: review+
Attachment #283847 - Flags: approval1.9?
Attachment #283847 - Flags: approval1.9? → approval1.9+
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v <-- nsDownloadManager.cpp new revision: 1.135; previous revision: 1.134 done Checking in toolkit/components/downloads/src/nsDownloadManager.h; /cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v <-- nsDownloadManager.h new revision: 1.51; previous revision: 1.50 done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago → 18 years ago
Resolution: --- → FIXED
Attached patch as checked in — — Splinter Review
Attachment #283847 - Attachment is obsolete: true
https://litmus.mozilla.org/show_test.cgi?id=4992 in-litmus+
Status: RESOLVED → VERIFIED
Flags: in-litmus? → in-litmus+
I do not see how this could be tested sanely.
Flags: in-testsuite? → in-testsuite-
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: