390748 - When downloading multiple files, Download Manager doesn't show files it has queued

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[Adam Guthrie]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007080204 Minefield/3.0a7pre

Steps to reproduce:
1. Browse to an HTTP directory with files in it, say MP3s (there must be at least 3 files).
2. Proceed to right click and Save As all of the files to your Desktop.
3. Open the Download Manager.

Actual results:
Only two or one of the files is shown in the Download Manager. However, the others do appear after the first ones finish downloading.

Expected results:
Show all of the files I saved in the Download Manager.

< sdwilsh> ok, I totally regressed that in Bug 380250 by enforcing an invarient

Comment 1

[Adam Guthrie]

This should block the release of Firefox 3 per Mr. Shawn Wilsher.

Comment 2

[Shawn Wilsher :sdwilsh]

So, the simple fix here is to add another state that comes between DOWNLOAD_NOTSTARTED and DOWNLOAD_DOWNLOADING (maybe DOWNLOAD_QUEUED?).

Then, a call to nsDownload::SetState(nsIDownloadManager::DOWNLOAD_QUEUED) would be sufficient to fix this.

Any takers?

Comment 3

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

This depends on a server that limits connections; Adam, do you have a well-known one that we can use in Litmus, that won't be going away soon?

Comment 4

[:Gavin Sharp [email: [e-mail]]]

can't you test this by tweaking the netwerk.http.max* prefs?

Comment 5

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

(In reply to comment #4)
> can't you test this by tweaking the netwerk.http.max* prefs?

Indeed I can; thanks for the tip.  I really should go through about:config/prefs.js. 

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Wasn't this broken and fixed before, too? Definitely blocking.

Comment 7

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

If someone from qa could test this and verify that this does the trick, it'd be greatly appreciated.

Comment 8

[cmtalbert]

(In reply to comment #7)
> Created an attachment (id=276047) [details]
> v1.0
> 
> If someone from qa could test this and verify that this does the trick, it'd be
> greatly appreciated.
> 
Stephen, Adam: I am spinning a release build of this patch + current code for testing. 

Comment 9

[cmtalbert]

Ok, build finally done.  With Adam's help, we verified that this patch *does not* address the issue.  The same problem occurs.

Comment 10

[Shawn Wilsher :sdwilsh]

Does anyone have a good test page for this?

Comment 11

[Adam Guthrie]

This is one of the ones I've used: http://www.square-kun.com/files/songs/The%20Shins/.

Comment 12

[Shawn Wilsher :sdwilsh]

Silly me...I had written this patch with the code from the UI rewrite in mind.  It *should not* work with what is currently checked in on trunk.  Once the new UI lands, I'll updated this.

Comment 13

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

Updated for the new UI. Tested with provided uri and steps to reproduce and this works for me.

Comment 14

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

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

in-litmus+

Comment 15

[Asaf Romano (gone)]

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

Please avoid (void) usage on methods which return nsresults.

Comment 16

[Shawn Wilsher :sdwilsh]

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

(In reply to comment #15)
> (From update of attachment 276423 [details] [diff] [review])
> Please avoid (void) usage on methods which return nsresults.
All instances of this are cases where I really don't care about the result.  I'll explain why for each one. 

>   rv = AddToCurrentDownloads(dl);
>+  (void)dl->SetState(nsIDownloadManager::DOWNLOAD_QUEUED);
>   NS_ENSURE_SUCCESS(rv, rv);
If SetState fails, it just means that updating the database failed, and we don't care at this point because the download will be changing state to DOWNLOAD_DOWNLOADING very soon *and* dispatching to the listeners happened.  The download can still happen, it just might not show up in the UI right away.

>                             nsIWebBrowserPersist::PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION);
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv)) {
>+    dl->mCancelable = nsnull;
>+    (void)wbp->SetProgressListener(nsnull);
>+    return rv;
>+  }
We don't care of setting the progress listener fails because we are already in a failure state.  The previous warning is more useful than this one (we have to do this however to prevent cycles from forming from a failure).

>-  NS_ENSURE_SUCCESS(rv, rv);
>+  if (NS_FAILED(rv)) {
>+    dl->mCancelable = nsnull;
>+    (void)wbp->SetProgressListener(nsnull);
>+    return rv;
>+  }
ditto

>+  (void)dl->SetState(nsIDownloadManager::DOWNLOAD_QUEUED);
same as previous reasoning for SetState

>+  rv = wbp->SaveURI(dl->mSource, nsnull, nsnull, nsnull, nsnull, dl->mTarget);
>+  if (NS_FAILED(rv)) {
>+    dl->mCancelable = nsnull;
>+    (void)wbp->SetProgressListener(nsnull);
>+    return rv;
>+  }
ditto

Comment 17

[Asaf Romano (gone)]

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

ok, r=mano.

Comment 18

[Shawn Wilsher :sdwilsh]

Checking in toolkit/components/downloads/public/nsIDownloadManager.idl;
new revision: 1.15; previous revision: 1.14
Checking in toolkit/components/downloads/src/nsDownloadManager.h;
new revision: 1.33; previous revision: 1.32
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
new revision: 1.101; previous revision: 1.100
Checking in toolkit/mozapps/downloads/content/download.xml;
new revision: 1.34; previous revision: 1.33
Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
new revision: 1.22; previous revision: 1.21
Checking in toolkit/components/downloads/test/unit/head_download_manager.js;
new revision: 1.5; previous revision: 1.4

Comment 19

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007081705 Minefield/3.0a8pre and the STR in comment 0.