408739 - clicking already downloading link doesn't pop dlmgr until first download is complete

Status: NEW

Description

[[e-mail]]

str:

go to your favorite http download site:
http://releases.mozilla.org/pub/mozilla.org/firefox/releases/granparadiso/alpha2/linux-i686/en-US/
click a link to download file (granparadiso-alpha2.tar.bz2)
dlmgr pops up and begins downloading
close dlmgr
click link again
get busy spinning cursor and dlmgr doesn't pop up again.

we should pop up the dlmgr so the user knows what's going on, otherwise it's thoroughly confusing. ;)

Comment 1

[[e-mail]]

also, favicon in tab continues throbbing, and title is Loading...

Comment 2

[(not reading, please use [e-mail] instead)]

I see the same things on win32:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121216 Minefield/3.0b3pre

Comment 3

[(not reading, please use [e-mail] instead)]

> also, favicon in tab continues throbbing, and title is Loading...

if I wait long enough for the first download to finish, what happens next is:

1)  I get the alert in the bottom right of my screen (from the alert service) that the download is complete
2)  the title of the tab goes back to "Index of /pub/mozilla.org/firefox/releases/granparadiso/alpha2/linux-i686/en-US"
3)  the favicon goes back to the right thing (it is no longer the throbber)
4)  I get the "Opening granparadiso-alpha2.tar.bz2 dialog

Comment 4

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

No UI feedback is bad enough to block ...

Comment 5

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

(In reply to comment #0)
> click a link to download file (granparadiso-alpha2.tar.bz2)
> dlmgr pops up and begins downloading
> close dlmgr
> click link again
> get busy spinning cursor and dlmgr doesn't pop up again.
Closing the download manager isn't a necessary step, but clicking the same link is crucial.

It seems like for some reason, we don't make a request if it's the same as one before. That's why the tab's favicon throbber keeps going.

The download manager isn't even notified of the second click.

Btw, this is exthandler as it pops open an open-with dialog and alt-clicking to download doesn't show this issue.

Comment 6

[Shawn Wilsher :sdwilsh]

File Handling per Comment 5

Comment 7

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

Someone suggest an assignee?

Comment 8

[Honza Bambas (:mayhemer)]

I can take a look at this.

Comment 9

[Honza Bambas (:mayhemer)]

Just for record: this bug is also present in FF2. 

The reason is very simple: for the download URL is created a cache entry that is not valid during life time of the download. Therefor, when you try to download the file the second time (from the same URL) and HTTP channel tries to open a cache entry for it, it fails with NS_ERROR_CACHE_WAIT_FOR_VALIDATION and HTTP channel then waits for OnCacheEntryAvailable call. It is triggered when 1) the download is complete 2) the download is canceled.

This is callstack where the HTTP channel opens the cache entry:

>	necko.dll!nsHttpChannel::OpenCacheEntry(int offline=0x00000000, int * delayed=0x0012ef3c)  Line 1426	C++
 	necko.dll!nsHttpChannel::Connect(int firstTime=0x00000001)  Line 285 + 0x10 bytes	C++
 	necko.dll!nsHttpChannel::AsyncOpen(nsIStreamListener * listener=0x05814520, nsISupports * context=0x00000000)  Line 3686 + 0xd bytes	C++
 	docshell.dll!nsURILoader::OpenURI(nsIChannel * channel=0x058138e8, int aIsContentPreferred=0x00000001, nsIInterfaceRequestor * aWindowContext=0x04404268)  Line 840 + 0x19 bytes	C++
 	docshell.dll!nsDocShell::DoChannelLoad(nsIChannel * aChannel=0x058138e8, nsIURILoader * aURILoader=0x0356cd20, int aBypassClassifier=0x00000000)  Line 7486 + 0x41 bytes	C++
 	docshell.dll!nsDocShell::DoURILoad(nsIURI * aURI=0x05814660, nsIURI * aReferrerURI=0x056b1f60, int aSendReferrer=0x00000001, nsISupports * aOwner=0x00000000, const char * aTypeHint=0x0012f584, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, int aFirstParty=0x00000001, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x0012f4f8, int aIsNewWindowTarget=0x00000000, int aBypassClassifier=0x00000000)  Line 7332 + 0x29 bytes	C++
 	docshell.dll!nsDocShell::InternalLoad(nsIURI * aURI=0x05814660, nsIURI * aReferrer=0x056b1f60, nsISupports * aOwner=0x00000000, unsigned int aFlags=0x00000001, const wchar_t * aWindowTarget=0x0012f688, const char * aTypeHint=0x0012f584, nsIInputStream * aPostData=0x00000000, nsIInputStream * aHeadersData=0x00000000, unsigned int aLoadType=0x00200001, nsISHEntry * aSHEntry=0x00000000, int aFirstParty=0x00000001, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x00000000)  Line 7071 + 0x7f bytes	C++
 	docshell.dll!nsWebShell::OnLinkClickSync(nsIContent * aContent=0x056e0cd0, nsIURI * aURI=0x05814660, const wchar_t * aTargetSpec=0x058147a8, nsIInputStream * aPostDataStream=0x00000000, nsIInputStream * aHeadersDataStream=0x00000000, nsIDocShell * * aDocShell=0x00000000, nsIRequest * * aRequest=0x00000000)  Line 891 + 0x6a bytes	C++


This must be catch somewhere at the level of URI loader or some like (I would appreciate some advice here). We have to pop-up the download manager from there (if not prohibit by user prefs).

I will submit a draft patch soon.

Comment 10

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

The download manager wouldn't pop up until the user selects what to do with the file (open with dialog). So potentially we would need to change the behavior of clicking links that are already open instead of waiting for the cache entry. ?

Comment 11

[Honza Bambas (:mayhemer)]

"already open" means files that are currently being downloaded or those that are already downloaded and potentially present on disk (or/and in the cache)?

What you suggest actually means to override the action which user expects to be done when file is downloaded? Problem is that (as I understand) we cannot open the dialog until the channel loads the mime type (or part of content) of the URL. This means that HTTP channel must do the request first but it is blocked with async open of the cache entry.

Comment 12

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

Well, it depends on what it should do. Clicking the link a second time should...

1) just open the download manager
2) download a second copy

?

Comment 13

[Honza Bambas (:mayhemer)]

Created attachment 307507 [details] [diff] [review]
Draft 1

Please just check this draft solution. It pop-ups the download manager with the item focused. This ignores the preference not to pop-up download manager. 

I will test it further more.

Comment 14

[Ed Lee :Mardak (PTO Sep 15 - Oct 7)]

The code seems reasonable if we want to make the download manager show the item when clicking the link a second time. Not sure if we want to bind the docshell to the toolkit download manager though.

Also, we should probably define what's the expected behavior.

The patch would make the second click cause the download manager to open as well as not cause a second download to start (even after the first finishes).

I suppose we do want the download manager to show when clicking the link, so the question is if there should be another download to start concurrently?

Comment 15

[Shawn Wilsher :sdwilsh]

docshell can in no way depend on toolkit fyi

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 307507 [details] [diff] [review]
Draft 1

The patch was just an example. Personally I also don't like to call download manager from such a low level code.

To sum, there are 3 possibilities:
1. Show the download manager (as the patch does)
2. Ask user what to do and change handle action on the current download
3. Ask user what to do and start another download

#1 is very simple just do it more clear way. IMHO I would expect such behavior, because if I want to change the action on the file I can do it after it has been downloaded

#2 is more complicated because we must bypass the cache (not to wait for the cache item) or remember the content type on the URI (what might be asynchronous, dependent on the content, in case of e.g. pictures w/o mime type header).

#3 means the same (bypass the cache) and actually run/instantiate another one (a concurrent one). As far as I am familiar with the cache code this is probably impossible.

As short/middle therm solution I would vote for #1.

Comment 17

[Honza Bambas (:mayhemer)]

If we want solution #1 then I become to conclusion we probably need some new hook in nsDocShell that would override click on link/link load that is already in progress (has been downloading). There is currently no mechanism how to gracefully bypass content load and channel creation (what we must avoid to fix the bad UI response - waiting for the cache item).

Any suggestions? I would vote for new observer event like "on-uri-load" taking nsISupportsBoolean that could be set TRUE by observers to bypass the load.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

4) wontfix this bug

Comment 19

[Mike Schroepfer]

I vote for comment 18 - taking this off the blocking list...

Comment 20

[Honza Bambas (:mayhemer)]

Or even WONTFIX ?