Comment 70 for bug 398128

(From update of attachment 439149)
>+ 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.