381603 - Adapt to download manager changes

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

With the move from RDF to MozStorage in bug 380250, _retryDownloads needs to be adapted/simplified - in case it's still required after all (depending on how bug 377243 turns out).

Anyhow: Automatic download restarting after a crash is broken on the latest Trunk builds.

Comment 1

[Shawn Wilsher :sdwilsh]

Would someone kindly point me to where the relevant session restore code is?

Comment 2

[Simon Bünzli]

(In reply to comment #1)
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.60&mark=1583-1653#1577

Comment 3

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This works correctly for me.  The whole comment about different file types stored is a bit suspicious to me.  I don't think it works that way anymore, but I left the try catch in there in case it is that way for windows (I can't check that).

Comment 4

[Simon Bünzli]

(In reply to comment #3)
>       try {
>         var savedToURI = Cc["@mozilla.org/network/io-service;1"].
>                          getService(Ci.nsIIOService).
>                          newURI(savedTo, null, null);
>         if (savedToURI.schemeIs("file"))
>           savedTo = savedToURI.path;
>       }
>       catch (e) { /* not a URI, assume it was a string of form #1 */ }

Please don't remove the comment if you leave the hack. OTOH since that hack isn't needed with mozStorage, the correction of wrongly stored paths (if indeed they are wrongly stored and not only wrongly returned) should happen during the conversion to mozStorage. So you should be able to remove the hack altogether (filing a follow up bug about investigating the conversion if needed).

>+      // We need to remove it from the database otherwise it just sits there

Please move that comment to the beginning of the code block where you remove all items (and add a further note why you can't just remove all items still marked as downloading in one swoop).

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

(In reply to comment #4)
> Please don't remove the comment if you leave the hack. OTOH since that hack
> isn't needed with mozStorage, the correction of wrongly stored paths (if indeed
> they are wrongly stored and not only wrongly returned) should happen during the
> conversion to mozStorage. So you should be able to remove the hack altogether
> (filing a follow up bug about investigating the conversion if needed).

I'm not so sure I handle that with the conversion, but I haven't seen any bugs filed on it either.  It is something I need to test more (but I think it is covered by the unit tests which are passing all platforms).  Looking at what nsIURI.spec returns, we always need to convert.

> Please move that comment to the beginning of the code block where you remove
> all items (and add a further note why you can't just remove all items still
> marked as downloading in one swoop).

Thanks for pointing that out.  That was completely unnecessary.

Comment 6

[Mike Connor [:mconnor]]

Comment on attachment 266520 [details] [diff] [review]
v1.1

looks good.  feels like something we need test coverage for (but that's true for session store in general)

Comment 7

[Shawn Wilsher :sdwilsh]

Checking in browser/components/sessionstore/src/nsSessionStore.js;
new revision: 1.61; previous revision: 1.60

Comment 8

[Jeff Walden [:Waldo]]

I suggest a Litmus test for this, since I doubt infra for sessionstore testing is going to happen soon.

Comment 9

[Simon Bünzli]

(In reply to comment #5)
> Looking at what nsIURI.spec returns, we always need to convert.

Note that you don't really convert the file: URL to a valid path, you just use its path element which happens to look very similar to a Unix path. The correct way to convert would be to QueryInterface the URI to a nsIFileURL and then use savedToURI.file.path.

Note to self: if download restarting fails for whatever reason, the download silently disappears from the list without the user being able to manually fetch its source URL...

Comment 10

[cmtalbert]

(In reply to comment #8)
> I suggest a Litmus test for this, since I doubt infra for sessionstore testing
> is going to happen soon.
> 

I'm trying to come up with some good steps for a Litmus case.  Here is the short version, let me know what you think?
1. Begin a large download
2. kill the firefox process (to simulate a crash)
3. Restart firefox and the download should automatically resume where it left off.
--> Note that in the current GP nightly (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a5pre) Gecko/20070531 Minefield/3.0a5pre) this automatic resume doesn't happen.  The incomplete download vanishes from the DM list.

Or, is this more of the test case that you had in mind:
1. Begin a large download
2. Click "Cancel" on the DM
3. Click "Retry" on the DM, and the download restarts *with the beginning of the file*

It seems to me that having the DM restart the entire file download is the proper behavior since we have a separate "Resume" link that restarts the download where it left off.

Look forward to your comments for the test case. Thanks!

Comment 11

[Shawn Wilsher :sdwilsh]

The former is correct, but there is still an issue at least with windows (see Bug 382513) which you might be seeing.

Comment 12

[cmtalbert]

(In reply to comment #11)
> The former is correct, but there is still an issue at least with windows (see
> Bug 382513) which you might be seeing.
> 
Seeing a similar issue in Linux too.  Will comment in bug 382513.

Comment 13

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/show_test.cgi?id=4589

in-litmus+ (Thanks to Clint for the testcase; let me know if it needs tweaking.)