Moving to a new bug to make clearly visible all bugs coming from the same issue and the way to fix them. Implementing async prompting seems not to be that big deal. I will have a first working patch in one or two days, hopefully. CC'ing Boris Zbarsky, I will ask him for reviews on this. Christian is too busy to make this as fast as we need. The main change will be in nsHttpChannel that will use AsyncPromptAuth instead of PromptAuth and in nsLoginManagerPrompter.js where I indent to implement AsyncPromptAuth with prompt queue. I already have a lot done right now. This bug should be b1.9.1+'ed because all bugs depending already are.

Almost complete, has one major problem: when user cancels the credentials prompt dialog we do not display content of the unauthenticated page. Synchronous implementation blocks the channel processing in OnStartRequest until dialog is closed. When it is canceled by user, channel does ProcessNormal and loads content of the unauthenticated page which is then displayed to user. Asynchronous implementation lets the channel throw content of the unauthenticated page away (http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpChannel.cpp#4887). The channel processing is than cut in half of OnStopRequest (see the patch). To solve this we have to do some magic with the data, probably store them and provide later after we know user has canceled the auth prompt.

Second version, fixes the unauthorized page content display. This could be considered as the first working version. It fixes multiple master password prompts and multiple user/password auth prompts. Everything is now serialized. Now I have to write an automated test for this. Do we have any proxy authentication testing capability?

So... when do you need the review by? Comment 0 doesn't fill me with confidence that I can do it by when you need it, since I don't really know this code, so won't be able to do a particularly fast review. Certainly nothing shorter than a week, and order of a week unless I completely drop everything else (read: stop reading e-mail). Even a week means dropping most things I'm doing... Is Christian's situation even worse? Initial thoughts based on a quick skim: I assume that this fixes the bugs it's blocking (or at least some of them) because it makes the prompt service itself not put up more than one prompt at a time? It's not clear to me how that fixes the "multiple prompts for the same password" issue... am I missing something? In any case, that makes HTTP depend on an undocumented implementation detail of a particular nsIAuthPrompt2 implementation. If that's something we really want to rely on, then nsIAuthPrompt2 needs to be changed accordingly. I should also note that the code as written doesn't follow the nsIAuthPrompt2 documentation about falling back if the method is not implemented. Again, either we need an API change here (with interface rev) or we need to follow the API.

(In reply to comment #3) > So... when do you need the review by? Comment 0 doesn't fill me with > confidence that I can do it by when you need it, since I don't really know this > code, so won't be able to do a particularly fast review. Certainly nothing > shorter than a week, and order of a week unless I completely drop everything > else (read: stop reading e-mail). Even a week means dropping most things I'm > doing... Is Christian's situation even worse? > I don't know anything about his situation I just know he doesn't have that much time for the mozilla stuff. His reaction to my first patches in bug 348997 took months. Problem is that Christian is the only one that knows this code really well, he is author of it. Patches in this bug are based on that feedback few days ago. I consider this bug as urgent because of such large feedback on bugs depending. If it is not considered so, I will not push on it. However, I believe I can have a working patch including tests (I just started work on it) in few days. If anyone has serious doubts this can get to 1.9.1 or some of the fix 3.1.X releases, tell me please. > Initial thoughts based on a quick skim: > > I assume that this fixes the bugs it's blocking (or at least some of them) > because it makes the prompt service itself not put up more than one prompt at a > time? Exactly, when you start Firefox having saved a session with pages requiring authentication screen is been blown up with tuns of auth dialogs. > It's not clear to me how that fixes the "multiple prompts for the same > password" issue... am I missing something? This patch makes http channel use the async prompt. Password manager now implements the async prompt method using a queue of prompt requests, duplicate requests are bound to already existing ones. Queue is then processed one by one, only a single dialog is pop-up at a time including the master password prompt. > In any case, that makes HTTP depend > on an undocumented implementation detail of a particular nsIAuthPrompt2 > implementation. If that's something we really want to rely on, then > nsIAuthPrompt2 needs to be changed accordingly. I find that interface perfectly fit my needs. > I should also note that the > code as written doesn't follow the nsIAuthPrompt2 documentation about falling > back if the method is not implemented. Again, either we need an API change > here (with interface rev) or we need to follow the API. Ups, I didn't notice. Anyway, the http channel is changed to be able to fallback to synchronous version easily. I use NS_ERROR_IN_PROGRESS to indicate we auth asyncly (there is no NS_SUCCESS_IN_PROGRESS, so far). Using the sync API and indicate NS_OK will make the channel use the previous implementation unchanged.

> Problem is that Christian is the only one that knows this code really > well, he is author of it. Right. > If it is not considered so, I will not push on it I agree it's reasonably urgent. So's everything else I'm working on or planning to work on in the next week or two, unfortunately... So again, what's the timeframe by which this needs to be landing on trunk? > However, I believe I can have a working patch including tests (I just started > work on it) in few days. So should I wait until then to review? That would certainly be a much more efficient use of my time than reviewing in-flux code... > If anyone has serious doubts this can get to 1.9.1 or some of the fix 3.1.X > releases, tell me please. There's no way an API change like that can happen in a 1.9.1.x release. It _might_ be able to happen in 1.9.1 if we're pretty sure that this API is not used in extensions and such (probably a good assumption, I suspect). > duplicate requests are bound to already existing ones OK, so the service is not only handling serialization of the prompts but also duplicate prompt elimination. Gotcha. That does make sense, since it means the various consumers don't need to do all that themselves. That said, it feels like the coalescing behavior needs to be configurable by the caller, at first blush. I'd have to read a lot more carefully than I've had time to so far to see whether there's a good reason that it doesn't. > I find that interface perfectly fit my needs. No. You find that your particular implementation of that interface suits your needs. I see nothing in the interface that requires the implementation to behave that way (e.g., where is coalescing of requests mentioned in the interface, nor is serialization of user-facing prompts mentioned). If the interface requires such behavior on the part of its implementors, then that needs to be VERY clearly documented in the IDL, and the IID needs to be changed (since you are in fact just creating a new method that does something totally different from the one currently on the interface and simply giving it the same name). As long as you're doing all that you might as well require implementations of this interface to implement this method, at least on trunk. Whether you do that on branch or not is a matter of how much compat hit we're willing to take here. The right course for branch might in fact be an nsIAuthPrompt2_1_9_1_BRANCH or some such, given the above...

(In reply to comment #5) > I agree it's reasonably urgent. So's everything else I'm working on or > planning to work on in the next week or two, unfortunately... So again, what's > the timeframe by which this needs to be landing on trunk? > If this has to land on 1.9.1 before first release, because of the API change, then it should be before trunk restriction for 1.9.1 blockers is over. It is best time to land this. > So should I wait until then to review? That would certainly be a much more > efficient use of my time than reviewing in-flux code... > Probably yes, I did r? more to get your attention and opinion than a regular review, what I did get, thanks. From your feedback it looks like I am on the right path. > There's no way an API change like that can happen in a 1.9.1.x release. It > _might_ be able to happen in 1.9.1 if we're pretty sure that this API is not > used in extensions and such (probably a good assumption, I suspect). > > That said, it feels like the coalescing behavior needs to be configurable by > the caller, at first blush. I'd have to read a lot more carefully than I've > had time to so far to see whether there's a good reason that it doesn't. > > > I find that interface perfectly fit my needs. > > No. You find that your particular implementation of that interface suits your > needs. I see nothing in the interface that requires the implementation to > behave that way (e.g., where is coalescing of requests mentioned in the > interface, nor is serialization of user-facing prompts mentioned). If the > interface requires such behavior on the part of its implementors, then that > needs to be VERY clearly documented in the IDL, and the IID needs to be changed > (since you are in fact just creating a new method that does something totally > different from the one currently on the interface and simply giving it the same > name). > > As long as you're doing all that you might as well require implementations of > this interface to implement this method, at least on trunk. Whether you do > that on branch or not is a matter of how much compat hit we're willing to take > here. The right course for branch might in fact be an > nsIAuthPrompt2_1_9_1_BRANCH or some such, given the above... Agree. So, for 1.9.1 to have a new interface but leave the method name unchanged, just change the IDL documentation. For trunk just change IID of the current interface + documentation. Or, what about new nsIAuthPromptCoalescent or nsIAsyncAuthPrompt or similar sound name interface?

> then it should be before trunk restriction for 1.9.1 blockers is over. That doesn't answer my question. I'm looking for a date here. Please coordinate with 1.9.1 drivers as needed? I'd rather not create a new interface on trunk; just leave the method where it is. One thought I had this morning. This patch coalesces all auth requests for the same info, right? And serializes all prompts? The former is probably good. The latter I'm not sure about. Would it make more sense to serialize prompts on a per-window basis, not a whole-app basis?

(In reply to comment #7) I will ask driver for concrete dates. For trunk let's change the documentation and IID, for 1.9.1 create a new interface as you suggested (with _1_9_1 appending). > One thought I had this morning. This patch coalesces all auth requests for the > same info, right? Yes, for the same key used to pick already existing login credentials from the database. > And serializes all prompts? It does, for the whole application. I originally had it per-window, but a 'window' here means a 'tab'. So, finally the result was almost the same as w/o this patch - when 10 tabs needing auth info, 10 dialogs appear, hidden each behind other, also it would need more code to make the master password prompt not to be duplicated. I feel the way I do it is more natural, for me.

> but a 'window' here means a 'tab'. Well, you'd just need to go from the window you're handed to a toplevel browser window or something else that corresponds with one. That shouldn't be a problem, I would think. So the question is whether serializing per-toplevel browser window would make more sense, especially when the windows are scattered over multiple desktops (e.g. Spaces on Mac or equivalent on Linux).

It's probably worth getting a ui-review on that part, I guess. As far as dates go, I don't need to-the-day stuff, but it would be nice to have an idea of whether I need to make time this week, or whether next Monday is ok, say. That depends on both the ship schedule and when you think you'll have a final patch ready...

Update - I have a working patch with test for prompt serialization and changes in nsHTTPChannel, needs cleanup and update to have a per-top-level window queue context as bz suggested yet. Problem is that I found a leak, definitely not caused by my patch (can reproduce using the new test for this bug w/o the code changes applied). When using cached proxy auth credentials the client-to-proxy connection gets NS_HTTP_STICKY_CONNECTION caps flag in nsHttpChannel::DoAuthRetry even the proxy connection is HTTP/1.0 w/o any keep alive headers. This makes the connection to be reused but immediately closed on reuse. Under these circumstances it then leaks, don't know why. To explain: I use fact we have PAC function in mochitest profile. So, I enhanced authenticate.sjs from password manager tests to emulate need for Proxy-Auth. httpd.js currently doesn't support keep-alive connections at all.

Bug 459620 describes the leak from comment 11.

(In reply to comment #9) > So the question is whether serializing per-toplevel > browser window would make more sense, especially when the windows are scattered > over multiple desktops (e.g. Spaces on Mac or equivalent on Linux). Hmm. I think that the prompt should be consolidated per-app, and not per-window. Otherwise, for the common case of having multiple windows on one desktop, users would still be wondering why they're getting multiple prompts for the same thing. It's an interesting problem, though, in that it's not very nice to have an action on one desktop being held up by a prompt on some other desktop (which you might not notice). But I'd suspect the answer to that problem is not having multiple prompts, but a better way of calling attention to that other desktop... Switching to that desktop, blinking an icon, etc. Or maybe close the old prompt, and reopen it for the current window? I'd suggest that we try and disentangle that problem, and deal with it in a followup. Even if we don't fix it in time for 3.1, I think we'd still be much better off by having this multiple-prompt bug fixed, even if it has some quirks when users have windows spread across desktops.

> users would still be wondering why they're getting multiple prompts > for the same thing. The consolidation by thing happens first. So you will see at most one prompt for any given credentials.

This scares the pants off of me, I really wouldn't want to ship this w/o at least one beta...

First "beta". In general: nsLoginManagerPrompter.js implements asyncPromptAuth and is responsible for coalescing and serialization of all prompts including master password prompt, proxy authentication and web authentication. nsHttpChannel is using this method now and expects to get credentials via callback. To nsHttpChannel changes: Because the prompt is async we must read data of the unauthenticated page content and save it in a pipe. If user later cancels the prompt dialog, we display this content using a pump based on the pipe, otherwise the data is threw away. This is solution w/o suspending the transaction. http channel is able to fall back to synchronous prompt (the previous implementation) when the async method is not implemented or otherwise fails. The leak mentioned is fixed, so far, by avoiding of reuse of non-keep-alive connection for auth retry. I would like to fix this in the conn manager where the bug really is, but it would take much more time. We can later properly fix bug 459620, would that be ok? To login manager changes: There is a global (per-app) queue that groups the same prompts together (binds callbacks to identical prompts by 1..n relation) and global flag used to hold invocation of more then one prompt at a time. The prompt queue is cleaned up when application shuts down. Still to do: Change IID and documentation of the nsIAuthPrompt2 interface for trunk and create 1.9.1 patch with new _1_9_1 interface to preserve compatibility.

Somewhat offtopic: could/should this be also used for cookie permission dialogs? Including making sure that there is at most one cookie dialog (at least for a specific host) at a time?

Aleksey: not as part of this bug, feel free to file a follow up.

Comment on attachment 359193 [details] [diff] [review] v0.9 Please see comment 13 for details on UX changes by this patch.

(In reply to comment #13) > Hmm. I think that the prompt should be consolidated per-app, and not > per-window. Otherwise, for the common case of having multiple windows on one > desktop, users would still be wondering why they're getting multiple prompts > for the same thing. > > It's an interesting problem, though, in that it's not very nice to have an > action on one desktop being held up by a prompt on some other desktop (which > you might not notice). But I'd suspect the answer to that problem is not > having multiple prompts, but a better way of calling attention to that other > desktop... Switching to that desktop, blinking an icon, etc. Or maybe close the > old prompt, and reopen it for the current window? > > I'd suggest that we try and disentangle that problem, and deal with it in a > followup. Even if we don't fix it in time for 3.1, I think we'd still be much > better off by having this multiple-prompt bug fixed, even if it has some quirks > when users have windows spread across desktops. Yeah, playing nicely on multiple desktops is an interesting topic and something we should file a follow up about if we're concerned, but I agree that it will still "feel broken" if Firefox asks for the same password multiple times, versus just coalescing them all and dealing with multi-desktop users separately.

> if Firefox asks for the same password multiple times This won't happen. See comment 14.

Oop - sorry bz, missed that.

Mike, according to bug 348997 please look at THIS bug.

I don't think this blocks, but it would be super nice to have. I'll loop back around for the ui-review, but I don't think I understand what the UX implications of the patch are based on reading comment 13. Can someone spell it out for me?

Comment on attachment 359193 [details] [diff] [review] v0.9 Christian, will you find time to take a look?

(In reply to comment #24) You know the current behavior, we bloat user with password prompts, even duplicated. The patch consolidates prompts to always pop just a single master password prompt, a single prompt for any proxy server authentication and a single prompt for identical web requests authentication. Prompts are also serialized to always show just a single prompt at a time, there is a queue. The queue is per the whole application. Prompt is shown over the window that requests the authentication, this has been left unchanged. Bz objected that having the queue per-application might cause problems when windows are spread across multiple desktops. Prompt being shown is still bound to a window that does the request. When such a prompt hangs on a currently hidden desktop, no authentication is possible in any other window, because the queue is stuck by that, currently hidden, prompt. My test with centos5 shows that session restore doesn't remember on which desktop each window was the last time, and opens all on the current desktop. So, the problem bz mentioned should not appear that often, it won't appear after start and when user opens a window he'll get the authentication prompt very soon, probably before he switches to a different desktop.

> session restore doesn't remember on which desktop each window was the last time That's a bug, it's filed, and I am eagerly awaiting the fix. It would sure help my life tremendously (on Mac). I don't think we want whoever's fixing that to have to revisit this code...

Comment on attachment 359193 [details] [diff] [review] v0.9 I tend to agree with Johnath in comment #20. Let's fix the problem for the majority of users, and then deal with multi-screen Linux users as a follow up. Generally, though, the idea of coalescing requests for Password Manager into a single request seems like the right UI. Don't even think that needs a r :)

Complete trunk patch, includes fix for handling multiple authentication challenges when user cancels the auth prompt and changes nsIAuthPrompt2 IDL comment and IID. For details see comment #16. The test now requires bug 476302.

Comment on attachment 359193 [details] [diff] [review] v0.9 Bah, you midaired my review of the v0.9 patch. :) >--- a/netwerk/protocol/http/src/nsHttpChannel.cpp ... >+ else if (rv == NS_ERROR_IN_PROGRESS) { >+ // authentication prompt has been invoked and result >+ // is expected asynchronously >+ // safe the authenticator for use later in OnAuthAvailable >+ // and now immediately return >+ mAuthenticator = auth; >+ return rv; >+ } s/safe/save, and formatting is a bit off. >+ rv = GetCurrentPath(path); >+ if (NS_FAILED(rv)) return rv; NS_ENSURE_SUCCESS()? >+ rv = authPrompt->AsyncPromptAuth() >+ >+ if (NS_SUCCEEDED(rv)) { ... >+ } >+ else { >+ // Fall back to synchronous prompt I wonder if the else path should have a more explicit error check (NS_ERROR_NO_INTERFACE)... I'd be slightly worried having a failure mode where we show one prompt, it returns NS_ERROR_FOO for some reason, and then we show another prompt. Also, is this patch complete? AFAICT there isn't a AsyncPromptAuth implementation in the tree that does anything -- specifically, nsPrompt and nsPromptService -- so I'm not sure how the password manager actually ends up invoked. [Our prompting is ridiculously complicated, so I wouldn't be surprised if it somehow did. I wrote up some notes at https://wiki.mozilla.org/User:Dolske/PromptRework a ways back, and it's hard keeping things straight!] >+NS_IMETHODIMP nsHttpChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo) Using a pipe for the notification seems a little odd... The network layer already caches auth credentials (nsIHttpAuthManager), so I'd naively expect to the notification to just be an alert to check there. But I'm not really familiar with the network code, so maybe this is crazy talk. >--- a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js ... >+function LoginManagerPromptFactory() >+{ >+ var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); >+ observerService.addObserver(this, "quit-application-granted", true); >+} Nit: style here is: var foo = Cc["xxx"]. getService(xxx); >+ if (topic == "quit-application-granted") { >+ for (p in this._asyncPrompts) { >+ asyncPrompt = this._asyncPrompts[p]; For patterns like this just use "for each (let asyncPrompt in xxx)". >+ for (b in asyncPrompt.binding) >+ { Nit: brace style (here and elsewhere) -- opening brace is never on a new line. Also, please avoid 1-character variables. >- asyncPromptAuth : function () { >- return NS_ERROR_NOT_IMPLEMENTED; >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { It seems like the async parts should, ideally, live over in nsPromptService and basically wrap a synchronous promptAuth call... The login manager doesn't really need to be involved with the synchronicity aspect. [Now you have The Police stuck playing in your head. :)] EG, the prompt service's asyncPromptAuth would decide if there's already a similar promptAuth call in progress (in which case is sets things up to wait for the result), or it wraps a new promptAuth call. I'm not sure how big of a change doing this would be; it would be great to do it this way, but layering/modularity of the prompt stuff is currently rather odd. >+ _doAsyncPrompt : function() >+ { I didn't look closely at this yet. >+ _newAsyncPromptBinding : function(aCallback, aContext) { >+ function Binding(aCallback, aContext) { >+ this.callback = aCallback; >+ this.context = aContext; >+ } >+ >+ Binding.prototype = { >+ callback : null, >+ context : null, >+ cancel : function() { >+ this.callback.onAuthCancelled(this.context, false); >+ this.callback = null; >+ this.context = null; >+ } >+ } >+ >+ return new Binding(aCallback, aContext); I don't think this is needed, there's only one caller, so it can just do: var myNewBinding = { callback : foo, context : bar, cancel : function() { baz }} Nit: "binding" is a little too generic of a name. >+++ b/toolkit/components/passwordmgr/test/475053frame.html ... >+ <iframe id="iframe1" src="http://example.com/tests/toolkit/components/passwordmgr/test/authenticate.sjs?r=1&user=user3name&pass=user3pass&realm=mochirealm3&proxy_user=proxy_user2&proxy_pass=proxy_pass2&proxy_realm=proxy_realm2"></iframe> Maybe just initially load these all w/o a src attribute (empty iframes), so that the top level test is explicitly driving all the various combinations of HTTP/proxy auth? You might also want to ensure that the end of the test clears the HTTP auth cache (specifically, the proxy auth -- hope that's possible!) so that we don't try to keep authenticating to the mochitest proxy in the tests that run later. >+</body> >+</html> >diff --git a/toolkit/components/passwordmgr/test/Makefile.in b/toolkit/components/passwordmgr/test/Makefile.in >--- a/toolkit/components/passwordmgr/test/Makefile.in ... >+ 475053frame.html \ >+ test_bug_475053.html \ Nit: Call these subtst_prompt_async.html and test_prompt_async.html. The 475053frame.html one should be under MOCHI_CONTENT. >+++ b/toolkit/components/passwordmgr/test/test_bug_475053.html ... >+ iframe1.src = "http://example.com/tests/toolkit/components/passwordmgr/test/authenticate.sjs?"+ Add a "/tests/toolkit/components/passwordmgr/test/authenticate.sjs" variable to this doesn't have to be repeated everywhere. >+ case 5: >+ // Same as the previous two steps but let the server generate >+ // huge content load to check http channel is capable of shadow >+ // load of the unauthenticated page content and display it when >+ // user cancels the prompt dialog before page has been loaded. What's this doing? Making the test less likely to accidentally pass if everything happens in one shot?

(In reply to comment #14) > The consolidation by thing happens first. So you will see at most one prompt > for any given credentials. Ok, I think my comment 13 was answering the wrong question. :) I didn't get the distinction between "consolidation" and "serialization". There are two things going on in this patch... 1) Consolidation of multiple prompts for the same auth info. I think there's not much question about this; we shouldn't show multiple prompts asking for the same info. 2) Serialization of multiple prompts, so that when we *do* need to ask for different auth info (eg, HTTP Auth for site1.com and site2.org), we only show 1 prompt at a time. #1 alone makes things better. Users will generally get just 1 proxy auth prompt, and 1 HTTP auth per requesting site. So, the multiple (but different) HTTP auth prompts would still trigger multiple master password prompts, but at least the quantity would be lower. #2 is wallpaper for the problem with multiple master password prompts. The first HTTP auth prompt triggers the MP prompt, and the 2nd HTTP auth prompt wouldn't be shown until the MP has been entered. But if you had, say, a few extensions that were accessing logins from the password manager directly (ie, no HTTP auth involved, like Weave), then you would still get multiple MP prompts. Not perfect, but helps 95% of the problem. I think per-app serialization for #2 is fine for now -- we're in a much better place than we are today. In the long run: * Make a similar fix in PSM so that multiple master password prompting doesn't happen (either a new async API, or just consolidate and block callers). * Continue to have proxy-auth and master-password consolidated per-app,

(In reply to comment #30) > (From update of attachment 359193 [details] [diff] [review]) > Bah, you midaired my review of the v0.9 patch. :) > Sorry :D According to your comments not much has changed in the new patch. > >+ rv = GetCurrentPath(path); > >+ if (NS_FAILED(rv)) return rv; > > NS_ENSURE_SUCCESS()? The style is if (NS_FAILED(rv)) return rv; to avoid bloat with logs. I keep it as it was. > > >+ rv = authPrompt->AsyncPromptAuth() > >+ > >+ if (NS_SUCCEEDED(rv)) { > ... > >+ } > >+ else { > >+ // Fall back to synchronous prompt > > I wonder if the else path should have a more explicit error check > (NS_ERROR_NO_INTERFACE)... I'd be slightly worried having a failure mode where > we show one prompt, it returns NS_ERROR_FOO for some reason, and then we show > another prompt. > It is good point, but I cannot imagine (from my experience) that such api would show a dialog and then synchronously failed. The purpose and definition of the method is to show the dialog asynchronously. If that fails, tell the consumer by callback about that failure - as the patch does. If fails to queue the prompt fail synchronously and then in any case fallback to sync api. > Also, is this patch complete? AFAICT there isn't a AsyncPromptAuth > implementation in the tree that does anything -- specifically, nsPrompt and > nsPromptService -- so I'm not sure how the password manager actually ends up > invoked. [Our prompting is ridiculously complicated, so I wouldn't be surprised > if it somehow did. I wrote up some notes at > https://wiki.mozilla.org/User:Dolske/PromptRework a ways back, and it's hard > keeping things straight!] > Good to know this document exists. However, as I studied the code the prompt requests bubbles through prompt service to the prompter bound to the window, that is the password manager. > >+NS_IMETHODIMP nsHttpChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo) > > Using a pipe for the notification seems a little odd... The network layer > already caches auth credentials (nsIHttpAuthManager), so I'd naively expect to > the notification to just be an alert to check there. But I'm not really > familiar with the network code, so maybe this is crazy talk. > No, the pipe is used to store content of the page that server returns with 401/407 response codes. It is in common cases an explanatory or fallback content that we must show to the user. > >+ if (topic == "quit-application-granted") { > > >+ for (p in this._asyncPrompts) { > >+ asyncPrompt = this._asyncPrompts[p]; > > For patterns like this just use "for each (let asyncPrompt in xxx)". > I actually search just for the first item in the array... I should use _asyncPrompts = new Array() and use shift() for this. > >- asyncPromptAuth : function () { > >- return NS_ERROR_NOT_IMPLEMENTED; > >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { > > It seems like the async parts should, ideally, live over in nsPromptService and > basically wrap a synchronous promptAuth call... The login manager doesn't > really need to be involved with the synchronicity aspect. [Now you have The > Police stuck playing in your head. :)] EG, the prompt service's asyncPromptAuth > would decide if there's already a similar promptAuth call in progress (in which > case is sets things up to wait for the result), or it wraps a new promptAuth > call. > The password manager does so. > I'm not sure how big of a change doing this would be; it would be great to do > it this way, but layering/modularity of the prompt stuff is currently rather > odd. > I have to take a look at this again to have concrete arguments why I did it this way or find another way. However, making the async things in JS seems easier than in C. > >+ _doAsyncPrompt : function() > >+ { > > I didn't look closely at this yet. > You should, it is the most important part :D > > >+ _newAsyncPromptBinding : function(aCallback, aContext) { > >+ function Binding(aCallback, aContext) { > >+ this.callback = aCallback; > >+ this.context = aContext; > >+ } > >+ > >+ Binding.prototype = { > >+ callback : null, > >+ context : null, > >+ cancel : function() { > >+ this.callback.onAuthCancelled(this.context, false); > >+ this.callback = null; > >+ this.context = null; > >+ } > >+ } > >+ > >+ return new Binding(aCallback, aContext); > > I don't think this is needed, there's only one caller, so it can just do: > > var myNewBinding = { callback : foo, context : bar, cancel : function() { baz > }} > > Nit: "binding" is a little too generic of a name. > Yeah, I'll do that. > >+++ b/toolkit/components/passwordmgr/test/475053frame.html > ... > >+ <iframe id="iframe1" src="http://example.com/tests/toolkit/components/passwordmgr/test/authenticate.sjs?r=1&user=user3name&pass=user3pass&realm=mochirealm3&proxy_user=proxy_user2&proxy_pass=proxy_pass2&proxy_realm=proxy_realm2"></iframe> > > Maybe just initially load these all w/o a src attribute (empty iframes), so > that the top level test is explicitly driving all the various combinations of > HTTP/proxy auth? > It would make my test framework more complicated, and there is no need to have this that generic IMHO. > You might also want to ensure that the end of the test clears the HTTP auth > cache (specifically, the proxy auth -- hope that's possible!) so that we don't > try to keep authenticating to the mochitest proxy in the tests that run later. > Good point! I have to do that. > >+ case 5: > >+ // Same as the previous two steps but let the server generate > >+ // huge content load to check http channel is capable of shadow > >+ // load of the unauthenticated page content and display it when > >+ // user cancels the prompt dialog before page has been loaded. > > What's this doing? Making the test less likely to accidentally pass if > everything happens in one shot? This is the most important stability test. It ensures that the unauthenticated page data is successfully and unbroken stored and repopulated to and from the pipe when user accepts/cancels the prompt sooner then this data is loaded from the server. Quit uncommon case but might happen in slow wireless nets or when the unauthenticated page is quite huge. The authenticate.sjs was enhanced to generate 1MB of data, on demand. It is highly unlikely that it will be read by necko before the prompt dialog is closed (there is 10ms ticker).

Crap, I did something to post my last comment before finishing. :-( In the long run, consider: * Make a similar fix in PSM so that multiple master password prompting doesn't happen (either a new async API, or just consolidate and block callers). * Figure out how to deal with per-app consolidation of proxy-auth/MP, when multiple desktops are involved. Maybe show a prompt on each desktop that requests it, and magically dismiss them all when any one is filled in? * Change HTTP Auth to be non-modal and serialized per-tab. HTTP auth shouldn't block switching to other tabs (user annoyance). This presents a challenge for logging into all your tabs after a session restore... Should we automatically cycle to the next tab needing auth? Should tabs blink to indicate they have a tab-modal prompt inside them? Anyway, I think these are longer-term problems, and don't seem to block progress for today. Oh, one other factoid. Eventually we would like to have optional automatic HTTP login (bug 223636). This means the consolidation/serialization implementation should eventually be able to give a request to password manager (which might provide a login automatically), without the serialization blocking on some unrelated prompt.

> Oh, one other factoid. Eventually we would like to have optional automatic HTTP > login (bug 223636). This means the consolidation/serialization implementation > should eventually be able to give a request to password manager (which might > provide a login automatically), without the serialization blocking on some > unrelated prompt. Good to know. That would require rewrite of the serialization code. We have to block on the master password prompt (that is invoked by lookup of existing logins). But serialization itself would have to begin after the login data was know. It would imo require to invoke the master password manually (directly) on the first auth request and block on it, then look existing logins up freely, in parallel. But its just a first tough...

I thought #2 was to deal with a blizzard of prompts all popping up in the user's face at startup (and in particular, multiple modal prompts parented to the same window, on OSes which allow that). If it also helps with the password manager, great. But if the patch already handles per-window serialization, I think we should go with it instead of asking for it to be dropped...

(In reply to comment #35) > I thought #2 was to deal with a blizzard of prompts all popping up in the > user's face at startup That too, yes. I'm ok with the serialization in the patch. It doesn't get us to a perfectionist nirvana, but it's a huge improvement on where we are today.

(In reply to comment #30) > It seems like the async parts should, ideally, live over in nsPromptService and > basically wrap a synchronous promptAuth call... The login manager doesn't > really need to be involved with the synchronicity aspect. [Now you have The > Police stuck playing in your head. :)] EG, the prompt service's asyncPromptAuth > would decide if there's already a similar promptAuth call in progress (in which > case is sets things up to wait for the result), or it wraps a new promptAuth > call. > > I'm not sure how big of a change doing this would be; it would be great to do > it this way, but layering/modularity of the prompt stuff is currently rather > odd. > What you describe here https://wiki.mozilla.org/User:Dolske/PromptRework is absolutely correct, this is how Firefox currently invokes auth prompt: nsIAuthPromptProvider* provider = nsHttpChannel->mCallbacks->GetInterface(nsIAuthPromptProvider) returns QueryInterface(nsIAuthPromptProvider) what is nsDocShell. [If this fails we fall back to AuthPromptWrapper instance (NS_QueryAuthPrompt2 -> NS_WrapAuthPrompt)] nsIAuthPrompt2* prompt = provider [nsDocShell] ->GetAuthPrompt() delegates to ww service->GetPrompt(mWindow) delegates to authpromptfactory service ->GetPrompt(aWindow) returns new instance of LoginManagerPrompter bound to the window. [When there is not any authpromptfactory registered ww service creates new instance of nsPrompt that delegates its AsyncPromptAuth to nsPromptService (that doesn't implement it). This also means there is no single sign on.] LoginManagerPrompter implements nsIAuthPrompt2 and we directly call its AsyncPrompAuth method from nsHttpChannel. To address: 1) We may change the get-provider code path and with it make all the prompt chaos cleaner. That makes this task go over its scope and wipes potential chance to get this to 1.9.1 out. 2) There is nsPromptService::AsyncPromptAuth method but from a different interface then nsIAuthPrompt2 with a different signature. We could move the coalescing/serialization code there and from LoginManagerPrompter delegate to the prompt service, but we cannot fill aCheckLabel and aCheckValue arguments. We know their values after we look existing logins up what is after we invoke MP prompt. Then, from nsPromptService we have to call back to LoginManagerPrompter because nsPromptService is not aware of saved logins. To find it, we would have to do some GI-QI magic again as http channel does. All this is doable.

> Because the prompt is async we must read data of the unauthenticated > page content and save it in a pipe. If user later cancels the prompt > dialog, we display this content using a pump based on the pipe, > otherwise the data is threw away. This is solution w/o suspending > the transaction. Is it always safe to assume that loading the unauthenticated page is OK to do? Most sites will just give you an "unauthorized" page, but I'm wondering if some more paranoid sites might treat auth failures as something sinister going on. We don't want to get anyone locked out of a site, etc. It doesn't sound like a big deal, but performing a spurious unauthorized request just seems slightly kludgy.

First load of a page that requires auth gets the unauth result (which is how we know it needs auth, if we haven't been there before) -- would be very surprised if someone got locked out for loading it unauthed (different from incorrectly authed).

Test...

Test...

Test...comment

Test comments by seema dated 27/2/2009 12.52 pm.

Seema, Jacob: This is a live bugzilla production installation, not your personal testing ground. Please read https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and/or use https://landfill.bugzilla.org/ for testing.

My bug 483599 got moved here. I agree that prompts should never overlap, but why do I get multiple master password prompts in Firefox 3.1b3? There should be just one! That would obviate this bug.

I just talked to dolske who said he could get to his review soon, but he was pretty worried about taking it in 191 in much the same way that jst is up in comment 15. Boris - any chance you could take biesi's part of this review, if he can't get to it? It's not a 191-blocker, but it sure is a desperately want-to-have - and 191 can't happen until central does.

Comment on attachment 359946 [details] [diff] [review] v1 Stealing review from biesi

bz: as per comment 15 and comment 19, if we want to get this, we'll need to get it in today. Doable? Dolske: you, too?

Also, is this actually blocked on the leak from bug 459620?

Comment on attachment 359946 [details] [diff] [review] v1 + rv = NS_NewPipe(getter_AddRefs(mUnauthenticatedDataInputStream), + getter_AddRefs(mUnauthenticatedDataOutputStream), + NET_DEFAULT_SEGMENT_SIZE, PR_UINT32_MAX); Couldn't you just suspend the transaction pump? (also a storage stream may be slightly better than a pipe) + PRUint32 writen; typo + // This is so far workaround to fix leak when reusing unpersistent + // connection for authentication retry. + if (conn && !conn->IsPersistent()) + conn = nsnull; why would that cause a leak? it's a local variable, the connection should be released when the function returns, right?

Note that if you do suspend the pump, I think quite a few changes to nsHttpChannel become unnecessary, e.g. no need to change OnStopRequest.

Comment on attachment 359946 [details] [diff] [review] v1 >+++ b/netwerk/base/public/nsIAuthPrompt2.idl >+[scriptable, uuid(651395EB-8612-4876-8AC0-A88D4DCE9E1E)] Please make sure to adjust the patch to not change this interface if you decide to try to land it on branch. >+ * This implementation MAY be responsible for 1) coalescence of identical >+ * prompts, that means prompts that would require user to enter identical >+ * data more then ones, while these data would already be available from >+ * cache anyway, show just ones but invoke appropriate callbacks for all >+ * requests made through call to asyncPromptAuth method and 2) serialization >+ * of prompts in context of the application or as otherwise user-friendly >+ * defined context. How about: This implementation may: 1) Coalesce identical prompts. This means prompts that are guaranteed to want the same auth information from the user. A single prompt will be shown; then the callbacks for all the coalesced prompts will be notified with the resulting auth information. 2) Serialize prompts that are all in the same "context" (this might mean application-wide, for a given window, or something else depending on the user interface) so that the user is not deluged with prompts. >+++ b/netwerk/protocol/http/src/nsHttpChannel.h Could you please document the new member variables and functions you're adding to nsHttpChannel? It's not clear to me what some of them are supposed to do. In particular, for the methods, what do they do and when should they be called? For the variables, what do they mean and when can one expect them to be set? Also, some of the methods (new and existing) now seem to have magic return values that mean certain things and are not documented. Also, some of the methods have in params, out params, maybe inout params. Document which is which. >+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp >@@ -103,35 +104,38 @@ nsHttpChannel::nsHttpChannel() You don't initialize the membes here in the order in which they're declared. Please fix that. >@@ -3119,37 +3129,84 @@ nsHttpChannel::GetCredentials(const char >+ // safe the authenticator, current challenge being processed s/safe/save/ I'm not sure what "the authenticator" is here, but you don't seem to be saving it. >@@ -3462,37 +3500,201 @@ nsHttpChannel::PromptForIdentity(PRUint3 >+ if (NS_SUCCEEDED(rv)) { So... Is it documented somewhere that AsyncPromptAuth() can throw to trigger fallback to sync prompt? It used to say so in the idl, but you removed that... >+ if (!mAuthRetryPending) { Why is this the right thing to check? >+ // create the unauthenticated data pipe only when we process >+ // the first challenge from the list we obtained >+ // otherwise this would prevent call of ContinueOnStopRequest >+ // from OnAuth* callbacks. And if this is an answer to the above question, please move it to _before_ the check. >+ mAsyncAuthContentBackup = PR_TRUE; So why is this boolean needed? Is it not equivalent to mUnauthenticatedDataInputStream && mUnauthenticatedDataOutputStream ? If not, why not? >+ mAsyncAuthDialogInProgress = PR_TRUE; How does this differ from mAuthRetryPending? (This should be documented in the header, not answered in the bug.) >+ rv = authPrompt->PromptAuth(this, >+ level, >+ holder, &retval); Fix the weird line-breaking here? >+NS_IMETHODIMP nsHttpChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo) Line-break after "aContext," please. And maybe don't name it if you don't use it? Just do |nsISupports* /* unused */,| instead. There's a lot of copy/paste from GetCredentialsForChallenge here, but subtly different (e.g. missing the whole identityInvalid hunk). Why does it need to be different? Can we refactor things to share that code? >+NS_IMETHODIMP nsHttpChannel::OnAuthCancelled(nsISupports *aContext, PRBool userCancel) >+ rv = GetCredentials(mRemainingChallenges.get(), mProxyAuth, creds); >+ if (NS_SUCCEEDED(rv)) { >+ mRemainingChallenges.Truncate(); >+ return ContinueOnAuthAvailable(creds); >+ } >+ else if (rv == NS_ERROR_IN_PROGRESS) { >+ return NS_OK; >+ } I think those conditions could use some explanation in terms of what we know GetCredentials managed to accomplish... >+ // ASYNC_AUTH_DIALOG_IN_PROGRESS flag here What's that? >+ // with OnStopRequets now OnStopRequest? >+ return NS_OK; Why do we not need to do any cleanup of mAsyncAuthDialogInProgress, at least, in the !userCancel case? >+nsHttpChannel::ContinueOnAuthAvailable(const nsCSubstring& creds) >+ // because we just received credentials to re-authenticate set >+ // drop both async auth flags because we don't need from now saving >+ // the unauthenticated page content data, it won't be displayed anyway I'm really not sure what this comment is trying to say... >+ // drop all remaining challenges to process, we don't need them, because >+ // now the request continues and if server or proxy responses with failure >+ // again we got new challenge list How about: Drop our remaining list of challenges. We don't need them, because we have now authenticated against a challenge and will be sending that information to the server (or proxy). If it doesn't accept our authentication it'll respond with failure and resend the challenge list. >+ if (mUnauthenticatedDataInputStream) >+ mUnauthenticatedDataInputStream = nsnull; >+ if (mUnauthenticatedDataPump) >+ mUnauthenticatedDataPump->Cancel(status); I haven't reviewed the details of this pipe stuff, because it seems like biesi's suggestion of suspending the pump should work. If it doesn't, let me know and I'll review the (well documented by that point, I hope) code. >+++ b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js >+ observe : function (subject, topic, data) { >+ if (topic == "quit-application-granted") { >+ for (p in this._asyncPrompts) { >+ asyncPrompt = this._asyncPrompts[p]; >+ delete this._asyncPrompts[p]; >+ >+ for (b in asyncPrompt.binding) >+ { >+ var binding = asyncPrompt.binding[b]; How about: var asyncPrompts = this._asyncPrompts; this._asyncPrompts = {}; for each (asyncPrompt in asyncPrompts) { for each (binding in asyncPrompt) { // stuff here } } >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { This seems to share a lot of code with the sync prompt case. Can we avoid the duplication? >+ _doAsyncPrompt : function() >+ // Find the first prompt key we have in the queue >+ for (var prompt in this._factory._asyncPrompts) >+ break; Please put the |var prompt;| before this loop, to make it clear what's going on? This is relying on the reader having completely internalized the weird var scoping rules, which doesn't seem good. >+ for (b in asyncPrompt.binding) { >+ var binding = asyncPrompt.binding[b]; Again, for each seems like a better fit to me. I didn't really review the tests. Let me know if you want me to.

Oh, so in bug 348997 I suggested not using Suspend()... However, I think you can avoid the problems of suspend by just suspending mTransactionPump, not the entire channel. Just make sure that you'll call Resume in OnAuthCancelled in all cases.

Comment on attachment 359946 [details] [diff] [review] v1 So, general comment: I'm really wary of rushing this into 1.9.1 at the last minute. But I do think we should get this into 1.9.2 quickly, lest it fall through the cracks again. >--- a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js >+function LoginManagerPromptFactory() >+{ >+ var observerService = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService); See comment 30. >+ observe : function (subject, topic, data) { >+ if (topic == "quit-application-granted") { >+ for (p in this._asyncPrompts) { >+ asyncPrompt = this._asyncPrompts[p]; See comment 30. >+ for (b in asyncPrompt.binding) >+ { >+ var binding = asyncPrompt.binding[b]; See comment 30. >+ if (binding.callback) { >+ self.log("Canceling async auth prompt callback " + binding.callback); Where is |self| coming from? > getPrompt : function (aWindow, aIID) { >- var prompt = new LoginManagerPrompter().QueryInterface(aIID); >+ var prompt = new LoginManagerPrompter(this).QueryInterface(aIID); > prompt.init(aWindow); > return prompt; > } This is a little odd, because LoginManagerPrompter implements multiple interfaces. When nsLoginManager does a createInstance() to get it as a nsILoginManagerPrompter, there isn't any factory being passed in (so I guess it get undefined, or maybe something else is there by default?). I think it would be better to just change nsILoginManagerPrompter.init() to take an extra argument for the factory. So it would be "prompt.init(aWindow, this)" here, and "prompterSvc.init(aWindow, null)" in nsLoginManager. I suppose you could instead share the async state in the LoginManagerPrompter prototype, but then dealing with the observer might be ugly. >+ // Invokes master password prompt, this is using the decoderRing >+ // to decrypt the password. This comment isn't needed. >+ // Invokes the username password prompt Ditto. >- asyncPromptAuth : function () { >- return NS_ERROR_NOT_IMPLEMENTED; >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { >+ var epicfail = false; >+ var cancelable = null; >+ >+ try >+ { >+ this.log("===== asyncPromptAuth called ====="); ... >+ var notifyBox = this._getNotifyBox(); >+ if (notifyBox) >+ this._removeLoginNotifications(notifyBox); Hmm, one case I'd worry about is prompts consolidating to one tab, having the user enter incorrect info, and having the retry prompts being consolidated to another tab. It looks they'd all call asyncPromptAuth again, thus ensuring any save-password notification bar is removed (right?). So that's good. We wouldn't want to leave up a notification bar for saving the wrong info. But this still means that when multiple tabs trigger an HTTP auth, and a new login is entered, the save-password notification gets shown on one random tab. I guess we should spin that out as a followup bug. Better than it is today, but awkward. Either we should show a notification on all those tabs (and remove it from all when the users clicks yes/no/never), or be smart about only showing it on the active tab (but then what to do when none of the authenticating tabs are active). >+ >+ var [hostname, httpRealm] = this._getAuthTarget(aChannel, aAuthInfo); >+ >+ cancelable = this._newAsyncPromptBinding(aCallback, aContext); Create the cancelable first, so it doesn't interrupt the flow as much. Just call it newAsyncPromptCancelable(). >+ this.log("Adding new prompt to the queue, callback = " + aCallback); >+ asyncPrompt = { >+ binding: [cancelable], >+ channel: aChannel, >+ authInfo: aAuthInfo, >+ level: aLevel >+ } And s/binding/cancelable/ here too. aLevel isn't part of the hashkey, but that's mostly ok because (1) we don't currently use it and (2) the hashkey will will include http:// or https:// to differentiate SSL from non-SSL. But I guess we should add it to the hashkey now, so it doesn't become an unexpected problem if we ever start using it in the future. >+ _doAsyncPrompt : function() >+ { ... >+ // Find the first prompt key we have in the queue >+ for (var prompt in this._factory._asyncPrompts) >+ break; I wish I knew of a nicer way to do that. :-( s/prompt/hashKey/. >+ let tm = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager); let tm = Cc["@mozilla.org/thread-manager;1"]. getService(Ci.nsIThreadManager); Either use let or var in this function, but not both. Var is fine, since the rest of the file uses it, but since all the kids are using let these days I'll accept a new function using let. >+ tm.mainThread.dispatch({ There's a lot of code for doing this inline... Better as: var runnable = { ... } tm.mainThread.dispatch(runnable, Ci.nsIThread.DISPATCH_NORMAL); >+ run : function() >+ { Nit: don't put opening braces on a new line (here and elsewhere). >+ var asyncPrompt = self._factory._asyncPrompts[prompt]; var prompt = self._factory._asyncPrompts[hashKey]; >+ ok = self.promptAuth( >+ asyncPrompt.channel, >+ asyncPrompt.level, >+ asyncPrompt.authInfo >+ ); ok = self.promptAuth(prompt.channel, prompt.level, prompt.authInfo); >+ for (b in asyncPrompt.binding) { >+ var binding = asyncPrompt.binding[b]; See comment 30 re this pattern. >+ asyncPrompt.authInfo = null; Why null this out? >+ _newAsyncPromptBinding : function(aCallback, aContext) { See comment 30. Didn't look at the tests yet, and am leaving the non-JS bits to bz/biesi to review.

(In reply to comment #52) > Also, is this actually blocked on the leak from bug 459620? This is actually fixing (or at least well working around) that bug (as per comment 16, para 4).

(In reply to comment #53) > (From update of attachment 359946 [details] [diff] [review]) > + rv = NS_NewPipe(getter_AddRefs(mUnauthenticatedDataInputStream), > + getter_AddRefs(mUnauthenticatedDataOutputStream), > + NET_DEFAULT_SEGMENT_SIZE, PR_UINT32_MAX); > > Couldn't you just suspend the transaction pump? (also a storage stream may be > slightly better than a pipe) > According to bug 348997 comment 205 you wanted me to find a fix that is not suspending the channel. Maybe I misunderstood and took it as not do any suspending at all, including on the transaction. However, when I suspend the transaction I can get some data of the unauthenticated page anyway as the transaction is asynchronous. This may break the content handler. Therefor I would need to cache the data somehow anyway and potentially use it (push it to the content handler) when user cancels authentication. > + // This is so far workaround to fix leak when reusing unpersistent > + // connection for authentication retry. > + if (conn && !conn->IsPersistent()) > + conn = nsnull; > > why would that cause a leak? it's a local variable, the connection should be > released when the function returns, right? It's a bit different. The problem is not here in nsHttpChannel but in nsHttpConnManager. When connection is reused in DoAuthRetry, connection manager doesn't free it later (I so far don't know why). So, I actually 'say' there is no connection when it is persistent. Then it is properly closed and another connection is open in DoAuthRetry. This only happens when the first attempt with wrong credentials sent fails and we are trying to authenticate again with new credentials gathered from user. I didn't try this with challenge or NTLM authentication. I don't have a proxy or site to test with and mochitest's httpd.js doesn't support keep alive connections at the moment.

> It's a bit different. The problem is not here in nsHttpChannel but in > nsHttpConnManager. When connection is reused in DoAuthRetry, connection manager > doesn't free it later (I so far don't know why). So, I actually 'say' there is > no connection when it is persistent. Then it is properly closed and another > connection is open in DoAuthRetry. This only happens when the first attempt > with wrong credentials sent fails and we are trying to authenticate again with > new credentials gathered from user. > Now I remember. We must not reuse connection that is not persistent for auth retry. From some reason it is not correctly freed by connection manager, that causes the leak. However, connection manager correctly assigns a new connection to the transaction.

> However, when I suspend the transaction I can get some data of the > unauthenticated page anyway as the transaction is asynchronous. It looks to me like suspending an input stream pump takes effect immediately; the only place where it makes OnDataAvailable calls is guarded on !mSuspendCount. Do you actually get data if you do suspend the transaction pump?

My last comment was based on an old assumption that transactions are running on a different thread. I re-checked that and it is obvious that it runs on the main thread and effect is immediate as you describe. So, if biesi agrees to suspend the transaction I will, with love, remove that new pipe for unauth data.

Yeah, I think that's the way to go. It should be a lot simpler...

If you're getting this dialog because of proxy authentication (as opposed to HTTP authentication), have a look at FoxyProxy Plus 3.1. You can enter your login credentials once and never be bothered by it again. Screenshot: http://foxyproxy.mozdev.org/fpplus/images/screenshots/current/auth.png More info: http://foxyproxy.mozdev.org/fpplus/

I still get this on shiretoko. I do not use proxies either.

@(In reply to comment #64) > have a look at FoxyProxy Plus 3.1 FoxyProxy Plus does have very nice features. Unfortunately, fixing a bug in an open source application with a paid-for plugin isn't the way to go. I recommend FoxyProxy for its features anyway - but not as a workaround for proxy-only repeat-auth issues.

The problem exists for any page that has login fields, not just http authorization requests and proxy-dependent pages. I know nothing about Firefox internals, so ignore this if it doesn't make sense, but I observe the following from skimming the discussions above: The line of thinking being pursued is to make every call or triggering of a master-password dialog asynchronous with (and thus independent of?) other calls or triggers. I would have thought that such an approach would be the CAUSE of the problem. Wouldn't it be possible to have the first instance of a call to display the master password dialog essentially call it as a new process or thread, and then set a flag that then causes all other tabs that want the master password to "queue up" behind the initiating tab? If the master password entry dialog box runs as a separate process or thread from the loading of any individual page, then it can notify each of the queued-up pages/tabs when it has been satisfied -- or cancelled. That would allow a user to make a mistake, have the master password process re-display the entry dialog as many times as needed for the user to get it right, and after the first right entry it would somehow notify each queued-up tab in sequence that it's OK to load the content or to fill in the username/password fields in a form. Or if the user cancels it, the notification would be whatever the dialog gives now to each pages if it's cancelled -- but it would feed the master password results to each queued-up tab in sequence. A kludgier but faster fix might be that the first time any tab triggers the master password dialog box, Firefox stops loading all pages immediately and waits for the results of the master password dialog. If the user CANCELS the dialog, then the first page would not be displayed or the user login fields would not be filled in, and the master-password dialog would probably be re-triggered by the NEXT tab that needs it (whether for proxy, http-auth, or login-fields). By stopping all page loading cold when the dialog box is displayed, Firefox behavior would be the same as if a user first opens up one page that triggers the master password, enters it, and then later opens up a second page that also checks for the master password. In that type of sequential operation, the browser already behaves correctly and doesn't repeatedly display the master password dialog. So forcing sequential handling of pages/tabs that require the master password would be a quick-and-dirty fix.

(In reply to comment #67) I just loaded Beta 4 of 3.5 and see that much progress has been made: I only had to enter my master password twice rather than 8 times with my current set of saved tabs. (No proxies, no http-auths -- they're all just pages with login forms/fields to be submitted.) So what remains appears to be a different issue, related to the "two requests for one page" issue. But it's gone from a major pain to a minor nuisance

(In reply to comment #68) I too was down to about 3 instances of the master password dialog this morning instead of the usual 6 or so.

(In reply to comment #67) > ... the CAUSE of the problem. Reverse of the reverse logic I reckon. ;)

Just so we're clear: 1) Any progress visible in b4 is accidental, as far as I can tell; this patch hasn't landed yet. 2) Every comment on this bug mails at least 60 people. 3) Anyone interested in why comment 67 is pretty much dead wrong is welcome to e-mail me personally; I'll send you the same explanation I sent Randy.

First tests show it is possible to suspend and resume the transaction. Is it potentially possible that the request would come from the http cache? (I would have to work with mCachePump it that case).

This is patch addressing most of the main comments. I'm now suspending the transaction, also all state flags has been removed, code is much much simpler. Tests left functionally intact. I can provide an interdiff if you wish, but it's more complex than the patch itself.

So it's nice that we have a simpler approach - do I understand correctly that even this approach is too scary to take on branch?

Comment on attachment 375178 [details] [diff] [review] v2 >* * * >* * * >* * * Nix these, please. >+++ b/netwerk/base/public/nsIAuthPrompt2.idl >+ * Method may throw any exception when the prompt fails to queue e.g. "This method" >+ * about the failure have to come through the callback. This way we "has to come" >+++ b/netwerk/protocol/http/src/nsHttpChannel.cpp The header should document that NS_ERROR_IN_PROGRESS is a valid return value for GetCredentials and GetCredentialsForChallenge, and document what the return value means. >@@ -3593,37 +3628,187 @@ nsHttpChannel::PromptForIdentity(PRUint3 >+ if (!mAuthRetryPending) { >+ // suspend the transaction pump to stop receiving the >+ // unauthenticated content data. We will throw that data >+ // away when user provides credentials or resume the pump >+ // when user refuses to authenticate. >+ LOG(("Suspending the transaction, asynchronously prompting for credentials")); >+ mTransactionPump->Suspend(); So the key is that if mAuthRetryPending then mTransactionPump is already suspended, right? When do we get to this code, exactly when mAuthRetryPending is true? Would it make sense to suspend the pump unconditionally and resume in more places, or is this really the best approah here? What does mAuthRetryPending mean, exactly? You never answered my question about it from comment 55. >+NS_IMETHODIMP nsHttpChannel::OnAuthCancelled(nsISupports *aContext, >+ // there are still some challenges to process, got through >+ // GetCredentials round again s/, got through GetCredentials round again/; do so/ >+ // some other way in a synchronous manner, process that s/that/those/ >+ // drop mAuthRetryPending flag and resume the trunsaction s/trunsaction/transaction/ >+ LOG(("Resuming the transaction, user canceld the auth dialog")); s/canceld/cancelled/ >+nsHttpChannel::ContinueOnAuthAvailable(const nsCSubstring& creds) >+ // comming from the network s/commming/coming/ >@@ -4929,22 +5117,27 @@ nsHttpChannel::OnStopRequest(nsIRequest >+ // This is so far workaround to fix leak when reusing unpersistent >+ // connection for authentication retry. >+ if (conn && !conn->IsPersistent()) >+ conn = nsnull; It's not clear to me what's going on here. Is the key that we don't want to do an auth retry on a non-persistent connection and should instead use a new one? How do we end up with connections that are not persistent when NS_HTTP_STICKY_CONNECTION is set? >+++ b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { >+ var epicfail = false; Seems to be unused.

Comment on attachment 375178 [details] [diff] [review] v2 Woo, this is looking very good! I'm minusing just because I'd like to skim the intradiff after this pile of little things are fixed, but expect a r+ then. >+ /** >+ * Return all information needed to build authorization information, >+ * all paramters except proxyAuth are out parameters. proxyAuth specifies >+ * with what authorization we work (WWW or proxy). >+ */ >+ nsresult GetAuthorizationMembers(PRBool proxyAuth, nsCSubstring& scheme, const char*& host, PRInt32& port, nsCSubstring& path, nsHttpAuthIdentity*& ident, nsISupports**& continuationState); I'm a little confused here... I don't think we use the resource's path (from http://site.com/path) as a key anywhere, but we very much do use the realm (from the WWW-Authenticate header). Is the "path" in this GetAuthorizationMembers() really the "realm"? >-function LoginManagerPromptFactory() {} >+function LoginManagerPromptFactory() >+{ Nit: opening brace should be on previous line. >+ observe : function (subject, topic, data) { ... >+ for each (asyncPrompt in asyncPrompts) { for each (var ...) Nit: you've got a double space before { >+ if (consumer.callback) { >+ this.log("Canceling async auth prompt callback " + consumer.callback); >+ consumer.callback.onAuthCancelled(consumer.context, true); >+ } Worth a try/catch here, lest a callback throw? >+ _doAsyncPrompt : function() >+ { Nit: no brace on new line >+ var runnable = { ... >+ } >+ catch (e) { Nit: catch on same line as brace >+ // Not having a callback means that consummer didn't provide it Nit: "consumer". >+ } >+ catch (e) { >+ // Throw away exceptions caused by callback Nit: catch on same line as brace >+ var tm = Cc["@mozilla.org/thread-manager;1"]. >+ getService(Ci.nsIThreadManager); Make this a memoizing getter, like this._ioService and friends. and s/tm/threadManager/ >+ _newAsyncPromptConsumer : function(aCallback, aContext) { >+ return { >+ callback : aCallback, >+ context : aContext, >+ cancel : function() { >+ this.callback.onAuthCancelled(this.context, false); >+ this.callback = null; >+ this.context = null; >+ } >+ } > } So, after a bit of IRC discussion, it would probably be good to have a QI implementation here (via XPCOMUtils.generateQI ideally). Should work fine as is, but XPConnect will let the caller QI to anything (and only error out if the callers actually tries to use non-existent interfaces). Having a strict QI might help prevent someone using it as a nsIFootGun. >--- a/toolkit/components/passwordmgr/test/authenticate.sjs >+ var proxy_expected_user = "", proxy_expected_pass = "", proxy_expected_realm = ""; ... >+ var proxy_expected_user = "", proxy_expected_pass = "", proxy_realm = "mochi-proxy"; You just redeclared the first two variables. Which one did you mean to use for the realm? >+++ b/toolkit/components/passwordmgr/test/test_prompt_async.html 4-space indent here, please... >+ // Class monitoring number of open dialog windows >+ // It checks there is always open just a single dialog per one >+ // top-level browser window. (TODO!) Looks like this TODO is done. >+ function dialogMonitor() { >+ var observerService = Cc["@mozilla.org/observer-service;1"] >+ .getService(Ci.nsIObserverService); Style nit again: var foo = Cc[...]. getService(...); >+ shutdown: function() { >+ netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); >+ var observerService = Cc["@mozilla.org/observer-service;1"] >+ .getService(Ci.nsIObserverService); Again. >+ function addLogin(host, realm, user, pass) >+ { Style nit. Rest of the test generally looks good, though I didn't go over it in excruciating detail.

Comment on attachment 375178 [details] [diff] [review] v2 Per comment 75.

> So the key is that if mAuthRetryPending then mTransactionPump is already > suspended, right? Exactly. > When do we get to this code, exactly when mAuthRetryPending > is true? When there are more then just one challenge received from the server/proxy and user cancels the first dialog for the first challenge. We reenter the code with mAuthRetryPending already up. > Would it make sense to suspend the pump unconditionally and resume in > more places, or is this really the best approah here? I changed the code to suspend the transaction only ones when we invoke the first dialog. So the problem is gone. > What does > mAuthRetryPending mean, exactly? You never answered my question about it from > comment 55. > w/o my patch it means: user provided credentials (synchronously during OnStartRequst) and now we have to throw away all content that came with 401/407 (in OnDataAvailable). In OnStopRequest we then have to replace the transaction and provide auth headers => DoAuthRetry call. w/ my patch it also means "we queued a dialog to ask user for the credentials and we are now waiting for callback with that credentials". > It's not clear to me what's going on here. Is the key that we don't want to do > an auth retry on a non-persistent connection and should instead use a new one? Exactly. The reason is that the connection manager is not handling non-persistent connections in this case well and we leak (another bug). It replaces the connection with a new one anyway but doesn't correctly throw away the current one. So I just force throwing the current non-persistent connection away on the http channel level. > How do we end up with connections that are not persistent when > NS_HTTP_STICKY_CONNECTION is set? > NS_HTTP_STICKY_CONNECTION is set only (in the whole necko code) to let the transaction keep the connection it have when doing the re-authentication. It doesn't make sense for non persistent connections anyway. > I'm a little confused here... I don't think we use the resource's path (from > http://site.com/path) as a key anywhere, but we very much do use the realm > (from the WWW-Authenticate header). Is the "path" in this > GetAuthorizationMembers() really the "realm"? It's not intended to be "realm", it's intended to be "path" and it's used as "path" in following calls. Do you see any concrete mistype or something?

(In reply to comment #78) > > I'm a little confused here... I don't think we use the resource's path (from > > http://site.com/path) as a key anywhere, but we very much do use the realm > > (from the WWW-Authenticate header). Is the "path" in this > > GetAuthorizationMembers() really the "realm"? > > It's not intended to be "realm", it's intended to be "path" and it's used as > "path" in following calls. Do you see any concrete mistype or something? No, I think it's fine. I was just concerned that the realm didn't seem to show up in the diff anywhere, but I now see it's handled separately a few lines up in the existing code.

bz, do you think you get to this to review? The patch is simpler. Thanks.

Comment on attachment 383686 [details] [diff] [review] v2.1 Please make sure a bug is filed on that issue with leaking when reusing a non-persistent connection, and put that bug number in the comment? >+++ b/netwerk/protocol/http/src/nsHttpChannel.h >+ * the user's decision will be gethered in a callback and is not an actual "gathered" >+++ b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js >+ asyncPromptAuth : function (aChannel, aCallback, aContext, aLevel, aAuthInfo) { >+ var epicfail = false; This continues to be unused, as in the previous patch version. Please remove it. Sorry for the lag; the month between the last time I looked and the patch getting updated meant I'd more or less forgotten everything about this stuff and had to look it all up again. So the review was gated on having a nice 5-6 hour block of time free. :(

Honza - I would lovelovelove to see a version with comments addressed so that we can land this on trunk. Excited! (Thank you, reviewers all).

(In reply to comment #83) > Honza - I would lovelovelove to see a version with comments addressed so that > we can land this on trunk. Excited! (Thank you, reviewers all). Just for info, I am looking now for a regression range where from one of the tests for this bug is failing thanks a bug in XPCOM (currently unreported). I got NS_ERROR_XPC_HAS_BEEN_SHUTDOWN when accessing iframe.contentDocument.getElementById method. When I find the cause and fix it I'll land this patch immediately.

The report of JS/XPCOM issue is bug 504877.

I commented on that bug; the short of it is that you're getting a security error (as you should, given your testcase code!) and we're just not reporting it very well. I have no idea why you didn't get that error before; did you change the testcase or something? If not, and if applying the patch to 3.5 doesn't give the same exception, then we may need a bug on _that_.

(In reply to comment #86) > I have no idea why you didn't get that error before; did you change the > testcase or something? If not, and if applying the patch to 3.5 doesn't give > the same exception, then we may need a bug on _that_. I didn't change anything from the last time. Notice UniversalXPConnect privilege in the localhost:8888 test body. From what you say I should get the same security check failure when from localhost:8888 accessing an iframe loading example.com. See https://bugzilla.mozilla.org/attachment.cgi?id=383686&action=diff line 402. iframe1 etc are <iframe name='iframe1> etc in localhost:8888/test_async_prompt.html loaded at that moment with example.com/subtst_prompt_async.html (line 216). I'm going to try w/o privilege and on 1.9.1.

It behaves correctly - got a security check failure - on 1.9.1 (either central) w/o the UniveralXPConnect privilege. Bug I reported was valid, I should be able to access property of iframe inside an iframe when UniveralXPConnect is engaged. That was broken. I checked that when all subframes are from the same origin (localhost:8888) I *CAN* access the properties. I will rewrite the test a bit to bypass that issue for now.

> From what you say I should get the same security check failure when from > localhost:8888 accessing an iframe loading example.com That would be one approach, yes. > Bug I reported was valid, I should be able to access property of iframe inside > an iframe when UniveralXPConnect is engaged. Where is it engaged? Top of doCheck? I believe XOW only checks for UniversalXPConnect on the topmost stack frame, not for all stack frames. It's by design, as I recall. But Blake would know more.

Just to be complete: I changed the test to add UniversalXPConnect privilege also to the function that the exception was threw in. It helped. (Same comment in bug 504877)

Woohoo! For us users out there, what is the first release of firefox in which we can expect seeing this fix?

We may reconsider getting this to 1.9.1. However, there is an API change (just changed IID of an interface, because meaning of a method has slightly changed, better say, just was not previously well documented, IMO). (In reply to comment #92) > Woohoo! For us users out there, what is the first release of firefox in which > we can expect seeing this fix? In Firefox 3.6 and every software based on Gecko 1.9.2a1+. If drivers decide we may take this to Firefox 3.5.x based on Gecko 1.9.1.x. But I don't see a big chance for that.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.1pre) Gecko/20090718 Shiretoko/3.5.1pre (.NET CLR 3.5.30729) I think is still broken. As of the latest Shiretoko update I've received (see above) at startup the browser appears to be using asynchronous prompting for the proxy password. In the meantime, the browser continues ahead and the result is that the homepage comes up with the "no credentials" error.

Scott, this change is not in the Shiretoko builds (and probably won't be). See comment 93. If you want to test this, you need to be using a Minefield build.

After several years with zero progress and then someone fixing this bug in a single day, how much effort would that really be to fix in 3.5? I mean you guys annoyed a lot of people who aren't even here on bugzilla. Listing this fix in the release notes might cause an outcry of relief that could very possibly move the earth ;) So, please, unless you're already 100% sure you're going to be tarred and feathered by the plugin authors, backport the fix for 3.5.2. The world would be a better place.

List of bugs probably fixed by this: https://bugzilla.mozilla.org/show_bug.cgi?id=339804 https://bugzilla.mozilla.org/show_bug.cgi?id=230190 https://bugzilla.mozilla.org/show_bug.cgi?id=406143 https://bugzilla.mozilla.org/show_bug.cgi?id=383419 https://bugzilla.mozilla.org/show_bug.cgi?id=419333 https://bugzilla.mozilla.org/show_bug.cgi?id=430287 https://bugzilla.mozilla.org/show_bug.cgi?id=460868 https://bugzilla.mozilla.org/show_bug.cgi?id=390760 https://bugzilla.mozilla.org/show_bug.cgi?id=501309

Why are those not in the "bugs depending on this" field here, exactly?

(In reply to comment #98) > Why are those not in the "bugs depending on this" field here, exactly? The keyworld here is "probably". Honestly I'm not sure this bugfix resolves all the above bugs.

That's fine; they should be tracked as depending on this, and if it doesn't fix them then they stay open...

(In reply to comment #97) Probably fixed: > https://bugzilla.mozilla.org/show_bug.cgi?id=339804 > https://bugzilla.mozilla.org/show_bug.cgi?id=230190 > https://bugzilla.mozilla.org/show_bug.cgi?id=430287 > https://bugzilla.mozilla.org/show_bug.cgi?id=460868 Not sure by this one: > https://bugzilla.mozilla.org/show_bug.cgi?id=501309 Bugs not related to this one: > https://bugzilla.mozilla.org/show_bug.cgi?id=406143 > https://bugzilla.mozilla.org/show_bug.cgi?id=383419 > https://bugzilla.mozilla.org/show_bug.cgi?id=419333 > https://bugzilla.mozilla.org/show_bug.cgi?id=390760

Sorry; my bad on the previous comment. On Minefield it does wait on the proxy password before displaying the first page.

Can I also add my voice to the "can this go into 3.5.x ASAP". This bug has been around for a *long* time (see Bug 230190) and is the cause of much pain.

For those interested, here https://build.mozilla.org/tryserver-builds/honzab.moz@firemni.cz-bug475053/ are test builds of Shiretoko (= Firefox 3.5.2pre) with this patch applied. Please test that build, but not only for the problem this bug solves (multiple prompts), use it as regular browser for some time and if there are any problems related to networking, please report it *as a new bug* blocking this one and CC me. Thanks.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre ID:20090722034040 Sorry Honza but this bug is not fixed. I clearly get one master password dialog if I have open several pages for one domain (e.g. intranet). But when having other website open in other tabs which also requires a login the master password dialog is shown multiple times. Steps: 1. Enable Master Password 2. Open a tab with content which is password protected (HTTP auth, e.g. intranet) 3. Open another tab with a page which has a normal user/password field to login 4. Restart Minefield. Current results: * A master password sheet is shown but closed immediately to show another master password sheet * When you have entered your master password (initially the focus is not in the textbox anymore) in the second sheet it is closed but the first one is shown again Is noticeable on OS X and Windows. I haven't tested on Linux. I feel we should reopen this bug.

Henrik, ok I am going to take a look at this. To be clear, do you claim it happens only after restart, i.e. after session restore?

I don't know of another way which lets reask for the master password and reload all the tabs at once. So for now it will happen after a restart and with all the tabs restored, yes.

No, this bug is fixed. Multiple master password prompts can still occur as a result of form logins -- basically bug 499233. *This* bug fixed the problem when it was caused by multiple HTTP/Proxy authentications. If you load one tab triggering HTTP auth, and one tab with a form login, you will still, unfortunately, get two master password prompts.

This was not "fixed" for me as of this morning - I am still being asked for my Master Password twice within the same tab upon startup (and it does not only happen after a system restore - it happens whenever I startup a new Firefox session.

<block>> Multiple master password prompts can still occur as a result of form logins -- basically bug 499233. *This* bug fixed the problem when it was caused by multiple HTTP/Proxy authentications.</block> I guess I should really be tracking 499233, then. I picked this one to vote for and follow because it seems like there should be a common point of failure (and resolution) for both. Don't both instances use the same call for a master password dialog box?

No, which is why there are two separate bugs. Also note that the two password dialogs are entirely different, a further hint at this fact.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090722 Minefield/3.6a1pre (.NET CLR 3.5.30729) > It fixes multiple master password prompts and multiple user/password > auth prompts Is this the right bug then to note multiple prompts? Starting up this morning I had an add-on update. That caused a proxy prompt, and then I got another proxy prompt when the main browser window opened. A second problem (which might need a separate bug) is that if you wait too long to respond to the proxy prompt on the add-on update, you get a dialog box saying that the script running on "addonsRepository.js" (sic; I might not have exactly the right name there) is taking too long, do you want to continue or cancel.

I tried out the test build, and this has indeed resolved the issue for me. I'm very pleased with this, as this one issue was the only reason why my organization had not been able to deploy Firefox 3 in our intranet. This bug has been a *major* showstopper for over a year, so I hope this fix does make it into the next official release.

Firefox 3.5.2 is now out. Did this make it? (It's not listed in the "complete list of changes" page.) Or do I have to stay with Shiretoko/3.5.2pre for the time being?

Ok, I rechecked this bug with a couple of different websites which require a HTTP authentication. Everything looks great with builds on OS X and Windows like Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090803 Minefield/3.6a1pre (.NET CLR 3.5.30729) ID:20090803044626 Can anyone please verify this patch with a recent Minefield build (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/) and an a box which needs multiple Proxy authentications? I don't have a proxy available. Help is very appreciated.

I'm using it (Shiretoko/3.5.2pre - see #104) behind an authenticating proxy. It's working OK for me. This is on Linux.

(In reply to comment #116) > I'm using it (Shiretoko/3.5.2pre - see #104) behind an authenticating proxy. > It's working OK for me. > This is on Linux. It has not been fixed for Firefox 3.5.x yet. So it probably works due to an extension which you are using. Please run a Minefield build with a clean profile for verification. Thanks.

Just to be clear.... The build pointed to in comment 104 is a _test_ build. It is NOT meant to be used for daily browsing by users, in general; in particular it will not get any security updates, and may have bugs that are not present in any release version of Firefox. If you want to use it with those caveats, that's your right, of course. Just please don't advertise it to people who are not as clueful as you are and might get stuck with an insecure browser. If and when this bug is fixed in the 3.5.x series (which uses Gecko 1.9.1.x), there will be a comment here to that effect, and the "status1.9.1" flag you can see on the right hand side of this page near the top, above the attachment list, will be changed to reflect which particular 1.9.1.x it was fixed it. Until that happens, there's no point asking whether it's been fixed in a particular 3.5.x release: it hasn't.

Well - the reason I asked (about 3.5.2) is that the bug is marked as: RESOLVED FIXED but form what you say it sounds otherwise...

Well - the reason I asked (about 3.5.2) is that the bug is marked as: RESOLVED FIXED but from what you say it sounds otherwise...

(In reply to comment #119) > Well - the reason I asked (about 3.5.2) is that the bug is marked as: > RESOLVED FIXED > but form what you say it sounds otherwise... FIXED does not mean it's fixed in every version. There are multiple versions in various states of testing and release, you know. comment 118 explains it. The current main development version (also called "the trunk") is an early Firefox 3.6 build (not even Alpha 1). Only well tested bugs can be moved to the 3.5.x or 3.0.x builds.

Thanks - I realized shortly the version-thing shortly after my comment. And I'll assume you mean "only well tested fixes". The bug is already well tested.

In this bugs scope:- Currently when you have a proxy server set which needs a password, firefox prompts you to save the password and subsequently pre-fills it - great - expected, wanted. But when it you first start it up and it finds updates for add ons the remembered information module is not used ... be good to use that in this case too. Can send screen shots if more explanation is required. --- Or need a new request?

(In reply to comment #123) > Or need a new request? Yes. Though I'm pretty sure there's already a bug on file for that.

one comment on the multi-desktop concerns. it's already the case that you can have a model popup on one desktop for a browser tab that lives on another desktop. when the popup takes place it shows up on the desktop you are currently on, so if you open a bunch of tabs (or start firefox with a saved session) and then hop over to another desktop to get useful work done while things load, it's very possible to end up with a popup on a different desktop. so the issue of a consolodated popup being on a different desktop from some tabs is a continuation/variation of an existing problem, not a new one. don't let it stop this from getting out

(In reply to comment #125) > one comment on the multi-desktop concerns. > See comment 13. If you want to, fill a bug to fix this for multiple desktops and CC me please.

I also get multiple master password prompts upon startup with multiple tabs. I'm using Namaroka 3.6b2pre (mozilla-1.9.2/rev/7347a2572f1d) running on MacOSX 10.6. The StartupMaster add-on (https://addons.mozilla.org/en-US/firefox/addon/9808) can be used as a workaround, but it's definately not perfect.

Sorry, the last comment was posted to the wrong bug. Please ignore.

Upon the update to 3.0.8 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)) I received a proxy authentication for every addon during the "checking for compatability" phase. The prompt boxes were also missing the text that identifies them as proxy authentication dialogs -- they had just the two fields for name and password.

if this bug was supposed to also fix the forms passwords in multiple tabs prompting multiple master passwords, well then that part is not fixed (tested in FF 3.6b5)

According to comment 123, this is fixed for multiple tabs. Not so here, in 3.5.6. I get some 30 proxy prompts whatever I do. Yes, I have read and tried the “network.negotiate-auth.allow-proxies” set to true (default); as well as false (suggested in http://burakdd.wordpress.com/2009/07/29/firefox-keep-asking-proxy-username-password/). No chance. I reboot and am presented with 30+ overlapping proxy-request pop-ups. It is high time to reopen it; I only don't know how.

udippel@gmail.com, this bug is not fixed in 3.5.anything, nor are there obvious plans to fix it there. See comment 118. You might want to try a beta of 3.6.

not realizing that this bug should also fix the multiple master password prompts when encountering form based passwords, there has been a new bug 499233 created for that issue. It already has collected 47 votes. Is it possible to take all votes for this bug and all other (now closed) bugs about the mutiple password issue and attach them to bug 499233 ? :-) So please, everyone reading this, go to bug 499233 and cast your vote. yes this is annoying, yes it's a showstopper and yes it makes FF impossible to use in enterprise environment.

When I am using a proxy with password, Firefox shows many password prompting dialogs overlapped on startup. Please reopen this bug.

Sizhuang: See comment 118 and, if you are using a beta, let us know which version.

Not fixed for the Master Pasword prompt in FF 3.5.7 on windows (bug 356097), it's really annoying. I can't understand that a bug like this, which deals with personal privacy (i've seen that some addons are proposed as workaround... what about confidence about them ?), is marked as RESOLVED FIXED, while the directly available releases of the product doesn't includes the bug fix. Any information on when the current target milestone for this bug will be landed in the FF 3.5 releases ? Anyway, thanks for your efforts guys.

(In reply to comment #142) > Not fixed for the Master Pasword prompt in FF 3.5.7 on windows (bug 356097), > it's really annoying. > I can't understand that a bug like this, which deals with personal privacy > (i've seen that some addons are proposed as workaround... what about confidence > about them ?), is marked as RESOLVED FIXED, while the directly available > releases of the product doesn't includes the bug fix. > Any information on when the current target milestone for this bug will be > landed in the FF 3.5 releases ? > Anyway, thanks for your efforts guys. The fix will be included in Firefox 3.6. There doesn't appear to be any plans on fixing this in Firefox 3.5.x

I am working at a customer site, and therefore I am logging into my computer with a different login domain and username then the one i need to use for to authenticate the automatic proxy (i.e. proxy.pac), allowing me to connect to the internet. With Firefox 3.5.7, when starting Firefox I was prompted to input the user name and password for the proxy authentication several time (I suspect once for every Add-On I have in my Firefox), but after that I didn't have to authenticate the proxy again. This morning I have upgraded to Firefox 3.6 (that was suppose to solve this defect), and now I am being asked to authenticate the user name and password for every new page I am opening and for any object on the page. I.e. from my perspective, things got WORSE, and I had to downgrade back to Firefox 3.5.7 to be able to continue using Firefox

Denya, please file a new bug for your issue. Attaching debug output per http://www.mozilla.org/projects/netlib/http/http-debugging.html and https://wiki.mozilla.org/Firefox:Password_Manager_Debugging would help diagnose whatever is happening.

Firefox 3.6 (Windows XP) permanently asks for proxy login while I type every new letter in the search bar. After "Enter" it asks login once more. All works fine if I login into proxy at first (and in Firefox 3.5). And could brouser ask authorisation AFTER I complete a sentence (e.g. after "Enter")

demidov, that's an interesting bug. Please file a new bug report for your problem, and mention the bug number here.

Also see bug 521467 (proxies only).

(In reply to comment #148) > demidov, that's an interesting bug. Please file a new bug report for your > problem, and mention the bug number here. There is no need for a new bug. The reported issue is already tracked by bug 538953.

(In reply to comment #143) > The fix will be included in Firefox 3.6. There doesn't appear to be any plans > on fixing this in Firefox 3.5.x I'm currently running Firefox 3.6 on a Mac, and the master password dialog still presents itself once per tab (perhaps only once per authenticated tab, but I lose count after having to type it 3 or 4 times). If asyncPromptAuth was intended to fix the master password issue, then perhaps this bug ought to be reopened. Thank you all for your efforts in trying to fix this.

I am running Firefox 3.6.6, and my issue I had logged, duplicate of this has been fixed. I no longer receive multiple password prompts for each opened tab when Firefox restarts or I use Open All Tabs via bookmarks. Are you running the latest Firefox (currently 3.6.6 for me), Ben? I'm extremely grateful to those who worked on this and others who logged it too.

I am running Firefox 3.6.6 on Ubuntu (via their repository) and I still see multiple password dialogs when restarting a session. I just tested closing a session with 7 tabs in 2 windows, a mix of encrypted/unencrypted and authenticated unauthenticated pages. When I relaunched, I saw 4 dialogs. That's less than the total number of tabs being restored, so perhaps the issue has been somewhat mitigated, but not fixed completely. Also, the dialogs still pop up in front of one another, so that if I begin typing my master password in the first one, the keystrokes get spread out randomly among each successive dialog as they appear.

In reply to comment #156: Ben, could you try with an "official Mozilla" Firefox 3.6.6 from http://www.getfirefox.com/ ? If that works, then obviously the Ubuntu Fx366 is "not as good" as the Mozilla Fx366. If both have the problem, well, I suppose you will be no worse than you are now.

A reminder to all, this bug is for multiple proxy password prompts. Multiple authentication prompts are still an issue and is bug 499233.

(In reply to comment #158) > A reminder to all, this bug is for multiple proxy password prompts. Multiple > authentication prompts are still an issue and is bug 499233. Oops, I should have said multiple master password dialog boxes are bug 499233.

Micah: Does that mean that for example bug 498905, bug 517758, bug 519658, bug 522716 and bug 542321 has been incorrectly marked as duplicates of this bug?

(In reply to comment #160) > Micah: Does that mean that for example bug 498905, bug 517758, bug 519658, bug > 522716 and bug 542321 has been incorrectly marked as duplicates of this bug? No, after reading the comments they seem to be duped correctly. Some are a tossup between this bug and bug 499233, but that's at the discretion of who dupes. Some even mention both bugs. No need to redupe, anyone who's interested can subscribe to bug 499233 if they're having master password issues.

Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) Gecko/20100625 Firefox/3.6.6 (.NET CLR 3.5.30729) there are still multiple proxy prompts in the situation where the browser performs initial add-on checks (e.g., to check to see if add-ons are compatible with a new version, or when updates are found). You get at least one proxy prompt at that stage and then at least one when the main window opens.

(In reply to comment #162) That's because Firefox restarts between the extension update and opening the main Firefox window. It might be somewhat mitigated by restartless extensions - if all your extensions use the no-restart-required mechanism. This bug is fixed. Please don't add further comments here. If you still have problems: consider that it might be bug 499233 or file a new bug.