DM should restore the database state gracefully after a crash

VERIFIED FIXED in mozilla1.9beta1

Status

()

Toolkit
Downloads API
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: sdwilsh, Assigned: Mardak)

Tracking

(Depends on: 1 bug)

Trunk
mozilla1.9beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

11 years ago
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.

Comment 1

11 years ago
What happens if a download can't be successfully restarted? Will it automatically change to DOWNLOAD_FAILED?
(Reporter)

Comment 2

11 years ago
Yes, as far as I can tell with the code, the nsIWebBrowserPersist will handle this gracefully with nsDownload.
(Reporter)

Updated

11 years ago
Flags: blocking-firefox3?
(Reporter)

Comment 3

11 years ago
Created attachment 268878 [details] [diff] [review]
v0.1

Not yet done, but until I get some concrete decisions as to when this should run, this is all I can do.
(Reporter)

Updated

11 years ago
Target Milestone: --- → Firefox 3 beta1
(Reporter)

Updated

11 years ago
Depends on: 386605
Flags: blocking-firefox3? → blocking-firefox3+

Updated

11 years ago
Target Milestone: Firefox 3 M7 → Firefox 3 M8

Updated

11 years ago
Blocks: 382513
(Reporter)

Updated

11 years ago
Duplicate of this bug: 391328
(Reporter)

Updated

11 years ago
Duplicate of this bug: 391998
(Reporter)

Comment 6

11 years ago
Created attachment 277181 [details] [diff] [review]
v1.0

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)
(Reporter)

Updated

11 years ago
Duplicate of this bug: 392794

Updated

11 years ago
Whiteboard: [need review Mano]
(Reporter)

Comment 8

11 years ago
Created attachment 278417 [details] [diff] [review]
v1.1

Updated to trunk.
Attachment #277181 - Attachment is obsolete: true
Attachment #278417 - Flags: review?(mano)
Attachment #277181 - Flags: review?(mano)
(Reporter)

Comment 9

11 years ago
Created attachment 278418 [details] [diff] [review]
v1.2

Let's get all the files...
Attachment #278417 - Attachment is obsolete: true
Attachment #278418 - Flags: review?(mano)
Attachment #278417 - Flags: review?(mano)
(Reporter)

Updated

11 years ago
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
(Reporter)

Comment 12

11 years ago
(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.

Updated

11 years ago
Whiteboard: [patch has review][checkin needed?]
(Reporter)

Comment 13

11 years ago
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?]
(Reporter)

Comment 14

11 years ago
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
Last Resolved: 11 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 → ---
(Reporter)

Comment 16

11 years ago
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.
Created attachment 282609 [details] [diff] [review]
myPatch v0.1

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)
(Reporter)

Comment 20

11 years ago
Created attachment 282766 [details] [diff] [review]
v2.0

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)
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
(Reporter)

Comment 22

11 years ago
still targeting M9 - I just need to test this.
Target Milestone: Firefox 3 M10 → Firefox 3 M9

Comment 23

11 years ago
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.

(Reporter)

Comment 25

11 years ago
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?

Comment 26

11 years ago
(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.
(Reporter)

Comment 27

11 years ago
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!

Comment 29

11 years ago
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!

Comment 30

11 years ago
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.

Comment 31

11 years ago
Created attachment 283734 [details]
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?
(Reporter)

Comment 32

11 years ago
Hrm - can you show me what line 2288 is in your version of nsDownloadManager.cpp is please (with some context too!).
(Assignee)

Comment 33

11 years ago
If I had to guess, it's mUpdateDownloadStatement which doesn't get set until /after/ we do RestoreDatabaseState. ;)

Comment 34

11 years ago
Created attachment 283737 [details]
Some context

Here ya go...the line it crashes on is about midway down the page.
(Assignee)

Comment 35

11 years ago
Created attachment 283754 [details] [diff] [review]
v3

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)
(Reporter)

Comment 36

11 years ago
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-
(Assignee)

Comment 37

11 years ago
Created attachment 283847 [details] [diff] [review]
v4

Un-function-ify.
Attachment #283754 - Attachment is obsolete: true
Attachment #283847 - Flags: review?(comrade693+bmo)
(Reporter)

Comment 38

11 years ago
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?

Updated

11 years ago
Attachment #283847 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 39

11 years ago
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
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 40

11 years ago
Created attachment 283911 [details] [diff] [review]
as checked in
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+
(Reporter)

Comment 42

11 years ago
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.