Closed Bug 499233 Opened 15 years ago Closed 14 years ago

multiple master password prompts triggered by filling form logins in multiple tabs

Categories

(Toolkit :: Password Manager, defect)

Product:
Toolkit
Component:
Password Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla2.0b3

People

(Reporter: Dolske, Assigned: Dolske)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

Patch v.6
39.78 KB, patch
sdwilsh
: approval2.0+
Details | Diff | Splinter Review 
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.]
Flags: blocking1.9.2?
isn't this a dupe of bug 356097 ?
No. Bug 356097 is about proxy auth, and will be fixed by bug 475053.
Ubuntu Bug:
https://bugs.launchpad.net/bugs/398128

User was seeing this in 3.0.11 and I'm seeing this in 3.5.
Flags: blocking1.9.2? → blocking1.9.2-
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?
Blocks: 460868
Blocks: 95397
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.
(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/).
Bug 475053 is not the same thing and it did not fix this.  (Already tested with nightlies after it was checked in.)
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.
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.
Depends on: 513534
Attached patch Patch v.1 (WIP) (obsolete) — — Splinter Review 
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.
Target Milestone: --- → mozilla1.9.3a1
Attached patch Patch v.2 (WIP) (obsolete) — — Splinter Review 
Cleaned up, still a few todos and needs tests, but I'll toss it to zpao for a prereview.
Attachment #400653 - Attachment is obsolete: true
Attachment #403604 - Flags: review?(paul)
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.
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?
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.
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?
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.
(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.
(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.
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.
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.
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.
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.
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...
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!
Depends on: CVE-2010-0172
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.
#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?
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.
Any are an improvement over the current situation, but my suggestion is to give the user the choice; add a Cancel All button.
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.
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.
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.
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.
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?
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).
(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.
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.
(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.
Attached patch Patch v.3 (WIP) (obsolete) — — Splinter Review 

        Attachment #403604 -
        Attachment is obsolete: true

    
    

    
  
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.

    
    

    
  
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).

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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.

    
  
Blocks: 177175

    
    

    
  
Can the patch on this bug be unhidden now?

    
    

    
  
Comment on attachment 421176 [details] [diff] [review]
Patch v.3 (WIP)

Indeed, the other bug has already shipped.

        Attachment #421176 -
        Attachment is private: false

    
    

    
  
hello, in what firefox release will it be included ?

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v.4 (obsolete)
        — 
          — Splinter Review
      

    


  
I need to write some tests for this. But I verified it works with manual testing, so it's good for a review pass.

        Attachment #421176 -
        Attachment is obsolete: true

        Attachment #439149 -
        Flags: review?(paul)

    
    

    
  
(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.

    
    

    
  
(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 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.

        Attachment #439149 -
        Flags: review?(paul)

    
    

    
  
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!

    
    

    
  
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?

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
Please solve this! This bug is so annoying! PLEASE!

    
    

    
  
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

    
    

    
  
(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).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v.5 (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #439149 -
        Attachment is obsolete: true

        Attachment #445640 -
        Flags: review?(paul)

    
    

    
  
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.

        Attachment #445640 -
        Flags: review?(paul) → review+

    
  
Depends on: 524909

    
  
Depends on: 437904
No longer depends on: 524909

    
    

    
  
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?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v.6
        — 
          — Splinter Review
      

    


  
Apply cleanly to trunk.

        Attachment #445640 -
        Attachment is obsolete: true

    
    

    
  
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.

        Attachment #460738 -
        Flags: approval2.0+

    
    

    
  
Pushed http://hg.mozilla.org/mozilla-central/rev/1e989961cfed

\o/
Flags: in-testsuite+
Target Milestone: mozilla1.9.3a1 → mozilla2.0b3

    
  
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
What version is this fix planned for? Because I started Firefox today and lone behold the password prompt spammed me 10 times.

    
    

    
  
Firefox 4.0. You can confirm it with the current 4.0b4 release or the upcoming 4.0b5 release.

    
    

    
  
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

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: