The default bug view has changed. See this FAQ.

Cross-Session resumable downloads (resume after quitting firefox)

VERIFIED FIXED in mozilla1.9beta1

Status

()

Toolkit
Downloads API
--
enhancement
VERIFIED FIXED
13 years ago
8 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: Mardak)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla1.9beta1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0 -
blocking-aviary1.5 -
blocking1.9 -
wanted1.9 +
blocking-firefox2 -
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: CON-007a, URL)

Attachments

(2 attachments, 5 obsolete attachments)

To be effective, downloads should be pause/resumable across sessions.
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P3
Target Milestone: --- → Firebird1.0

Comment 1

13 years ago
*** Bug 234405 has been marked as a duplicate of this bug. ***
Blocks: 215093
Target Milestone: Firefox1.0 → Firefox1.0beta

Comment 2

13 years ago
Before you focus on making them resumable across sessions, shouldn't you try to
make resuming work within a session? As it is, whenever one uses the Pause
button you effectively cancel your download and have to start over again. I know
there's a bug for Seamonkey where this is being worked on (Bug 18004), but
someone in there said that Firefox's download manager is different from the one
in Seamonkey.
Thought I'd mention it... :)

Comment 3

13 years ago
taking qa
QA Contact: aebrahim

Updated

13 years ago
Flags: blocking1.0?
Flags: blocking1.0? → blocking1.0+
missing the 1.0 boat. 
Priority: P3 → P4

Comment 5

13 years ago
It looks like the backend needed for this is already done by bug 107552.

Bradley Baetz said in a blog comment:
"We also have backend code to resume HTTP/FTP downloads (across sessions, too),
which I wrote a while back. Theres just no front end code for it..."
http://weblogs.mozillazine.org/gerv/archives/005581.html
Depends on: 107552

Comment 6

13 years ago
(In reply to comment #4)
> missing the 1.0 boat. 

Nooooooooo, I was looking so forward to this... It would so make people favor it
over IE. Only free browser that actually would resume downloads on its own, now
thats a selling point. 

Comment 7

13 years ago
(In reply to comment #4)
> missing the 1.0 boat. 

Setting blocking-aviary1.0- and target for After Firefox 1.0.
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
Target Milestone: Firefox1.0beta → After Firefox 1.0

Comment 8

13 years ago
*** Bug 251392 has been marked as a duplicate of this bug. ***

Updated

13 years ago
No longer blocks: 215093

Comment 9

13 years ago
*** Bug 256620 has been marked as a duplicate of this bug. ***

Comment 10

13 years ago
See my bug <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=256620">256620</a> for a more
complete description of this bug.

Comment 11

13 years ago
Please work this into the priority list.  "Retry" has never worked for me, and
now I finally know why.  

If "retry" won't work, then don't make it available in the GUI until it does work.

My net connection is not reliable.  I would like to rely on Firefox to be able
to resume "Resumable" sources.  So far sources that I resume from normally fail
to resume in my Firefox RC1.0.

-Tor
*** Bug 262734 has been marked as a duplicate of this bug. ***
*** Bug 264589 has been marked as a duplicate of this bug. ***

Comment 14

13 years ago
I hope this will get implemented in the final release of Firefox 1.0 because I
see this as an important feature for a download manager... otherwise I can't see
the sense of making a download manager with resume ability because normally you
use this ability if you turn off the PC and want to continue later...

Comment 15

13 years ago
*** Bug 269563 has been marked as a duplicate of this bug. ***

Comment 16

12 years ago
Ben, I know you're all working on the roadmap right now, so do you plan on
working this in somewhere in the near future? It's being requested a lot on the
forums at the moment.
More details are at the URL in the URL field.  Don't expect too much detail
(probably more than exists now, but I have no idea how much more), because I'd
bet Ben would rather write something useful than write something that's
"marketable" (in the small-community fans sense of the word).

Comment 18

12 years ago
The only thing I don't like about Firefox is unfortunately the downloader, so I
still use IE when downloading big files. If you implemented this feature, this
would be the best browser on the market.

Comment 19

12 years ago
(In reply to comment #7)
> (In reply to comment #4)
> > missing the 1.0 boat. 
> 
> Setting blocking-aviary1.0- and target for After Firefox 1.0.

Well, "After Firefox 1.0" has arrived. Reblocking for 1.1.
Flags: blocking-aviary1.1?

Comment 20

12 years ago
*** Bug 267163 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.1? → blocking-aviary1.1-
Summary: Cross Session resumable downloads → Cross Session resumable downloads (resume after quitting firefox)
*** Bug 288558 has been marked as a duplicate of this bug. ***

Comment 22

12 years ago
*** Bug 291163 has been marked as a duplicate of this bug. ***

Comment 23

12 years ago
*** Bug 292255 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Blocks: 163993

Updated

12 years ago
No longer blocks: 163993
*** Bug 295778 has been marked as a duplicate of this bug. ***

Comment 25

12 years ago
*** Bug 295982 has been marked as a duplicate of this bug. ***

Updated

12 years ago
Flags: blocking-aviary2.0?

Updated

12 years ago
Flags: blocking1.9a1?

Comment 26

12 years ago
*** Bug 308397 has been marked as a duplicate of this bug. ***

Comment 27

12 years ago
*** Bug 311264 has been marked as a duplicate of this bug. ***

Comment 28

12 years ago
*** Bug 316740 has been marked as a duplicate of this bug. ***
Assignee: bugs → nobody
Status: ASSIGNED → NEW
QA Contact: aebrahim-bmo-507 → download.manager

Comment 29

12 years ago
if nobody is able to integrate this for 2.0 we should not bother with it and just focus right now on a complete redesign of download manager for 1.9 (I know, already a feature for 3.0) that would let us integrate this feature a bit easier. We need a real download manager, wich is both clean (not bloated) and customizable. Maybe torrent support too or a web-page sniffing feature in it, if you got enought skillz and courage. May the force be with Firefox 3 ;-)
I don't think this can get done in a robust and stable manner in time for Fx2 without API churn on the Gecko level.  Minusing, but if someone is very interested in making this happen, talking to darin and biesi would be a good first step in scoping the work.
Flags: blocking-aviary2? → blocking-aviary2-
related to bug 87151?  (wow, 133 votes)

Comment 32

12 years ago
Please ensure that the download manager window disappears automatically, after showing completed downloads. It would be nice if the download manager disappers after 1 second. 

Comment 33

12 years ago
...I prefer to just Launch the file from the Download Manager.

Comment 34

11 years ago
*** Bug 321191 has been marked as a duplicate of this bug. ***

Updated

11 years ago

Updated

11 years ago
Blocks: 331979
*** Bug 321191 has been marked as a duplicate of this bug. ***
whose domain is this? Is it practical to fix this or should we be making it clear somehow in the ui that you can't resume?

Comment 37

11 years ago
See http://wiki.mozilla.org/Download_resuming.
I'd be interested in fixing this, but I'm not sure when I'll have the time to do so; it's unlikely to happen within the next couple months.  Reassigning for now so that I don't forget this, but if someone wants to write a patch in that time I won't be too sad I didn't get around to doing so.  ;-)
Assignee: nobody → jwalden+bmo

Updated

11 years ago
Blocks: 349971

Comment 39

11 years ago
*** Bug 353379 has been marked as a duplicate of this bug. ***
Flags: blocking1.9a1? → blocking1.9-
*** Bug 359668 has been marked as a duplicate of this bug. ***

Comment 41

11 years ago
*** Bug 362593 has been marked as a duplicate of this bug. ***

Comment 42

11 years ago
We really want to have this for Firefox 3.
Flags: blocking-firefox3?

Updated

10 years ago
Duplicate of this bug: 338704

Comment 44

10 years ago
I think this is extremely important for Firefox 3 as well. 

If this isn't fixed, users should at least be alerted to it's limitations, and that you can't resume a download between sessions. The sooner this is made clear, the better. Imagine how frustrating this could get on dialup for a very large file. Even on broadband, large downloads can take hours and laptops hibernate, etc.

Updated

10 years ago
Blocks: 372972

Updated

10 years ago
Flags: blocking-firefox3? → blocking-firefox3+
Hello,

I will be working on this, "Cross-session download resume", under Dan Mosedale (dmose).

My first job is the UI design for the Download Manager(DM). I made a small survey of DMs of few other browsers and found the DM of Safari to be very pleasing and suitable(with certain modifications. I request you people to pour in ideas for the UI. I will come up with a simple XUL definition ASAP. 

Lets have this in FF-3.

Regards
Brahmana
you might find bug 18004 comment 102 and the following ones interesting, although that wasn't for firefox.

Comment 47

10 years ago
Personally, I think Opera's DM is much better than any others, it provides more control and information. The current Fx DM is very basic, and so is Safari's if I recall correctly. I'm not sure if we're going for a more functional or a more streamlined UI here, but I think the way it is already it's quite dumbed down, and Safari's is if anything even more dumbed down. However, I don't know what the aims of the Fx team are so this might be way off. Just sharing what my thoughts as a "power user" are I guess.

Comment 48

10 years ago
(In reply to comment #45)
> My first job is the UI design for the Download Manager(DM). I made a small
> survey of DMs of few other browsers and found the DM of Safari to be very
> pleasing and suitable(with certain modifications. I request you people to pour
> in ideas for the UI. I will come up with a simple XUL definition ASAP. 

I don't think the UI really needs any additional work. The Firefox download manager already has percentage complete and pause/resume links. 

In my opinion, the issues to define for this bug are:
 - Should the Download Manger open itself? If so, under what circumstances / respecting which preferences?
 - Should the downloads automatically resume, or should this be manual? Should this be a preference?
Even if we were planning on doing a UI redesign of the download manager (we're currently not) that's out of scope for this bug, and certainly hasn't been brought to me by Dan.  We've got a fairly comprehensive comparative analysis that we might act on, but the focus of this bug should be on making the backend support download resume across sessions.  There's very little UI that would be necessary to make this work with the current DM, in any case.

Comment 50

10 years ago
> In my opinion, the issues to define for this bug are:
>  - Should the Download Manger open itself? If so, under what circumstances /
> respecting which preferences?
>  - Should the downloads automatically resume, or should this be manual? Should
> this be a preference?
> 
Should depend on what session restore says to do.

If they click or have set session restore to go - yes resume, if not allow manual resumption by however you plan to do so.

Comment 51

10 years ago
Any bugs should be kept as narrow in definition as possible. Apart from the many sensible reasons for this, there's also the satisfaction of saying "we killed X bugs" instead of we killed one bug :)

Seriously all that is needed is to add a "Resume" link in the DM to the first link that is either Pause (which will now become more meaningful) or Open.

This bug is about functionality, not UI. 

It's simply a matter of making the existing 'Pause' function span sessions.

One thing at a time people :)

Comment 52

10 years ago
Oops, my mistake, sorry all. 

The "Pause / Resume" link is already there but separate to the "Cancel / Open" links.

This bug is therefore, IMHO, UI agnostic :) It's a back end thing. 

If DM is to be improved (getting it to open faster than a minimum of 5 seconds would be a great start) then that should be a separate bug.

Comment 53

10 years ago
Oops, my mistake, sorry all. 

The "Pause / Resume" link is already there but separate to the "Cancel / Retry / Open" links.

This bug is therefore, IMHO, UI agnostic :) It's a back end thing. 

If DM is to be improved (getting it to open faster than a minimum of 5 seconds would be a great start) then that should be a separate bug.

Updated

10 years ago
Blocks: 377243
btw,  implementionally wise :

* is resume going to be implemented using ResumeAt method ? 
* Is output stream gets closed even when "normal pause" is called ?

Updated

10 years ago
Whiteboard: CON-007a

Comment 55

10 years ago
If you do delve into the UI aspects, perhaps you can borrow something from here:

Download Manager Tweak extension
http://dmextension.mozdev.org/
It is indeed the case that there's not a ton of UI work that's needed here, and some of the confusion was caused by me not communicating clearly enough.  That said, it is generally a good idea to get UI design up front in order to avoid possible impedance mismatch between the desired UI and the code behind it.  I'll assert that a quick design of what the end-to-end experience of cross-session download resumption should be like is in order (probably on the wiki).  It should address at least the questions in comment 48 should be addressed, and Brahmana also mentioned to me an interesting behavior that Safari does with partial downloads that we may also wish to consider.
Hello,

A separate bug has been introduced to track the progress of my project. It is https://bugzilla.mozilla.org/show_bug.cgi?id=377243

I have put up a initial design proposal there. I request the folks watching this bug to have a look at that also.

Regards
Brahmana

Comment 58

10 years ago
Will there be any type of indicator for the user if a server doesn't support resume? External download managers like GetRight show you if a server doesn't support resuming, so you can give those downloads priority.
(In reply to comment #58)
> Will there be any type of indicator for the user if a server doesn't support
> resume? External download managers like GetRight show you if a server doesn't
> support resuming, so you can give those downloads priority.
> 

We had a discussion about this. I plan to provide a small image/icon in the DM window for each download entry. But we are still not able to decide what sort of image would make the users understand that there is server support for resume or not. It is mentioned in the design at http://wiki.mozilla.org/User:Brahmana/Design_Proposal_1 

So it would nice if I get to know what other Download Managers do or how they show it. 

Inputs for this are requested from the users of other DMs.

Regards
Brahmana
you could change the colour of the file name to make understand to user resumable  downloads.
For example they could be green while normal downloads could be black, a tooltip could show then the full url of the file and under a green text "You can resume this download" or a black text "you cannot resume this download"

an icon could be a rotating circle to indicate a resumable, a broken circle to indicate a non resumable download

Updated

10 years ago
Assignee: jwalden+bmo → dmose
Target Milestone: Future → Firefox 3 beta1
Summary: Cross Session resumable downloads (resume after quitting firefox) → Cross-Session resumable downloads (resume after quitting firefox)

Updated

10 years ago
Blocks: 254544

Updated

10 years ago
Duplicate of this bug: 316740

Updated

10 years ago
Target Milestone: Firefox 3 M7 → Firefox 3 M8
This is a P2 based on the PRD, fixing blocking status (should be wanted, not blocking)
Flags: blocking-firefox3+ → blocking-firefox3-
Whiteboard: CON-007a → CON-007a [wanted-firefox3]

Comment 63

10 years ago
Where is P1, P2, CON-007a, etc. explained? This would be helpful, but there is nowhere to find this out like there is with the Keywords (by clicking the hyperlink). Thanks.
Taking, retargeting.

Note to self on implementation:
Remove paused resumable downloads from mCurrentDownloads before possibly trowing up the popup.
Fix nsDownloadManager::PauseResumeDownload to get the download from the database on resuming if it isn't in mCurrentDownloads.
I think that should do it for the back-end.  Not sure how the UI will react...
Assignee: dmose → comrade693+bmo
Priority: P4 → --
Target Milestone: Firefox 3 M8 → Firefox 3 M9
Version: unspecified → Trunk
...and track the temp file in the database, can't resume without it :/
Created attachment 280141 [details] [diff] [review]
patch, v1

It seemed a little odd to me to pull paused resumable stuff out of mCurrentDownloads before possibly throwing up the dialog and then having to put it back if the user cancelled.  So I opted instead to pull any paused download out at the time of pausing in nsDownloadManager::PauseResume.

There is still something goofy going on here, in that pressing pause now doesn't actually pause it; I suspect some relationship to the UpdateDB call I added to nsDownload::Pause, so perhaps my strategy above is the wrong one.

It's also still unclear whether the resuming after restarting the browser is actually a proper resume or just a re-download.
Created attachment 280144 [details] [diff] [review]
patch, v2

OK, the not really pausing thing was just a thinko.  Fixed here.  Things are still somewhat wonky, though, in that intra-session resume is now busted.  So this still needs some coding & testing love.
Attachment #280141 - Attachment is obsolete: true
(Assignee)

Comment 68

10 years ago
Comment on attachment 280141 [details] [diff] [review]
patch, v1

(In reply to comment #66)
> It seemed a little odd to me to pull paused resumable stuff out of
> mCurrentDownloads before possibly throwing up the dialog and then having to put
> it back if the user cancelled.  So I opted instead to pull any paused download
> out at the time of pausing in nsDownloadManager::PauseResume.
The funky resume-paused-download-on-cancel is for fake-paused downloads that still have the channel open. I commented somewhere in bug 377243 about not needing to resume a real-paused download because the connection is already closed.

> There is still something goofy going on here, in that pressing pause now
> doesn't actually pause it; I suspect some relationship to the UpdateDB call I
> added to nsDownload::Pause, so perhaps my strategy above is the wrong one.
No need to call UpdateDB because SetState will do so.

> It's also still unclear whether the resuming after restarting the browser is
> actually a proper resume or just a re-download.
Did you try with the patch from bug 395134 (which needs bug 385734 and bug 395205 to display correctly)? I've been testing by downloading a .dmg, pause, resume and see if it passes its integrity check when opening.

>+  // target
>+  rv = stmt->BindStringParameter(3, aTempPath);
nit: // tempPath

>-  rv = stmt->BindInt64Parameter(3, aStartTime);
>+  rv = stmt->BindInt64Parameter(4, aStartTime);
...
I've been thinking about doing it for my own patches, but we could just do Bind*Parameter(i++) so that we don't need to change the number each time we insert something..

> nsresult
> nsDownloadManager::PauseResumeDownload(PRUint32 aID, PRBool aPause)
...
>+  rv = dl->PauseResume(aPause);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  if (aPause) {
>+    (void)mCurrentDownloads.RemoveObject(dl.get());
>+  }
We don't want to remove the object for fake-paused downloads, so probably just move the RemoveObject call into PauseResume's path that handles real-pausing.

>@@ -2065,17 +2103,20 @@ nsDownload::PauseResume(PRBool aPause)
>-    return SetState(nsIDownloadManager::DOWNLOAD_PAUSED);
>+    rv = SetState(nsIDownloadManager::DOWNLOAD_PAUSED);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    return UpdateDB();
SetState calls UpdateDB, but calling twice shouldn't do anything bad either..

sdwilsh: About the paused states, should we have a DOWNLOAD_FAKEPAUSED? (That might help clean up the code.. maybe) And on the UI side of things, a new label for real-paused? Now that's a nightmare.. "Active" for queued, downloading, fake-paused, scanning, "Paused" for paused, and "Completed" for everything else... 'what?! there's paused items in both Active and Paused...'
A few comments on the patch:
-I don't the see the column being added in nsDownloadManager::CreateTable
-I don't like removing a download always if it is paused.  Chances are, a user isn't going to pause for cross session stuff usually.  Better solution:
Determine the proper count at quit-application-requested, but do not actually remove the downloads.  When we receive the notifications for quit-application, then remove the resumable downloads, and cancel the non-resumable ones (at this point they've said OK).
-You may want to add a convenience method on nsDownloadManager called IsResumable.  All you'll check is if mEntityID.IsEmpty().
-On resumed downloads that are not in the mCurrentDownloads, you'll have to add them into the array.
-You may want to pull out the resumable code into a new method - nsDownload::ResumeAt.  This isn't necessary, but I think it will make the code a bit easier to follow.  It would involve pulling everything from here (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.113#2077) to here (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp&rev=1.113#2133) out, but it's pretty straightforward.
-Need a test case for the schema migration ;)
(Assignee)

Comment 70

10 years ago
dmose, sdwilsh: I can take this if you want. I've got the other patches from various other bugs (bug 385734, bug 377243, bug 395188, bug 395134) already applied in my stack to make it easier to test and stuff.
(Assignee)

Comment 71

10 years ago
Created attachment 280199 [details] [diff] [review]
v3

This patch (+ others in the stack) has cross session downloads working for manually paused downloads. I tested for both exthandler and wbp downloads of .dmg files, and the archive was able to verify itself and extract Firefox.

Quitting with some paused and some active downloads will only cancel the actively transferring ones. One tiny issue is that when starting up Firefox, there is no status, but that will likely be fixed by bug 394263.

1) Start download
2) Pause download
3) Quit Firefox
4) Open Firefox
5) Resume download
6) Finish download

Note that with steps 2 and 5, it means we aren't doing auto-pause/auto-resume here.

Comments:
- There's a bunch of new "flags" for nsDownload: IsPaused, IsResumable, WasResumed, IsRealPaused as well as a couple helper functions Cancel and Resume.
- Database changes are for mTempFile's path (not target) for use with exthandler. 
- Various refactoring and cleanup..
Assignee: comrade693+bmo → edilee
Attachment #280144 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #280199 - Flags: review?(comrade693+bmo)
(Assignee)

Updated

10 years ago
No longer blocks: 377243
Depends on: 377243, 385734, 395134, 395188, 395205
(Assignee)

Comment 72

10 years ago
Created attachment 280239 [details] [diff] [review]
v4

Per request of sdwilsh, I've broken up the patch into smaller pieces. Neat thing with mercurial is that I can keep a whole stack of patches and export them all (hg export xs.1.patch:xs.10.patch) as one big patch.

So here we are with just 10 ----ing patches. ;)

xs.1 cleanup spacing
xs.2 cleanup stars
xs.3 move logic out of .h
xs.4 i++ indexing
xs.5 helper-function-ify flags
xs.6 use flag functions
xs.7 refactor PauseResume to Pause, Cancel, Resume, RealResume
xs.8 use refactored Cancel and cleanup/fix issues
xs.9 add tempPath to schema
xs.10 implement cross session resumable downloads :)

I suppose technically if I was able to break it into 10 pieces, there could be 10 separate bugs filed..

Notes:
3) IsInProgress wasn't even used
7) no more pause/resume -> pauseResume -> pause/resume
8) includes moving aDl->mDownloadManager = this; to save using it in 3 places
8) only resume fake-paused downloads on cancel (and not real-paused)
8) made retryDownload similar to resume for cycle creation failure cases
9) adding a new column is cleaner with (4)'s i++ change
10) only cancel non real-paused downloads, and restore them on startup
Attachment #280199 - Attachment is obsolete: true
Attachment #280239 - Flags: review?(comrade693+bmo)
Attachment #280199 - Flags: review?(comrade693+bmo)
Could we hold off on xs.1 and xs.2 and actually spin those off into new bugs please?
Comment on attachment 280239 [details] [diff] [review]
v4

>+  // Track where we resumed because progress notifications restart at 0
>+  mResumedAt = fileSize;
>+  mCurrBytes = 0;
>+  mMaxBytes = LL_MAXUINT;
This is one of your other bugs, yes?

>@@ -1077,12 +1107,17 @@ nsDownloadManager::AddDownload(DownloadT
>   // Creates a cycle that will be broken when the download finishes
>   dl->mCancelable = aCancelable;
> 
>-  // Adding to the DB
Please keep that comment

>+nsresult
>+nsDownloadManager::RestoreActiveDownloads()
>+{
>+  nsCOMPtr<mozIStorageStatement> stmt;
>+  nsresult rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+    "SELECT id "
>+    "FROM moz_downloads "
>+    "WHERE state = ?1 "
>+      "AND LENGTH(entityID) > 0"), getter_AddRefs(stmt));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = stmt->BindInt32Parameter(0, nsIDownloadManager::DOWNLOAD_PAUSED);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasResults;
>+  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResults)) && hasResults) {
>+    nsRefPtr<nsDownload> dl;
>+    // Keep trying to add even if we fail one, but make sure to return failure
>+    if (NS_FAILED(GetDownloadFromDB(stmt->AsInt32(0), getter_AddRefs(dl))) ||
>+        NS_FAILED(AddToCurrentDownloads(dl)))
>+      rv = NS_ERROR_FAILURE;
>+  }
>+  return rv;
>+}
>+
I'm actually pretty sure that this will not work.  SQLite doesn't like more than one statement open on the database at the time, so you have to close one statement then start another.  This is why we use an nsTArray of download id's in RestoreDatabaseState.

Otherwise, this looks pretty good - just a lot of trivial changes in the beginning.  Please post a new patch with comments fixed.
(Assignee)

Comment 75

10 years ago
Created attachment 280259 [details] [diff] [review]
v4.1

(In reply to comment #74)
> (From update of attachment 280239 [details] [diff] [review])
> >+  // Track where we resumed because progress notifications restart at 0
> This is one of your other bugs, yes?
Yes, this is bug 395134 from the depends on list, but this patch will move the code.

> >@@ -1077,12 +1107,17 @@ nsDownloadManager::AddDownload(DownloadT
> >-  // Adding to the DB
> Please keep that comment
Restored.

> >+nsresult
> >+nsDownloadManager::RestoreActiveDownloads()
> >+  while (NS_SUCCEEDED(stmt->ExecuteStep(&hasResults)) && hasResults) {
> >+    if (NS_FAILED(GetDownloadFromDB(stmt->AsInt32(0), getter_AddRefs(dl)))
> SQLite doesn't like more than one statement open on the database at the time
I would have been surprised if SQLite didn't support multiple readers at the same time.

"While a given SQLite connection is capable of having multiple statements open, its locking model limits what these statements can do concurrently (reading or writing). It is in fact possible for multiple statements to be actively reading at one time. It is not possible, however, for multiple statements to be reading and writing at one time on the same table -- even if they are derived from the same connection. "
http://developer.mozilla.org/en/docs/Storage#SQLite_Locking

I updated the comment to be aware of not modifying the table as we iterate.
Attachment #280239 - Attachment is obsolete: true
Attachment #280259 - Flags: review?(comrade693+bmo)
Attachment #280239 - Flags: review?(comrade693+bmo)
(Assignee)

Comment 76

10 years ago
For those who want to test cross session resumable downloads, I *highly recommend* creating a new profile just incase -- *** especially if you haven't been using nightly builds regularly ***.
http://kb.mozillazine.org/Profile_Manager#Creating_a_new_profile
(And if that sounds scary, you probably don't want to try this out.)

There's a mac and linux build of this patch + dependent patches and everything seems to be working for me.
https://build.mozilla.org/tryserver-builds/89-dtownsend@mozilla.com-firefox-try-mac.dmg
https://build.mozilla.org/tryserver-builds/90-dtownsend@mozilla.com-firefox-try-linux.tar.bz2
(There is no win32 build for technical reasons of the build server..)

There's 3 attributes to test for..
1) no pause, same session resume, cross session resume
2) http, ftp
3) save link, save as (prompt; e.g., click a link to .dmg)

NHL = no pause, http, save link

Mac OS X:
NHL works
NHP works
NFL works
NFP works
SHL works
SHP works
SFL works
SFP works
XHL works
XHP works
XFL works
XFP works

Those are my own results using files from http://ftp.mozilla.org and ftp://ftp.mozilla.org. Clicking a .dmg link results in a "prompt" type of download while right click -> save link is the "save link" download.

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.6-candidates/rc2
Scroll down to see the list of .dmg files and choose a different locale each time to avoid not actually downloading (using cache). After downloading, opening the dmg will cause itself to verify its integrity which should succeed if the file was successfully downloaded.

Assuming we get enough testing (in the next few hours), this might make it into Firefox 3 Alpha 8.
Comment on attachment 280259 [details] [diff] [review]
v4.1

r=sdwilsh, with comments on irc fixed.
Attachment #280259 - Flags: review?(comrade693+bmo) → review+
(Assignee)

Updated

10 years ago
Blocks: 395537

Updated

10 years ago
Attachment #280259 - Flags: approval1.9?
(Assignee)

Comment 78

10 years ago
Created attachment 281115 [details] [diff] [review]
v4.1 xs.9's schema testcase

Whoops, forgot the schema migration from v4 to v5. This belongs with xs.9.patch...

If v4.1 patch gets approved, does that automatically mean each subpatch gets approved so it'll be okay to split the superpatch into multiple bugs? This bug really should only have xs.10.patch and another bug with xs.9 + xs.schema and various other bugs.

If so, I'll attach the binary v4.sqlite file to the split off bug.
Attachment #281115 - Flags: review?(comrade693+bmo)

Updated

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

Updated

10 years ago
Depends on: 396457
(Assignee)

Comment 79

10 years ago
Created attachment 281212 [details] [diff] [review]
patch for checkin

Now that this bug depends on bug 396457, all that's needed for the patch in this bug is xs.10.
Attachment #281115 - Attachment is obsolete: true
Attachment #281115 - Flags: review?(comrade693+bmo)

Updated

10 years ago
No longer blocks: 372972
(Assignee)

Comment 80

10 years ago
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.124; previous revision: 1.123
done
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.h,v  <--  nsDownloadManager.h
new revision: 1.46; previous revision: 1.45
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
This seems to work really well.

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

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

-and-

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

I tested with a bunch of Linux ISO FTP/HTTP servers, and a good amount of ad-hoc/stress testing, and it's resilient.

Verified FIXED (phew!)
Status: RESOLVED → VERIFIED
Flags: in-litmus?

Updated

10 years ago
Duplicate of this bug: 406298
https://litmus.mozilla.org/show_test.cgi?id=5021

and

https://litmus.mozilla.org/show_test.cgi?id=4720
Flags: in-litmus? → in-litmus+
Flags: wanted-firefox3+
Whiteboard: CON-007a [wanted-firefox3] → CON-007a
Duplicate of this bug: 426577

Updated

9 years ago
Duplicate of this bug: 434795

Comment 86

9 years ago
I suggest reopening, because this feature does not reliably work on FF3.  I have such a download (will share by e-mail on request) where 65.5 MB out of 74.1 was completed on an earlier attempt, but FireFox *refuses* to resume from there, will always restart if I try to resume it.  I don't know if the problem is an incomplete block at end of file, or some flags missing in FF, but it's reproducible and needs to be fixed.
I strongly suspect that this is a server issue.  Regardless, please file a new bug with the file in question.
As mentioned by Shawn, problem can be from the server side also. If the server is a HTTP/1.0 server, it will be support resuming and hence Firefox can't resume and will fall back to the old behavior or restarting the download.

If you are able to do a resumable download from the same server using some other tool then please mention that also in the new bug that you would be filing.

Regards,
Brahmana.

Comment 89

9 years ago
I resumed the same file at three earlier points during the ten hours it took to get this far, so the server is certainly capable of it.

Comment 90

9 years ago
-> new bug, bug 445543.
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.