Closed Bug 382513 Opened 17 years ago Closed 17 years ago

Don't remove unsuccessfully retried downloads from the downloads list

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 3 alpha8

People

(Reporter: zeniko, Assigned: zeniko)

References

Details

Attachments

(1 file, 2 obsolete files)

for check-in
6.34 KB, patch
Details | Diff | Splinter Review 
Steps to reproduce:
1. Start downloading a larger file (which takes at least 10 seconds to complete)
2. Terminate Firefox' process (faking a crash)
3. Cut your network connection (or delete/rename the file)
4. Restart Firefox and resume the crashed session

Actual result:
There's no trace of the download in the Downloads Manager.

Implementation note:
Keeping the download item's id around (passing it to the AutoDownloader) until the download has actually been restarted and only then removing it from the DB would yield better results.

Sidenote: instead of savedToURI.path use

savedToURI instanceof Ci.nsIFileURL ? savedToURI.file.path : savedToURI.spec

(otherwise downloads will never be restored on Windows)...
Flags: blocking-firefox3?
Assignee: nobody → sdwilsh
So, just to clarify, this is windows only?  I do not get this behavior on my mac (I am working on getting my windows build setup).
Status: NEW → ASSIGNED
Attached patch fix (obsolete) — — Splinter Review 
This should actually happen on all platforms, I'm only able to test it on Windows though.

BTW: Just removing a download item from the DB isn't enough to make it disappear from the opened Downloads Manager. What am I missing?
Attachment #266719 - Flags: review?(sdwilsh)
There are some async issues with the database and the UI, which is what I'm fighting with and why I haven't uploaded a patch yet :)

Bug 381803 would probably take care of this.
Comment on attachment 266719 [details] [diff] [review]
fix

I am looking at this closer now, and we have to do something with the download (as in not leave it as it is).  In the past, with the RDF file, if the target and src were the same, it would overwrite that entry in the downloads.rdf file.  That just happened that way with rdf.  However, I don't do it that way now, so we have an issue.

Also, what happens if a user doesn't restore the session?  What happens to the download (I haven't tested this at all, just putting my thoughts on the bug)?

I think that marking the download as failed, and trying again would be the ideal solution here.
Attachment #266719 - Attachment is obsolete: true
Attachment #266719 - Flags: review?(sdwilsh)
(In reply to comment #4)
> I am looking at this closer now, and we have to do something with the download
> (as in not leave it as it is).

We don't, do we? We remove the download from moz_downloads once successfully restarted. The problem is that there seems no way to notify the DM that an item has been DELETE FROM'd (as in "there's no stored procedure dispatching a notification or something"). So the general question: how are extensions supposed to interact with moz_downloads while the DM is open?

> Also, what happens if a user doesn't restore the session?

We then also don't try to resume any downloads. They just sit there...

> I think that marking the download as failed, and trying again would be the
> ideal solution here.

The ideal solution would be to implement download resuming (cf. bug 377243) and then continue downloading automatically without SessionStore having to do anything.

What we could do so far is to set all apparently active downloads to a new state (DOWNLOAD_INTERRUPTED) when the download-manager component is initialized. Those downloads would display a "Retry" link in the DM and SessionStore could just invoke the same code path "Retry" does... (it has to be a new state so that we don't accidentally restart a download canceled/failed for any other reason).
Depends on: 382825
This will become immensely easier once Bug 382825 lands.  I think what we should do is mark any downloads that are DOWNLOAD_DOWNLOADING to DOWNLOAD_FAILED (regardless if a user restores their session or not), then just call nsIDownloadManager::retryDownload for all of them.  Let the download manager backed handle all the issues of file urls and what not.

Now, the question I need to figure out is how can I run code if a user decided to not restore their session?
(In reply to comment #5)

Sorry, I meant to address points in this comment in my last comment...

> We don't, do we? We remove the download from moz_downloads once successfully
> restarted. The problem is that there seems no way to notify the DM that an item
> has been DELETE FROM'd (as in "there's no stored procedure dispatching a
> notification or something"). So the general question: how are extensions
> supposed to interact with moz_downloads while the DM is open?

The retryDownload method that is going to be added in Bug 382825 will use an existing entry in the database.  This way nothing does get deleted anymore.  I think this addresses your concern, if not, please elaborate.

> The ideal solution would be to implement download resuming (cf. bug 377243) and
> then continue downloading automatically without SessionStore having to do
> anything.

Yes, but that hasn't landed yet, so we need something at least for now (and in case that doesn't happen).


> What we could do so far is to set all apparently active downloads to a new
> state (DOWNLOAD_INTERRUPTED) when the download-manager component is
> initialized. Those downloads would display a "Retry" link in the DM and
> SessionStore could just invoke the same code path "Retry" does... (it has to be
> a new state so that we don't accidentally restart a download canceled/failed
> for any other reason).

I think you might be right.  This may be the easiest way to implement this, and this could work out well with cross-session downloads.  I'd like to get a second opinion from biesi.

(we should probably change the description of this bug since it has morphed a bit)
(In reply to comment #6)
> mark any downloads that are DOWNLOAD_DOWNLOADING to DOWNLOAD_FAILED

Doesn't DOWNLOAD_FAILED mean that e.g. the transfer was interrupted due to a network failure? I wouldn't restore those but really only those which were still active at the point of the crash -- hence my suggestion for DOWNLOAD_INTERRUPTED.

> Now, the question I need to figure out is how can I run code if a user decided
> to not restore their session?

Should you not care about a session being restored or not, we could just as well move the whole out of SessionStore over to the download manager component (in that case you could even just restart all DOWNLOAD_DOWNLOADING items without the need for introducing a new state). As I said, I'd prefer automatic resuming to only happen when restoring though.
(In reply to comment #5)
> What we could do so far is to set all apparently active downloads to a new
> state (DOWNLOAD_INTERRUPTED) when the download-manager component is
> initialized. Those downloads would display a "Retry" link in the DM and
> SessionStore could just invoke the same code path "Retry" does... (it has to be
> a new state so that we don't accidentally restart a download canceled/failed
> for any other reason).

Couldn't you use DOWNLOADING, or whatever that state is called? The only way to have such downloads when the browser starts is when they were pending in a previous session, so you should resume them now.
(In reply to comment #9)
> Couldn't you use DOWNLOADING, or whatever that state is called? The only way to
> have such downloads when the browser starts is when they were pending in a
> previous session, so you should resume them now.

The problem comes in when the user doesn't want to restore the session.  We don't restart the downloads, so we have to do something with them.  Leaving them as nsIDownloadManager::DOWNLOAD_DOWNLOADING makes for a useless UI (it looks as if it is downloading, but it isn't really and none of the buttons will actually work).  This would be extremely difficult to work around in the UI, but adding the new state would make it trivial to work around.
Attached patch retry all available downloads (obsolete) — — Splinter Review 
So, things now boil down to going through the list of interrupted downloads, checking for the availability of their sources and retrying them.
Assignee: sdwilsh → zeniko
Attachment #268664 - Flags: review?(sdwilsh)
Comment on attachment 268664 [details] [diff] [review]
retry all available downloads

>+      linkChecker.asyncCheck(new AsyncDownloadRetrier(dlId), null);
I don't see this defined anywhere in the patch, or in the existing file.

Also, any reason why you aren't using the new api in the download manager to retry past downloads?
http://mxr.mozilla.org/mozilla/source/toolkit/components/downloads/public/nsIDownloadManager.idl#164
You're questions would be answered by grepping the patch for "function AsyncDownloadRetrier" and "this._dlManager.retryDownload(this._dlId);" and re-reading comment #11.

Anyway: what would happen if I called retryDownload directly when the download source isn't available (when either we or the server are offline)? We wouldn't want to annoy the user by retrying something which is doomed to fail from the start...
Wow - sorry.  I need to stop looking at patches right when I wake up...

I'm OK with this, but I'm thinking that maybe we should do the async checking in the retyDownload function so that all consumers of it can take advantage of that.

Would you be willing to do that in this bug, or do you want me to file a new bug and do that quickly?
and just to be clear, I still think we need to add a new state for instances where the user chooses to not restore a crashed session as per comment #10.  I was hoping biesi had something to say about it.
(In reply to comment #14)
> Wow - sorry.  I need to stop looking at patches right when I wake up...

Good morning, America! ;-)

> Would you be willing to do that in this bug, or do you want me to file a new
> bug and do that quickly?

I'd gladly leave that to you and another dependent bug since I currently haven't got a build environment handy.

(In reply to comment #15)
> I still think we need to add a new state for instances where the user
> chooses to not restore a crashed session as per comment #10.

If we introduce that new state, what about always retrying downloads right when you update the state (after the async availability check, of course)? Then we could move this completely out of SessionStore (where it's never really belonged but just landed as a hack from the time when SessionStore was an extension on its own)... Anyway: new bug, please. :-)
Comment on attachment 268664 [details] [diff] [review]
retry all available downloads

Actually, it looks like the retry download does fail gracefully if the URI isn't valid - it will try to start the download, and if it ends up failing, it'll end up as a failed download.

That's what the code looks like it'll do at least, but I haven't tested it.  If this is the case, I'd prefer if you just used retryDownload and let the UI handle the issue of it succeeding or not.

The biggest issue with the download manager handling all this is that it doesn't start at startup yet - but I can see that needing to change when we do cross session resumable downloads anyway.

I suppose that, for now, we should check this patch in and do the rest of the work in follow-up bugs.
Attachment #268664 - Flags: review?(sdwilsh) → review+
Attachment #268664 - Flags: review?(gavin.sharp)
I filed Bug 384810 to do this in the back-end.  I actually don't think we need to worry about adding a new state (see bug for reason why).
(In reply to comment #15)
> and just to be clear, I still think we need to add a new state for instances
> where the user chooses to not restore a crashed session as per comment #10.  I
> was hoping biesi had something to say about it.

Hm, I'm way behind on bugmail. But if you don't want to retry downloads when the user chooses not to restore their session, you could just cancel them all, couldn't you? (basically call cancelDownload instead of retryDownload)


(In reply to comment #19)
> Hm, I'm way behind on bugmail. But if you don't want to retry downloads when
> the user chooses not to restore their session, you could just cancel them all,
> couldn't you? (basically call cancelDownload instead of retryDownload)
Actually, that view is outdated.  See Bug 384810 for the nitty gritty now
Flags: blocking-firefox3? → blocking-firefox3+
Target Milestone: --- → Firefox 3 M8
Depends on: 384810
Attachment #268664 - Flags: review?(gavin.sharp) → review+
I haven't tested this, I'm assuming at least one of you has - please confirm before landing :)
Well, it works and it doesn't. SessionStore correctly tries to restart interrupted downloads, but retryDownload throws:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDownloadManager.retryDownload]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///C:/Benutzer/Simon/Dokumente/fx-dev/firefox/components/nsSessionStore.js :: anonymous :: line 1624"  data: no]

Since retryDownloads throws independently of this patch, I'd like the patch to land anyway and figure out what's wrong in a follow-up bug against the DM.
Keywords: checkin-needed
The main hunk of this patch doesn't seem to apply, could you attach an updated version?
Attached patch for check-in — — Splinter Review 
Thanks, Gavin!
Attachment #268664 - Attachment is obsolete: true
mozilla/browser/components/sessionstore/src/nsSessionStore.js 	1.71
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Keywords: checkin-needed
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
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 → ---
(In reply to comment #26)
> When I force quit, Restore Session, and open the Download Manager, there's no
> trace of the file.

Any errors in the console? Anyway, this might just as well be due to bug 384810 (which has replaced most of this bug's patch).
Simon: whoops, sorry about that; I should've read comment 22.

I don't see any errors in the Error Console.  I guess I should take this over to bug 384810 (or open a new spinoff bug).

Feel free to reclose; until this works, I won't be able to verify this or bug 384810, but that's expected.
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Using builds:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102913 Minefield/3.0a9pre

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102904 Minefield/3.0a9pre

I tested:

* FTP
* HTTP
* Restore Previous Session
* Start New Session

(and the various combinations thereof), all by forcing Minefield to quit (kill -9 with the PID on Linux, Ctrl-Alt-Del on Windows, and Command-Option-Esc on Mac).

Didn't see any obvious issues; the file was always there, and always successfully resumed the download.

Verified FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: