393821 - Quick downloads (cached, local, data:, failed) don't appear until you reopen Download Manager

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

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

Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007082618 Minefield/3.0a8pre, and I saw this on Fedora Core 7, as well.

Noticed this while testing the fix for bug 393556.

Summary: "Failed" downloads don't appear until you reopen Download Manager.

Steps to Reproduce:

1. Right-click on the following: http://www.domain.fake/not_existent_document.pdf
2. Wait for the download to fail; acknowledge the warning
3. Look at the Download Manager.

Actual Results: 

The failed file's entry isn't populated until you close and reopen the DM window; oddly enough, successive files (if you repeat the steps) seem to populate their entries correctly.  The "Active" section
appears, but with no entries.

Expected Results:

The failed downloads named "not_existent_document.pdf" should immediately appear after confirming the failure-alert dialog in the "Completed" section as "Failed" status.

Comment 1

[Ed Lee :Mardak]

Strange.. I can't reproduce this in a debug build with disable-optimize.

Is there any issue of js not handling events quick enough? I.e., the QUEUED DownloadState event change is sent up to the listener and the FAILED event follows right after it. So both events are waiting to be processed by onDownloadStateChange and it grabs the failed first then queued?

Also, there's possibly some strangeness going on if the download manager isn't initially opened..

Comment 2

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

Attachment: Screenshot of SQLite Data Browser with the entry

Comment 3

[Radek 'sysKin' Czyz]

I can reproduce it for successful downloads as well. Steps to reproduce:

1. Find a relatively small file which is NOT in your cache.
2. Click on it (I haven't tried right-clicking with save-as).
3. When on open/save dialog, wait a bit. The file will download in background and you must wait until it finishes.
4. Finally, pick either save or open (doesn't matter which)

Expected results: DL manager will open and will show the file.
Actual results: it won't open. If it is open, it will not show the file.

Opening it later on shows the file.

Perhaps this helps.

Comment 4

[Shawn Wilsher :sdwilsh]

ah crap, this could be a situation where the download finished in exthandler before we even know about it :/

Comment 5

[Ed Lee :Mardak]

It's not going through exthandler. I'm alt-clicking the link to download which does saveURI -> wbp.

Comment 6

[Ed Lee :Mardak]

(In reply to comment #4)
> ah crap, this could be a situation where the download finished in exthandler
> before we even know about it :/
Filed bug 394536.

Comment 7

[Ed Lee :Mardak]

Attachment: v1

I wasn't writing this patch in particular for this bug.. but it seems like it's fixed with the tryserver build. (I couldn't test it with my debug builds as mentioned before.)

https://build.mozilla.org/tryserver-builds/[e-mail]
https://build.mozilla.org/tryserver-builds/[e-mail]2

It also has patches for bug 385734, bug 394548, bug 394263, bug 394615, bug
394516, bug 394798, bug 223895, bug 393821

So it might have been one of those changes as well.. but code-wise, it seems like the patch works for this bug. Additionally I don't see bug 394536 with the trybuild either.

Patch notes: We should be correctly createDownloadItem-ing now... so no "d'oh" situations, and therefore remove that code from DPL.js

Comment 8

[Shawn Wilsher :sdwilsh]

hmm, I need to think about this a bit

Comment 9

[Ed Lee :Mardak]

Comment on attachment 279539 [details] [diff] [review]
v1

Not quite so it seems. The createDownloads are needed if the dlmgr is already open. We can continue to remove the check here but we'll need to have something tell dlmgr to refresh its list when a download starts.

Comment 10

[Ed Lee :Mardak]

Attachment: v2

Call buildActiveList instead of calling its own createDownloadItem. This unifies the item creation, so debugging future issues is easier with just a single code path to think about. Also, put state in a variable to avoid multiple XPC overheads..

Apply some magic and trust everything is working as expected so that we'll never not have a downloadItem from DownloadProgressListener! (other than initial state to queued)

Comment 11

[Ed Lee :Mardak]

Needs bug 394546's patch, otherwise there'll be a ghost item when retrying a download.

Comment 12

[Ed Lee :Mardak]

Don't checkin before bug 394263 either.

Comment 13

[Ed Lee :Mardak]

Bug 394615 lets these failed downloads actually STOP and trigger the completed.

Comment 14

[Ed Lee :Mardak]

For all things blocked by this bug, you can try a buildbot build that should have the problems fixed.

mac: https://build.mozilla.org/tryserver-builds/[e-mail]
linux: https://build.mozilla.org/tryserver-builds/[e-mail]2

sorry no windows..

Comment 15

[Ed Lee :Mardak]

Yayy, windows build bot is back alive. :)

win: https://build.mozilla.org/tryserver-builds/[e-mail]
mac: https://build.mozilla.org/tryserver-builds/[e-mail]
lin: https://build.mozilla.org/tryserver-builds/[e-mail]2

I've tested the blocked bugs on mac and windows, and things seem to be working again. You can try one of these builds to see if it fixes your bug or wait for this bug to land for nightlies.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 279553 [details] [diff] [review]
v2

r=sdwilsh

Comment 17

[Ed Lee :Mardak]

Attachment: v2.1

unbitrot spacing changes from bug 394263

Comment 18

[Shawn Wilsher :sdwilsh]

Comment on attachment 280441 [details] [diff] [review]
v2.1

r=sdwilsh

Comment 19

[Reed Loden [:reed]]

Mardak, are you going to check this in?

Comment 20

[Shawn Wilsher :sdwilsh]

This bug isn't ready for checkin because it still has an unresolved dependency.

Comment 21

[Ed Lee :Mardak]

Checking in toolkit/mozapps/downloads/content/DownloadProgressListener.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/DownloadProgressListener.js,v  <--  DownloadProgressListener.js
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/mozapps/downloads/content/downloads.js;
/cvsroot/mozilla/toolkit/mozapps/downloads/content/downloads.js,v  <--  downloads.js
new revision: 1.92; previous revision: 1.91
done

Comment 22

[Ed Lee :Mardak]

Attachment: as checked in

Comment 23

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

Verified FIXED using

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

Haven't seen this happen for cached, local, data:, or failed (testcase in comment 0) testcases.

Comment 24

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

in-litmus+

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