Closed
Bug 597374
Opened 14 years ago
Closed 14 years ago
Downloading of a file from a password protected directory fails
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b7
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
blocking1.9.2 | --- | - |
status1.9.2 | --- | wanted |
status1.9.1 | --- | unaffected |
People
(Reporter: thomas.braun, Assigned: mayhemer)
References
(Depends on 1 open bug, )
Details
(Keywords: regression, Whiteboard: [has workaround])
Attachments
(1 file, 1 obsolete file)
2.66 KB,
patch
|
Dolske
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)
When trying to download a file via a https:// from a password protected directory, Firefox asks for the user name and password. After entering the user credentials, nothing more happens - also copying the link into the browsers address bar does not work.
When putting the link in the address bar after *restarting* the browser, the download is working as expected.
I already checked with CharlesProxy and it seems as if Firefox does not send any credentials (or anything else) at all to the web server after entering username/password.
The supplied URL demonstrates the problem.
Reproducible: Always
Steps to Reproduce:
1. go to the URL
2. right-click on the link -> "save file as..."
3. enter the following username and password: mozilla / test
Actual Results:
nothing happens
Expected Results:
File should be downloaded
I already talked to moo / Thomas B. Ruecker (dm8tbr@afthd.tu-darmstadt.de) on the #firefox.de IRC channel and he could reproduce the problem as well.
Comment 1•14 years ago
|
||
For completeness: here's my build ID
Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Reporter | ||
Comment 2•14 years ago
|
||
using a newly created user profile does not help either
Reporter | ||
Comment 3•14 years ago
|
||
The problem is not limited to HTTPS-URLs, but also applies to non-SSL connections, Testcase URL is http://www.ellis-events.com/download.htm, Username and password is the same.
Reporter | ||
Comment 4•14 years ago
|
||
I now checked some older Firefox versions plus the current 4.0 Beta 6:
Firefox 3.0
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.14) Gecko/2009082707 Firefox/3.0.14
and
Firefox 3.5.7
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7
behave both as expected.
The current 4.0 beta:
Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6
shows the same problem as 3.6.10
Comment 5•14 years ago
|
||
I think this is enough info for the bug to be reproducible.
Tested latest trunk.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•14 years ago
|
||
As this is obviously a regression, we could also need a regressionwindow.
Keywords: regression,
regressionwindow-wanted
Reporter | ||
Updated•14 years ago
|
Summary: Downloading of a file via HTTPS from a password protected directory fails → Downloading of a file from a password protected directory fails
Comment 7•14 years ago
|
||
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/247524e19d0c
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090720 Minefield/3.6a1pre
Fails:
http://hg.mozilla.org/mozilla-central/rev/f2a58ffcd00c
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=247524e19d0c&tochange=f2a58ffcd00c
Comment 8•14 years ago
|
||
In local build,
The following changeset causes the probrem.
d6895cb5ac3b Honza Bambas — Bug 475053 - Implement asyncPromptAuth to fix multiple password prompt overlap
Updated•14 years ago
|
Keywords: regressionwindow-wanted
Updated•14 years ago
|
Blocks: 475053
blocking2.0: --- → ?
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → Trunk
Updated•14 years ago
|
blocking1.9.2: --- → ?
Reporter | ||
Comment 9•14 years ago
|
||
If I save the user credentials for the downloaded resource, I get the following, slightly different behaviour:
- the first attempt to "save as..." shows the user/password dialog with the username and password already filled in, but the download fails.
- A second attempt (*without* closing the browser) succeeds, the file gets downloaded then.
Comment 10•14 years ago
|
||
Did this work in Firefox 3.6, or 3.5? I.e. is it only a problem in Firefox 4 betas or is this an older problem...?
Comment 11•14 years ago
|
||
This problem happens on 3.6 and 4.0b7pre.
3.5 works fine.
And See comment #4
Comment 12•14 years ago
|
||
And comment 7. I suspect the sync prompt thing is a good regression candidate here.... and that we do want to fix this on 1.9.2 if we can.
Reporter | ||
Comment 13•14 years ago
|
||
Am I right to assume that a fix probably will not make it into 3.6.11 as it is only 4 days to go until code freeze and only 3 blocking issues left?
Comment 14•14 years ago
|
||
Unless some sort of magic happens, that's probably true....
Comment 15•14 years ago
|
||
Not going to "block" a branch release (we've managed to live with it for 10 updates), but we'll look at the patch when it's done for trunk. It probably should block Firefox 4.
Comment 16•14 years ago
|
||
Honza, please see comment 8. Blocking.
Assignee: nobody → honzab.moz
blocking2.0: ? → betaN+
Assignee | ||
Comment 18•14 years ago
|
||
nsContextMenu (in its js file) in saveAs command assigns its own listener to the channel. Before nsContextMenu asyncOpens the channel it starts a timer that fires in one second and cancels the channel with NS_ERROR_SAVE_LINK_AS_TIMEOUT. The timer is canceled in the listener's OnStartRequest.
Workaround: set browser.download.saveLinkAsFilenameTimeout to 60000. It will make the code wait for one minute instead of just a single second.
Going to investigate why this worked before the patch for bug 475053.
Whiteboard: [has workaround]
Assignee | ||
Comment 19•14 years ago
|
||
Here's why: when the timer cancels the channel, nsHttpChannel::Cancel cancels wait for the auth prompt and gets OnAuthCancelled with userCancel=false. In case of userCancel=false it doesn't cancel+respin the transaction and therefor it hangs forever suspended. And, it keeps the cache entry and therefor the download is no more possible until firefox restart. nsHttpChannel needs to be patched.
However, there is also a different bug. In bug 299372 we introduced a code that waits a second for the server response to get the filename from content-disposition headers, if this times out, we do it an older way, w/o waiting for the header to do not make user wait. This new code, however, doesn't count with a pop-up of the authentication dialog. Nor it counts with many other things like url classification, async cache lookup, all that may delay OnStartRequest. Starting the timer before AsyncOpen is wrong IMO.
I have to think of some API we already have to use it to start the timer.
See Also: → 299372
Assignee | ||
Comment 20•14 years ago
|
||
Patching nsHttpChannel doesn't help to fix this. We get both the auth prompt and save dialog (from the legacy save uri system) on the screen.
Assignee | ||
Comment 21•14 years ago
|
||
Fix for both problems. Not sure of the reviewer for the nsContextMenu.js changes.
Attachment #478561 -
Flags: review?(mconnor)
Attachment #478561 -
Flags: review?(bzbarsky)
Comment 22•14 years ago
|
||
Comment on attachment 478561 [details] [diff] [review]
v1
the magic wheel of "not actually me" lands on.... dolske!
Attachment #478561 -
Flags: review?(mconnor) → review?(dolske)
Comment 23•14 years ago
|
||
Comment on attachment 478561 [details] [diff] [review]
v1
>+++ b/browser/base/content/nsContextMenu.js
...
> saveLink: function() {
> // canonical def in nsURILoader.h
> const NS_ERROR_SAVE_LINK_AS_TIMEOUT = 0x805d0020;
>+ // defined in nsISocketTransport.idl
>+ const NS_NET_STATUS_WAITING_FOR = 0x804b000a;
The previous line has the excuse that the value is in a .h (and, sadly, not in Components.results), but for the one you're adding you should just use Ci.nsISocketTransport.STATUS_WAITING_FOR.
>+ onStatus: function sLA_callbacks_onStatus(aRequest, aContext, aStatus, aHost) {
>+ if (aStatus != NS_NET_STATUS_WAITING_FOR)
>+ return;
I don't think this is right (2nd half of comment 19, too).
The problem bug 299372 was fixing (well, improving what had earlier been fixed) was to not have clicking "Save As" do nothing for a long period of time before showing UI. EG, consider being on a terrible connection where it takes a minute to resolve the host and successfully connect. AIUI, we won't get NS_NET_STATUS_WAITING_FOR until after that point, so this reverts us back to the original bug. Thus, I think we really need to start the timer immediately.
It's a little sucky to have the auth prompt come up while we're showing the Save As dialog, but I don't see an easy way around that. Followup fodder for finding a way to suppress the auth prompt until after the Save As has been responded to? I suppose we could try suspending the channel while the dialog is up (if the timeout fired)? I'm a bit wary of making non-authenticated downloads slower (because we wouldn't be slurping down data while waiting for the user), but maybe that's ok since the site is already slow to respond to the request...
I think we need a test here too. Should be testable via a .sjs that deliberately does not respond to a request?
Attachment #478561 -
Flags: review?(dolske) → review-
Assignee | ||
Comment 24•14 years ago
|
||
OK, if we do not authenticate to the site, we don't have a chance to get the proper header anyway. So, I suggest to cancel the channel and fall back to the old way when the channel asks for an auth prompt. We then can run the timer before we open the channel (what I really don't like, but understand why we need it).
Comment 25•14 years ago
|
||
Honza, what's the nsHttpChannel change (which I assume is what you want me to review) actually doing?
Assignee | ||
Comment 26•14 years ago
|
||
(In reply to comment #25)
> Honza, what's the nsHttpChannel change (which I assume is what you want me to
> review) actually doing?
The 'userCancel' argument is true when user presses the cancel button in the auth dialog (refuse to provide credentials). It is false when the queued authentication task has been canceled by something else then user, e.g. cancellation of the channel, as in our case.
However, while we wait for the auth prompt result (result here is also user or internal cancellation of the dialog) the transaction is always suspended. So, we always have to resume it after we get the result, whatever it is.
We was not doing that for internal cancellation, that was making the channel hang forever, also with the cache entry. I changed then the condition to just check the transaction is non-null.
We are apparently missing a test for this.
Assignee | ||
Comment 27•14 years ago
|
||
- the channel is canceled after it demands an auth prompt
- other remains unchanged
To preserve the file name determination even we must prompt for credentials would be to cancel the timer when the channel wants an auth prompt, provide an auth prompt, and start the timer again after a notification comes to the progress listener, whatever notification it is - the only way to figure out the credentials were provided. This is relatively complex, but if we really want it, I can have a patch and a good test for it.
I'll add a test anyway and update the patch with a test ones again.
Attachment #478561 -
Attachment is obsolete: true
Attachment #481178 -
Flags: superreview?(bzbarsky)
Attachment #481178 -
Flags: review?(dolske)
Attachment #478561 -
Flags: review?(bzbarsky)
Comment 28•14 years ago
|
||
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]
r=me for the nsContextMenu.js change.
Attachment #481178 -
Flags: review?(dolske) → review+
Comment 29•14 years ago
|
||
Attachment #481178 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 30•14 years ago
|
||
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]
http://hg.mozilla.org/mozilla-central/rev/d9fdddb0a30e
Attachment #481178 -
Attachment description: v2 → v2 [Check in comment 30]
Assignee | ||
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 31•14 years ago
|
||
Now that this is fixed (thanks to everybody involved) - what are the chances that this fix will find its way into 3.6.12 and not only 4.x?
Updated•14 years ago
|
Target Milestone: --- → mozilla2.0b8
Comment 32•14 years ago
|
||
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]
>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>@@ -953,24 +953,27 @@ nsContextMenu.prototype = {
> function callbacks() {}
> callbacks.prototype = {
> getInterface: function sLA_callbacks_getInterface(aIID) {
> if (aIID.equals(Ci.nsIAuthPrompt) || aIID.equals(Ci.nsIAuthPrompt2)) {
>- var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>- getService(Ci.nsIPromptFactory);
>- return ww.getPrompt(doc.defaultView, aIID);
>+ // If the channel demands authentication prompt, we must cancel it
>+ // because the save-as-timer would expire and cancel the channel
>+ // before we get credentials from user. Both authentication dialog
>+ // and save as dialog would appear on the screen as we fall back to
>+ // the old fashioned way after the timeout.
>+ timer.cancel();
>+ channel.cancel(NS_ERROR_SAVE_LINK_AS_TIMEOUT);
> }
> throw Cr.NS_ERROR_NO_INTERFACE;
> }
> }
Is it actually intended that this code/case now throws instead of returning (something)?
If so, I'd say it lacks an explicit comment.
Assignee | ||
Comment 33•14 years ago
|
||
(In reply to comment #32)
> Is it actually intended that this code/case now throws instead of returning
> (something)?
It's actually the fix for this bug.
> If so, I'd say it lacks an explicit comment.
And I'd say the comment is enough to explain what we are doing here, isn't it?
Comment 34•14 years ago
|
||
(In reply to comment #33)
> It's actually the fix for this bug.
Ah, unusual but if it is then fine.
Thanks for the confirmation.
> And I'd say the comment is enough to explain what we are doing here, isn't it?
Well, I know little about this code and the comment seemed related to the 2 .cancel(), not to the s/return/throw/.
I would have expected at least something like a '// fall through' to be (even more) explicit.
I guess NS_ERROR_NO_INTERFACE after succeeding .equals() confuses me by default :-|
Updated•14 years ago
|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in
before you can comment on or make changes to this bug.
Description
•