Last Comment Bug 499233 - multiple master password prompts triggered by filling form logins in multiple tabs
: multiple master password prompts triggered by filling form logins in multiple...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Password Manager (show other bugs)
: Trunk
: All All
: -- normal with 59 votes (vote)
: mozilla2.0b3
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 437557 488637 490331 516051 522228 533145 538253 540796 543319 546184 547166 550112 555672 557142 566835 571187 574633 575584 577895 584003 587113 590020 593292 600549 614184 (view as bug list)
Depends on: 437904 513534 CVE-2010-0172
Blocks: 177175 cuts-focus masterpassword 95397 460868
  Show dependency treegraph
 
Reported: 2009-06-18 16:45 PDT by Justin Dolske [:Dolske]
Modified: 2010-11-23 07:35 PST (History)
120 users (show)
benjamin: blocking1.9.2-
dolske: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (WIP) (14.21 KB, patch)
2009-09-14 19:20 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.2 (WIP) (19.88 KB, patch)
2009-09-29 15:33 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.3 (WIP) (25.61 KB, patch)
2010-01-11 17:32 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.4 (26.28 KB, patch)
2010-04-14 17:43 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch v.5 (40.02 KB, patch)
2010-05-16 19:37 PDT, Justin Dolske [:Dolske]
paul: review+
Details | Diff | Review
Patch v.6 (39.78 KB, patch)
2010-07-27 19:15 PDT, Justin Dolske [:Dolske]
sdwilsh: approval2.0+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2009-06-18 16:45:33 PDT
Multiple master password prompts can be triggered in a variety of ways. Bug 475053 will fix many of them, by serializing HTTP auth and proxy auth prompts. The main remaining cause will be from opening multiple tabs with logins available (perhaps at startup), each of which triggers a master password prompt.

Login manager may need to serialize the filling of forms in a similar way, so that multiple master password prompts are not invoked. Alternately, we may (instead? also?) want to look at involving PSM in the fix, so that opening 1 tab with form auth and 1 tab with HTTP auth don't each invoke a master password prompt.

[I'm filing a new bug on this, since the tangle of existing bugs is an unhelpful mess.]
Comment 1 Justin Dolske [:Dolske] 2009-06-18 16:45:54 PDT
*** Bug 488637 has been marked as a duplicate of this bug. ***
Comment 2 Matthias Versen [:Matti] 2009-06-19 10:02:24 PDT
isn't this a dupe of bug 356097 ?
Comment 3 Justin Dolske [:Dolske] 2009-06-19 10:06:53 PDT
No. Bug 356097 is about proxy auth, and will be fixed by bug 475053.
Comment 4 Micah Gersten 2009-07-14 00:09:52 PDT
Ubuntu Bug:
https://bugs.launchpad.net/bugs/398128

User was seeing this in 3.0.11 and I'm seeing this in 3.5.
Comment 5 Randy Steer 2009-07-22 14:01:36 PDT
THIS is what I've been voting for in multiple other bugs (from 3.0 to 3.5).  I don't understand why it's not all considered one bug.  A tab/process/thread opens a URL and -- for whatever reason -- a login is required and a call is made to the master password dialog.  For future maintenance alone, I would think that all of those circumstances would be using the same call and subroutines for master password.  Can't the call for master password check, and if necessary set, a flag, and then put each process/thread in a queue until the MP is entered?
Comment 6 João Carlos Mendes Luís 2009-08-04 08:02:35 PDT
Bug 369963 is marked as solved, but indeed is a duplicated of this, and therefore, unsolved...

It is a very old and annoying bug for those who keep many open tabs between browser restarts.
Comment 7 RNicoletto 2009-08-06 02:39:44 PDT
(In reply to comment #6)
> Bug 369963 is marked as solved, but indeed is a duplicated of this, and
> therefore, unsolved...
> 
> It is a very old and annoying bug for those who keep many open tabs between
> browser restarts.

bug 475053 has been resolved and landed on trunk. If you're experiencing this bug try reproducing with a clean profile on latest trunk version (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/).
Comment 8 Jason Bassford 2009-08-06 05:09:22 PDT
Bug 475053 is not the same thing and it did not fix this.  (Already tested with nightlies after it was checked in.)
Comment 9 askmike2009 2009-08-07 00:16:06 PDT
Same problem (I think it was not the case in Firefox 2): I have several tabs opening at startup with identification sessions for which my credentials are saved. It triggers a corresponding number of Master Password requests instead of one request for all.

Reproducible: Always

Steps to Reproduce:
1. Have as startup page with two (or more) tabs with identification sessions (for instance your email account and another account)
2. Have the Master Password remember your credentials for the said tabs
3. Restart Firefox

Actual Results:  
You will have two (or more) prompts for Master Password; filling in the first prompt will not make the other prompts disappear.

Expected Results:  
Have one prompt for all. 

Uneducated guess:
Recognition of a website with credentials saved in Master Password should not trigger immediate prompt for Master Password but a CHECK for existence of untreated (i.e. not filled in correctly and not canceled) Master Password prompt.
Comment 10 BoffinbraiN 2009-08-17 03:10:14 PDT
I can confirm this.  It annoys me every day when I start up Firefox.  It's been the case for all of Firefox 3 and Firefox 2 as well, for as long as I can remember.

Entering the master password correctly on each individual prompt causes the login credentials for the respective tab (whichever it is) to fill in, and for those you cancel, they are not, so it's obviously every process for itself.

I don't know how it's implemented at the moment, but maybe it should be set up in a callback format, a little like AJAX requests.  Password manager remembers whether a prompt is open, and while there's no response from the user, it queues the requests and finally calls the callback functions for each tab that was asking for credentials.
Comment 11 Nickolay_Ponomarev 2009-08-29 03:57:14 PDT
*** Bug 490331 has been marked as a duplicate of this bug. ***
Comment 12 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-09-11 14:24:18 PDT
*** Bug 516051 has been marked as a duplicate of this bug. ***
Comment 13 Justin Dolske [:Dolske] 2009-09-14 19:20:07 PDT
Created attachment 400653 [details] [diff] [review]
Patch v.1 (WIP)

Somewhat rough work-in-progress. Still need to handle a few edge cases, clean things up, and add tests. But the basic concept works with this.
Comment 14 Justin Dolske [:Dolske] 2009-09-24 22:20:23 PDT
*** Bug 437557 has been marked as a duplicate of this bug. ***
Comment 15 Justin Dolske [:Dolske] 2009-09-29 15:33:09 PDT
Created attachment 403604 [details] [diff] [review]
Patch v.2 (WIP)

Cleaned up, still a few todos and needs tests, but I'll toss it to zpao for a prereview.
Comment 16 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-09-29 17:06:02 PDT
Comment on attachment 403604 [details] [diff] [review]
Patch v.2 (WIP)

>+                // XXX Can we detect if this document has died in the time since
>+                // we deferted it?

My question too. You can write a test for it :) (or at least make sure we have a litmus test)

>+                        // XXX do we need be able to defer this too?

Presumably yes? You know the code better, but if we haven't decrypted anything for the form yet, I would think this would have to be deferred.

All-in-all it looks good. You know the parts you need to finish. My only concern (and one you say you know the answer to) is how this affects 3rd party consumers. If I'm understanding this right, it shouldn't actually affect them at all since they mostly aren't trying to fill forms.
Comment 17 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-10-14 10:24:29 PDT
*** Bug 522228 has been marked as a duplicate of this bug. ***
Comment 18 Tom Levels 2009-10-27 01:14:57 PDT
I used to have an extension which solved this problem, but since a few days it does not work anymore. Any progress on the fix?
Comment 19 henrik 2009-11-03 23:51:34 PST
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.
Comment 20 Mitch 2009-11-13 13:40:23 PST
This hasta make it in to 3.6, this bug is old.

Many have suggested putting the master password prompt first, before any windows open, this (would) have solved both bugs. Is this what the patch does?
Comment 21 Honza Bambas (:mayhemer) 2009-11-13 13:49:07 PST
Works perfectly! The memory problem seems to be a bug in Apoint.exe but I'll keep an eye on it. Thanks for fixing this.
Comment 22 Honza Bambas (:mayhemer) 2009-11-13 13:49:34 PST
(In reply to comment #21)
> Works perfectly! The memory problem seems to be a bug in Apoint.exe but I'll
> keep an eye on it. Thanks for fixing this.

Sorry, wrong bug.
Comment 23 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2009-11-13 14:33:07 PST
(In reply to comment #20)
> This hasta make it in to 3.6, this bug is old.

Unless our plans drastically change, this won't happen - plan is to freeze for RC next week, and this (and the changes it depends on) haven't even made it onto trunk yet.

> Many have suggested putting the master password prompt first, before any
> windows open, this (would) have solved both bugs. Is this what the patch does?

No, this patch keeps track of whether or not we're showing the master password UI, if you enter it once, the other requests for it get notified so they don't also popup the dialog.
Comment 24 Maxime Q. 2009-11-26 03:38:51 PST
Any progress on the fix?

The bug is always present in 3.6b4 and it's very irritating ... I hope it will be fix in 3.6 final.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2009-12-06 00:14:54 PST
*** Bug 533145 has been marked as a duplicate of this bug. ***
Comment 26 Gunstick 2009-12-20 13:38:13 PST
I have done some testing with firefox 3.6b5 and this bug is not fixed.
Which surprises me as the proxy auth bug which is similar IS fixed.
Now here is a race condition. If you have mutitple tabs with password forms and an authenticating proxy it depends what comes first.
If the proxy kicks in first, you get a single master password prompt and no prompt at all for the forms in the tabs.
If the proxy is late, then you get a master password for each tab and one for the proxy.

What I don't get, as many other people, is why this has not been fixed all together instead of one for proxy auth, one for basic auth, one for form auth and so on?

Result: firefox is still as "unusable" as before and even got a more erratic behaviour inside enterprises: sometimes it works sometimes not. 

Also why has this new bug appeared whereas form based multiple prompts have already been discussed in other bugs for years? Looks to me as if people want to evade the issue.
Comment 27 Aaron Digulla 2009-12-21 02:54:18 PST
In one of the many comments in all these bugs, someone suggested the one and only correct solution: Create a global lock for *all and any kind of dialog*.

Even if you pop up just a single dialog per proxy, master password, site auth, ...., that still means that I can get a second dialog while I'm typing in the first. I can type text blind but not passwords. So I'll always press RETURN for the wrong dialog.

Therefore, FF must never open a second dialog, no matter what.

A great optimization would be to wait for asking for anything until I open a tab which actually needs the password.
Comment 28 Aaron Digulla 2009-12-21 02:56:13 PST
Along the same lines, I don't see how this bug depends on bug 513534. I would say it's the other way around: When the dialogs have become sequential, we can start on the optimization.
Comment 29 Robert Kaiser (not working on stability any more) 2009-12-28 05:40:36 PST
I wonder if we should have a generic framework that enables us never to ask for the master password twice at the same time in a more general way - Standard8 has been working on something for Thunderbird to make things better there, not sure how much it would fit that...
Comment 30 Steve Kelem 2009-12-28 14:59:04 PST
When I start up Firefox (3.5.5) I have lots of tabs.

I get SIXTEEN password popups! I type my password into the first one that lets me type and hit Enter. The popup doesn't disappear. A new one pops up on top of it. Sometimes it pops up while I'm typing in the first one, grabbing some of the newly-typed characters.

When there are lots of password popups, ONLY ONE of them will take input or honor its buttons.  This turns "entering your password" into a game of whack-a-mole, trying to guess which one of the sixteen popups will let me hit Cancel! I'm not sure what else is going on (probably the page loads for each of the tabs), but it's REALLY SLOW, so it takes several minutes before a real firefox window appears!
Comment 31 Justin Dolske [:Dolske] 2010-01-04 20:08:11 PST
I'm updating the patch for this bug, and am curious what people want to happen for one specific situation:

Suppose you are restoring a session in which there are multiple HTTP Auth requests for different servers. One master password prompt will be displayed while the others wait. Now, suppose for some reason you click Cancel instead of entering the master password. What should happen to the pending auth requests, and why (what's your use case)?

1) Cancel all of them. [No further MP or HTTP prompts displayed while restoring]

2) Cancel only the prompts for which you have stored logins. [ie, no further MP prompts.]

3) Don't cancel any of them. As each is processed, ask again for the Master Password if there are stored logins for the site.

4) Don't cancel any of them, but don't prompt again for the Master Password while restoring [thus, each prompt will be empty, with no login prefilled].

I'm planning on implementing #1, on the theory that if you're restoring a large number of tabs but click Cancel for the MP, you don't want bothered with any more prompts. The user might even just be borrowing someone else's computer, and has no interest in authenticating to anything (eg, they just want to check gmail). You can, of course, reload any tab to trigger the MP/HTTP auth again.

#3 is the easiest, but I don't think it's very useful (it just leads to clicking Cancel over and over). #4 may be rather hard to do due to implementation details.
Comment 32 Brendan Hide 2010-01-04 22:02:44 PST
#4 seems is an interesting option however I don't think it would be appropriate without also making the other options available to the user. Perhaps these four cases can be implemented as an option in a separate feature request.

Personally I prefer #1.

Does the above logic apply when the multiple prompts are not for a restore, such as when we click a bookmark with multiple tabs or quickly open multiple links in new tabs?
Comment 33 Josh Rosenbaum 2010-01-04 23:02:01 PST
If the master password prompt only came up when you clicked the tab with the logins then I'd definitely say #3. However, based on my usage the master password comes up for all tabs at once, so #1 makes the most sense. It doesn't make sense to me to cancel it once and then want to enter it directly after.
Comment 34 J. Joseph Felten 2010-01-04 23:52:06 PST
Any are an improvement over the current situation, but my suggestion is to give the user the choice; add a Cancel All button.
Comment 35 Jonathan Pritchard 2010-01-05 00:35:12 PST
A Cancel All button sounds the most transparent and in keeping with current methodologies. I thought you could always utilise the hang down (probably not the right name for it) that comes up across the top of a web page to ask if you want to save a password and instead give an explanation of the Cancel All, something like that.
Comment 36 Aaron Digulla 2010-01-05 00:49:03 PST
Implement *anything* and do it *before* 3.6 goes bad! The cancel button is an interesting piece of UI ambiguity but if it delays this patch by even a single hour, drop the work on it and open a new bug to fix it later.

To solve the ambiguity, you'd need a way to tell the user "There are N other pages which also need the master password", otherwise she can't make an educated decision. If you can't provide this data, then "Cancel" must be equals "Cancel All" (otherwise, it would be pointless to even have a Cancel button).

If "Cancel" isn't always "Cancel All", then what does a single cancel mean? Why would the user cancel a single MP request? Is there a situation where this would make sense at all?

As I see it, there is never a situation where I want to provide the MP for some connections but not for others. So when I press cancel, I just want to get rid of the damn dialog, period. That's why I think it's always "Cancel All". If you can, then set a flag not to ask again for the MP until all tabs have loaded and close the dialog.

If there is no simple way to implement this, then just open a new bug and concentrate on fixing the issue at hand.
Comment 37 Manolis Stamatogiannakis 2010-01-05 03:37:04 PST
I largely agree with Aaron. It is imperative to improve on the current situation before the next FF release. So, solution #1 would be the best to pursue in the short term.

In the long-term (i.e. *after* the next FF release), I would opt for a refinement of #2. From my use of FF, it is quite common to only want to enter the master password (e.g. to quickly access your email) and skip any other password prompts for later. So, a more flexible solution would be the following:
   * "Cancel" for master password prompt would cancel all prompts for which you have stored logins. This prompt should appear before any other prompts for which you have no credentials stored.
   * For the remaining prompts that have no stored logins there will be an additional "Cancel All" button. The number of prompts that will be canceled with this button should also be displayed (as Aaron proposes).

Unless the user wants to enter passwords for only some of the non-master password prompts, this solution is almost* optimal in terms of required user effort. Here is a table of usage scenarios and the associated user effort:

    |                             master password
    |----------------------------------------------------------------
    |       |           enter            |      do not enter
---------------------------------------------------------------------
    | enter | 1 master passwd            | 1 click                   
    | all   |        +                   |        +                  
    |       | 1 passwd per other prompt  | 1 passwd per other prompt
  p |       | (minimum effort)           | (minimum effort)
  a |----------------------------------------------------------------
o s | enter | 1 master passwd            | 1 click                          
t s | some  |        +                   |        +                   
h w |       | 1 click or passwd          | 1 click or passwd
e o |       | per other prompt           | per orther prompt         
r r |----------------------------------------------------------------
  d | enter | 1 master passwd            | 2 clicks                  
  s | none  |        +                   | (minimum effort + 1 click)
    |       | 1 click                    |
    |       | (minimum effort)           |                           


*The "almost" is because of the extra click in the last cell of the table.
Comment 38 Robert Kaiser (not working on stability any more) 2010-01-05 05:42:58 PST
Dolske, as long as we have popup password prompts for HTTP Auth, #1 is best, I think. If we (ever) switch to in-tab-prompts, then #4 seems to be best, but I think it should be up to the bug doing the different prompting to care about that.
Comment 39 Aaron Digulla 2010-01-05 05:45:52 PST
That said, I'd really prefer when the password prompts (normal and master) would be deferred until I actually click on the tab which needs them. But again, this is another story. Let's just get the "single password prompt" working. This bug is several years old and it's such a pain that I'm willing to spend money on the fix. Justin, do you have a PayPal account?
Comment 40 Alex Faaborg [:faaborg] (Firefox UX) 2010-01-05 20:55:39 PST
solution #1 is fine for the short term.  Ultimately I think we are going to want to switch to a model where the master password is presented in manner more analogous to OS user accounts (and is only entered once to log into the browser).
Comment 41 Henrik Skupin (:whimboo) 2010-01-06 14:24:55 PST
*** Bug 538253 has been marked as a duplicate of this bug. ***
Comment 42 Ulrich Windl 2010-01-07 03:07:28 PST
(In reply to comment #31)
IMHO those tabs requiring access to secrets protected by the master password would "queue" a request for the master password. Once the master password is entered, all those requests would be fulfilled. What should happen if the user presses "Cancel" for the master password? As the user doesn't know the order of requests in the queue, the obvious solution IMHO would be to annotate the master password dialog with the number of requests (e.g. "Master password (7 requests)"). Then, if cancelling that, it should be obvious that those 7 requests cannot continue until a master password is entered. Also the reasonable solution IMHO would be either to cancel all requests in the queue if the master password isn't entered, or to keep all requests in the queue, and re-open the prompt for the master password (while there are requests in the request queue). Obviously the second solution is not what the user might want.
Independent from that is the effect that most tabs will still want to access the secrets, so they might re-queue requests and thus re-open the prompt for the master password.
Comment 43 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-08 08:24:41 PST
Let's go with option 1, no need to add a "Cancel All" button since adding a string will make it impossible to back-port this to 1.9.2 when it's done.
Comment 44 Chris AtLee [:catlee] 2010-01-08 08:31:43 PST
(In reply to comment #31)

#1 is fine.  The only reason I hit cancel right now is so that I can dismiss the 4 or 5 dialogs that pop up for me on session restore.  Assuming I only got one prompt, the only reasons I would hit cancel would be because I'm offline, or know that I don't want any of the tabs from session restore.
Comment 45 Justin Dolske [:Dolske] 2010-01-11 17:32:52 PST
Created attachment 421176 [details] [diff] [review]
Patch v.3 (WIP)
Comment 46 Justin Dolske [:Dolske] 2010-01-11 17:36:09 PST
Patch v.3 happens to be applied on top of a minor security patch in my queue, so I've marked it as private, just to be extra cautious. I expect to be able to unmark it shortly.
Comment 47 etcoproc 2010-01-13 08:03:48 PST
My suggestion is: when Firefox starts and each tab begins to load, some of them ask for password. FF should ask user for master password (showing only one window). If the user clicked cancel, FF should stop asking. But when user enters some page that depends on password FF should ask ONCE more. If user cancels again, FF should quit asking (until FF restarts).
Comment 48 RNicoletto 2010-01-15 03:01:26 PST
(In reply to comment #31)
> I'm updating the patch for this bug, and am curious what people want to happen
> for one specific situation:

I vote for #1 too.
Comment 49 e-nighthawk 2010-01-15 06:30:52 PST
I haven't any problem with thunderbird 2.23 - now I update TB to version 3.0 and I have been asked for:
1. Master password
2. 1st Password to google calendar in lightning for 1st google calendar
3. 2nd Password to google calendar in lightning for 2nd google calendar

I have 3-rd google calendar yet (totally i have 3 calendars), but for 2 of them I use the same google login - so Thunderbird prompts me about passwords for callendars ony 2 times.

I think I should be prompt only once to type master password.
Comment 50 Justin Dolske [:Dolske] 2010-01-15 11:09:39 PST
Thanks for the feedback everyone, case #1 from comment 31 is what I'm implementing here. Any further refinements should be requested in separate bugs.
Comment 51 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-01-26 13:47:11 PST
*** Bug 542321 has been marked as a duplicate of this bug. ***
Comment 52 Mats Palmgren (:mats) 2010-02-11 21:18:31 PST
*** Bug 540796 has been marked as a duplicate of this bug. ***
Comment 53 Matthias Versen [:Matti] 2010-02-15 01:06:31 PST
*** Bug 543319 has been marked as a duplicate of this bug. ***
Comment 54 Matthias Versen [:Matti] 2010-02-15 01:06:39 PST
*** Bug 546184 has been marked as a duplicate of this bug. ***
Comment 55 Ria Klaassen (not reading all bugmail) 2010-02-18 23:32:54 PST
*** Bug 547166 has been marked as a duplicate of this bug. ***
Comment 56 Kevin Brosnan 2010-03-04 00:54:57 PST
*** Bug 550112 has been marked as a duplicate of this bug. ***
Comment 57 Joe Drew (not getting mail) 2010-03-22 19:36:05 PDT
Can the patch on this bug be unhidden now?
Comment 58 Justin Dolske [:Dolske] 2010-03-22 19:47:24 PDT
Comment on attachment 421176 [details] [diff] [review]
Patch v.3 (WIP)

Indeed, the other bug has already shipped.
Comment 59 Matthias Versen [:Matti] 2010-03-30 10:52:37 PDT
*** Bug 555672 has been marked as a duplicate of this bug. ***
Comment 60 Micah Gersten 2010-04-04 14:17:57 PDT
*** Bug 557142 has been marked as a duplicate of this bug. ***
Comment 61 Pascal 2010-04-13 01:20:52 PDT
hello, in what firefox release will it be included ?
Comment 62 Kevin Brosnan 2010-04-13 11:52:35 PDT
You are on the cc list so you should get an email when someone attaches an updated patch. Once the patch is approved and checked in there will be a comment in this bug with the mecurial mozilla-central revision that includes the fix for this bug.
Comment 63 Justin Dolske [:Dolske] 2010-04-14 17:43:50 PDT
Created attachment 439149 [details] [diff] [review]
Patch v.4

I need to write some tests for this. But I verified it works with manual testing, so it's good for a review pass.
Comment 64 Mark Banner (:standard8) 2010-04-15 01:52:21 PDT
(In reply to comment #63)
> Created an attachment (id=439149) [details]
> Patch v.4

Justin, I've just taken a quick glance at this - would this change also fix the case where code wants to get a login direct from the PW manager without being form fill or going through the prompt mechanism? i.e. calling findLogins direct?

Its just that being able to get a login without using the prompt code but having master password prompts not stack up is one of the last remaining pieces that mailnews is currently having to work around. Don't let me extend the scope of this bug, but I just want to make you aware of it as I haven't got around to filing a separate bug yet.
Comment 65 Justin Dolske [:Dolske] 2010-04-15 18:24:02 PDT
(In reply to comment #64)

> Justin, I've just taken a quick glance at this - would this change also fix the
> case where code wants to get a login direct from the PW manager without being
> form fill or going through the prompt mechanism? i.e. calling findLogins
> direct?

No. For other code (and extensions) using the password manager API, to avoid triggering another MP prompt you'll need to add code to check nsILoginManager.uiBusy (and if true, defer your work until an observer sees that the master password has been entered).

Unfortunately, it's not possible to fix this in a generic way without breaking the API. Once NSS has invoked the PSM callbacks to show the prompt the first time, logging in from that prompt won't work unless we unwind the stack all the way back to the original caller. So if a second request comes along while the prompt is up, the only choices are to show a second prompt or return an error.

The long term fix here is to either change how the token login / prompting is implemented, or make the login manager interfaces async (ie, return results through callbacks). Fodder for another bug, though!
Comment 66 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-04-19 17:32:34 PDT
Comment on attachment 439149 [details] [diff] [review]
Patch v.4

>+    get isLoggedIn() {
>+        let status = this._sdrSlot.status;
>+        this.log("SDR slot status is " + status);
>+        if (status == Ci.nsIPKCS11Slot.SLOT_READY ||
>+            status == Ci.nsIPKCS11Slot.SLOT_LOGGED_IN)
>+            return true;
>+        if (status == Ci.nsIPKCS11Slot.SLOT_NOT_LOGGED_IN)
>+            return false;
>+        throw "uhh unexpected slot status?";

uhh expected better error message?

>+                        // Don't trigger a 2nd master password prompt (it's
>+                        // unlikely we'll get here, because we normally don't
>+                        // attach this listener until after a MP entry).
>+                        if (this._pwmgr.uiBusy)
>+                            return;
>+

So how would we get to this case? I'm fine with it there if we can actually hit it, and it would be good to have a test to make sure that we're actually preventing this case.

>     autoCompleteSearch : function (aSearchString, aPreviousResult, aElement) {
>         // aPreviousResult & aResult are nsIAutoCompleteResult,
>         // aElement is nsIDOMHTMLInputElement
> 
>         if (!this._remember)
>-            return false;
>+            return null;

What's this change for?

>+        // If we're currently displaying a master password prompt, defer
>+        // processing this document until the user handles the prompt.
>+        if (this.uiBusy) {
>+            this.log("deferring fillDoc for " + doc.documentURI);
>+            let self = this;
>+            let observer = {
>+                QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
>+
>+                observe: function (subject, topic, data) {
>+                    self.log("Got deferred fillDoc notification: " + topic);
>+                    // Only run observer once.
>+                    Services.obs.removeObserver(this, "passwordmgr-crypto-login");
>+                    Services.obs.removeObserver(this, "passwordmgr-crypto-loginCanceled");
>+                    if (topic == "passwordmgr-crypto-loginCanceled")
>+                        return;
>+                    self._fillDocument(doc);
>+                },
>+                handleEvent : function (event) {
>+                    // Not expected to be called
>+                }
>+            };
>+            // Trickyness follows: We want an observer, but don't want it to
>+            // cause leaks. So add the observer with a weak reference, and use
>+            // a dummy event listener (a strong reference) to keep it alive
>+            // until the document is destroyed.
>+            Services.obs.addObserver(observer, "passwordmgr-crypto-login", true);
>+            Services.obs.addObserver(observer, "passwordmgr-crypto-loginCanceled", true);
>+            doc.addEventListener("mozCleverClosureHack", observer, false);
>+            return;
>+        }
>+

Trickyness indeed. Is this something we do elsewhere in the code? Is mozCleverClosureHack an event name that's standard or can we make that more password managery? Or even something super clever & ghostbustery?

>@@ -103,34 +102,47 @@ LoginManagerPromptFactory.prototype = {
>         for (hashKey in this._asyncPrompts)
>             break;
> 
>         if (!hashKey) {
>             this.log("_doAsyncPrompt:run bypassed, no prompts in the queue");
>             return;
>         }
> 
>+        // If login manger has logins for this host, defer prompting if we're
>+        // already waiting on a master password entry.
>+        var prompt = this._asyncPrompts[hashKey];
>+        var prompter = prompt.prompter;
>+        var [hostname, httpRealm] = prompter._getAuthTarget(prompt.channel, prompt.authInfo);
>+        var hasLogins = (prompter._pwmgr.countLogins(hostname, null, httpRealm) > 0);

Didn't we already do the hasLogins check when adding the prompt? Isn't it just |prompt.hasLogins| & we can avoid this additional DB hit?

>+        if (hasLogins && prompter._pwmgr.uiBusy) {
>+            this.log("_doAsyncPrompt:run bypassed, master password UI busy");
>+            return;
>+        }
>+
>         this._asyncPromptInProgress = true;
>+        prompt.inProgress = true;
>+
>         var self = this;
> 
>         var runnable = {
>             run : function() {
>                 var ok = false;
>-                var prompt = self._asyncPrompts[hashKey];
>                 try {
>                     self.log("_doAsyncPrompt:run - performing the prompt for '" + hashKey + "'");
>                     ok = prompt.prompter.promptAuth(prompt.channel,
>                                                     prompt.level,
>                                                     prompt.authInfo);

Can just make this |prompter| now.

It's looking good so a pending r+ - I'll just clear it for now & wait for tests & the above.
Comment 67 Brian 2010-04-25 12:18:37 PDT
Hello, thanks everyone for contributing to the resolution to this bug!  

A few questions:  
1)  Does Justin's patch also address multiple modal password dialogs raised by multiple tabs requesting *HTTPS* authentication? or is this patch specific to HTTP prompts only?
2)  Any expectations as to when this will hit trunk or any other maintenance branches of 3.5.5?  

Cheers!
Comment 68 João Carlos Mendes Luís 2010-04-25 18:17:43 PDT
I could be wrong, but I think this same bug is affecting Thunderbird.

Since I started using extensions that need master password access, I have to enter it once for each extension, and once for Thunderbird itself.

For example, the lightning extension, when configured to use a Google calendar shows this behavior.  Is this the same bug, or just an extension bug?
Comment 69 Micah Gersten 2010-04-25 18:20:36 PDT
(In reply to comment #68)
> I could be wrong, but I think this same bug is affecting Thunderbird.
> 
> Since I started using extensions that need master password access, I have to
> enter it once for each extension, and once for Thunderbird itself.
> 
> For example, the lightning extension, when configured to use a Google calendar
> shows this behavior.  Is this the same bug, or just an extension bug?

No, that's a separate issue, see bug 349641.
Comment 70 PTT 2010-04-27 01:10:23 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.9) Gecko/20100315 Firefox/3.5.9 GTB7.0 (.NET CLR 3.5.30729)

Since Firefox 3.5.x it's annoying me when restart old sessions, but after having typed in once I Cancel all Request's and reload all Tab's new. Then it'll work.
Comment 71 Anders Rickardsson 2010-05-14 02:04:18 PDT
Please solve this! This bug is so annoying! PLEASE!
Comment 72 Brian 2010-05-15 12:47:39 PDT
On a related note, can someone with Bugzilla privileges please confirm bug 524909 & offer some suggestions?  524909 is a bit more annoying as you are unable to type anything at all in the modal Master Password input dialog! (note:  the browser doesn't appear to be hung, it just seems to refuse any sort of text entry in the input field).   

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.5) Gecko/20091102 Firefox/3.5.5
Comment 73 Justin Dolske [:Dolske] 2010-05-15 20:21:26 PDT
(In reply to comment #66)

> >+                        // Don't trigger a 2nd master password prompt (it's
> >+                        // unlikely we'll get here, because we normally don't
> >+                        // attach this listener until after a MP entry).
> 
> So how would we get to this case?

Hmm, I guess we can't. I was thinking some mix of loading a login page, canceling the resulting MP prompt, and then later revisiting the page to type in a login... But the MP cancelation would throw, and fillDoc() would thus bail out before attaching an autocomplete listener. So, not just edge-casy but shouldn't be possible. Removed. Ditto for the other case of this.


> >         if (!this._remember)
> >-            return false;
> >+            return null;
> 
> What's this change for?

Noticed while checking code that autoCompleteSearch is supposed to return an nsIAutoCompleteResult (or null), not a bool.

> >+            doc.addEventListener("mozCleverClosureHack", observer, false);
> 
> Trickyness indeed. Is this something we do elsewhere in the code?

Yeah, I first used it over in browser.js for some plugin stuff.

> Didn't we already do the hasLogins check when adding the prompt? Isn't it just
> |prompt.hasLogins| & we can avoid this additional DB hit?

Hmm, good catch. I think that's deadwood from an early version of the patch. I've removed the |prompt.hasLogins| setting, since it wasn't used. Better to check it right when needed (otherwise we could miss a recently-saved login).
Comment 74 Justin Dolske [:Dolske] 2010-05-16 19:37:39 PDT
Created attachment 445640 [details] [diff] [review]
Patch v.5

Fixed nits, added a test. The test passes for me, but seems to trigger failures in other later tests. Looks like we're running into bug 437904. :( Someone's going to need to debug that problem before this can safely land.

Also, painful test-debugging note to self: avoid global "var crypto" because that can lead to accidentally using window.crypto elsewhere, giving unfortunate results.
Comment 75 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-05-19 16:28:41 PDT
Comment on attachment 445640 [details] [diff] [review]
Patch v.5

Looks great. Just 2 comments on the tests. r=zpao with them addressed and figuring out what's triggering the other failures (nothing jumped out at me while looking at them

>+++ b/toolkit/components/passwordmgr/test/test_master_password.html
>+    // XXX check that there's 1 MP window open

For both of these XXX, I realize that counting would be a more exact science, but you can you at least check |pwcrypt.uiBusy|. None of the tests are actually checking it. It would be nice to have explicit checks for uiBusy up the chain, but they all end up forwarding pwcrypt's value, so that's ok.

>+ // XXX do a test5ABC with clicking cancel?

Yea, probably should. Part of the expected behavior is that cancelling should cancel all pending ones.
Comment 76 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2010-05-19 16:42:52 PDT
*** Bug 566835 has been marked as a duplicate of this bug. ***
Comment 77 Jo Hermans 2010-06-10 02:18:07 PDT
*** Bug 571187 has been marked as a duplicate of this bug. ***
Comment 78 Dão Gottwald [:dao] 2010-06-25 05:39:37 PDT
*** Bug 574633 has been marked as a duplicate of this bug. ***
Comment 79 Kai Engert (:kaie) 2010-06-25 09:12:31 PDT
I tried to implement a more generic solution.
Instead of the patch from this bug, could you please try bug 177175 attachment 454051 [details] [diff] [review] and give feedback?
Comment 80 Matthias Versen [:Matti] 2010-06-29 15:19:07 PDT
*** Bug 575584 has been marked as a duplicate of this bug. ***
Comment 81 Jo Hermans 2010-07-11 04:07:52 PDT
*** Bug 577895 has been marked as a duplicate of this bug. ***
Comment 82 Justin Dolske [:Dolske] 2010-07-27 19:15:34 PDT
Created attachment 460738 [details] [diff] [review]
Patch v.6

Apply cleanly to trunk.
Comment 83 Shawn Wilsher :sdwilsh 2010-08-02 15:56:18 PDT
Comment on attachment 460738 [details] [diff] [review]
Patch v.6

a=sdwilsh, but the first sign of trouble on the tree because of this should mean this gets backed out stat.
Comment 84 Justin Dolske [:Dolske] 2010-08-02 16:04:08 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/1e989961cfed

\o/
Comment 85 Arie Paap [:wildmyron] 2010-08-04 00:01:04 PDT
*** Bug 584003 has been marked as a duplicate of this bug. ***
Comment 86 Matthias Versen [:Matti] 2010-08-13 13:55:29 PDT
*** Bug 587113 has been marked as a duplicate of this bug. ***
Comment 87 Arie Paap [:wildmyron] 2010-08-23 23:27:50 PDT
*** Bug 590020 has been marked as a duplicate of this bug. ***
Comment 88 Nicholas Patrick (Peter) Solin Jr. 2010-08-31 17:32:33 PDT
What version is this fix planned for? Because I started Firefox today and lone behold the password prompt spammed me 10 times.
Comment 89 Ryan VanderMeulen [:RyanVM] 2010-08-31 17:35:10 PDT
Firefox 4.0. You can confirm it with the current 4.0b4 release or the upcoming 4.0b5 release.
Comment 90 Matthias Versen [:Matti] 2010-09-03 15:35:28 PDT
*** Bug 593292 has been marked as a duplicate of this bug. ***
Comment 91 Jo Hermans 2010-09-29 12:34:51 PDT
*** Bug 600549 has been marked as a duplicate of this bug. ***
Comment 92 Pascal 2010-11-18 05:27:35 PST
hum now that this master password popup storm bug has been solved - it would be also time to solve the cookie confirmation popup storm also before firefox 4 release : big #515521
Comment 93 Matthias Versen [:Matti] 2010-11-23 07:35:08 PST
*** Bug 614184 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.