Steps To Reproduce: 1. Set Master Password properly 2. Open https://accounts.firefox.com/signin?context=web 3. Click [cancel] when Master Password dialog pops up --- no bug, it works as expected 4. Click password field --- Master Password dialog pops up, it maybe expected behavior. 5. Click [cancel] to close the dialog --- Master Password dialog pops up again. --- Bug Actual Results: Master Password dialog pops up again. I can not do anything operate Firefox. Expected Results: Master Password dialog should disappear.

This is since Firefox52. Firefox51 is not affected.

Regression window: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=ed8b016100297fdd10f45f02ebf0202b91559a23&tochange=2f89807ca30c474bed545f10c9e52a434f297a71 Regressed by: 2f89807ca30c Matthew Noorenberghe — Bug 1334026 - Show the the insecure field warning on insecure password fields even if they're not marked. r=mconley

Matt, I'm still sorting out how to best write a test for this (suggestions welcome). Feel free to review the first part. I initially wanted to ask for feedback instead of review, but this seems to work quite well. So please let me know if you think there's a flaw in the general approach.

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. https://reviewboard.mozilla.org/r/122324/#review126550 ::: modules/libpref/init/all.js:4426 (Diff revision 1) > pref("signon.formlessCapture.enabled", true); > pref("signon.storeWhenAutocompleteOff", true); > pref("signon.debug", false); > pref("signon.recipes.path", "chrome://passwordmgr/content/recipes.json"); > pref("signon.schemeUpgrades", false); > +pref("signon.masterPasswordReprompt.timeout_ms", 1800000); Isn't searchLogins used for many different UIs? And is the timeout applying to all of them? If so, this timeout is way too high as I don't think there's a way for a user to change their mind to decide to login to MP. Ideally it would be just big enough so we don't cause what appears to be an infinite loop. For the purposes of 52 we really just need to fix autocomplete I think, does that make a fix easier/smaller? ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61 (Diff revision 1) > + if (e.result == Cr.NS_ERROR_ABORT) { > + log("User cancelled master password prompt."); Did you consider using the observer notification instead? It may give you more flexibility. I was thinking that the autocompleteSearch code would check `isLoggedIn` and not touch pwmgr storage if the prompt was recently cancelled (checking a timestamp like you have). What do you think about that?

We could still take a fix for beta 53 and esr52 but too late for 52 release.

(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #6) > Isn't searchLogins used for many different UIs? And is the timeout applying > to all of them? If so, this timeout is way too high as I don't think there's > a way for a user to change their mind to decide to login to MP. Ideally it > would be just big enough so we don't cause what appears to be an infinite > loop. > > For the purposes of 52 we really just need to fix autocomplete I think, does > that make a fix easier/smaller? You're right, I think we only need to fix autocomplete. I updated that. I changed the timeout to be 15 minutes, but I don't think we should make it much shorter. I wasn't able to consistently break the infinite loop, it's occurring on all sorts of things like focusing or clicking the password field or focusing the window. Ideally someone would look into the different ways satchel is calling the autocomplete because something must be broken over there. > > ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61 > (Diff revision 1) > > + if (e.result == Cr.NS_ERROR_ABORT) { > > + log("User cancelled master password prompt."); > > Did you consider using the observer notification instead? It may give you > more flexibility. I was thinking that the autocompleteSearch code would > check `isLoggedIn` and not touch pwmgr storage if the prompt was recently > cancelled (checking a timestamp like you have). What do you think about that? That didn't fire when I tried it, so I kept it like this :/

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. https://reviewboard.mozilla.org/r/122324/#review131782 Sorry Johann but I think this has some problems. Also there really should be a test for this if we want to uplift. ::: modules/libpref/init/all.js:4409 (Diff revision 2) > pref("signon.formlessCapture.enabled", true); > pref("signon.storeWhenAutocompleteOff", true); > pref("signon.debug", false); > pref("signon.recipes.path", "chrome://passwordmgr/content/recipes.json"); > pref("signon.schemeUpgrades", false); > +pref("signon.masterPasswordReprompt.timeout_ms", 900000); // 15 Minutes Add to the comment stating it's for MP caused by autocomplete only for now. If it was general purpose I think it should be much lower but for autocomplete only the user can simply reload the page if they want to force the MP dialog to come back. ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:40 (Diff revision 2) > * @deprecated > */ > _recipeManager: null, > > + // Tracks the last unsuccessful master password attempt. > + _lastMPLoginAttempt: Math.NEGATIVE_INFINITY, `_lastMPLoginCancelled`? ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:42 (Diff revision 2) > _recipeManager: null, > > + // Tracks the last unsuccessful master password attempt. > + _lastMPLoginAttempt: Math.NEGATIVE_INFINITY, > + > + _searchLogins(formSubmitURL, hostname) { This method needs a better name e.g. `_searchAndDedupeLogins` ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:61 (Diff revision 2) > + // Dedupe so the length checks below still make sense with scheme upgrades. > + // Below here we have one login per hostPort + action + username with the > + // matching scheme being preferred. This comment belongs with the original code from line 320 ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:258 (Diff revision 2) > + let timeDiff = Date.now() - this._lastMPLoginAttempt; > + if (!Services.logins.isLoggedIn && timeDiff < this._repromptTimeout) { Please avoid accessing `Date.now` (which can be quite slow) in this somewhat hot code path (running on many keystrokes in a username+password field) if `isLoggedIn` is true. I think you can inline it and wrap the conditions in two lines. ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:260 (Diff revision 2) > + log("Not searching logins for autocomplete since the master password " + > + `prompt was last cancelled ${timeDiff / 1000} seconds ago.`); Did you mean to also `Math.round`? That would reduce the spamminess of the logging since the console is supposed to dedupe identical consecutive log messages with a counter. ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:260 (Diff revision 2) > + log("Not searching logins for autocomplete since the master password " + > + `prompt was last cancelled ${timeDiff / 1000} seconds ago.`); > + return; > + } Are you sure it's fine to return here? I think we will leak the request in LoginManagerContent if we don't respond. See https://dxr.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd/toolkit/components/passwordmgr/LoginManagerContent.jsm#208-209,214,216,218,233,252 I think we need to respond with a message for the requestID with an empty array and double check this won't break the insecure password warning (I think it already handles adding the warning row on an empty array but not 100% sure). ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:490 (Diff revision 2) > .CustomEvent("InsecureLoginFormsStateChange")); > }, > }; > + > +XPCOMUtils.defineLazyPreferenceGetter(LoginManagerParent, "_repromptTimeout", > + "signon.masterPasswordReprompt.timeout_ms", 1800 * 1000); Make this 900000 to match the default.

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. https://reviewboard.mozilla.org/r/122324/#review131782 > Please avoid accessing `Date.now` (which can be quite slow) in this somewhat hot code path (running on many keystrokes in a username+password field) if `isLoggedIn` is true. I think you can inline it and wrap the conditions in two lines. Good point, though I moved isLoggedIn to a separate if-condition to be able to reuse timeDiff > Are you sure it's fine to return here? I think we will leak the request in LoginManagerContent if we don't respond. See https://dxr.mozilla.org/mozilla-central/rev/f914d40a48009c5acd1093e9939cc0ec035696dd/toolkit/components/passwordmgr/LoginManagerContent.jsm#208-209,214,216,218,233,252 > > I think we need to respond with a message for the requestID with an empty array and double check this won't break the insecure password warning (I think it already handles adding the warning row on an empty array but not 100% sure). Oh, good catch. I didn't even know this request caching existed.

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. https://reviewboard.mozilla.org/r/122324/#review139876 Thanks for pushing this over the finish line! ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:39 (Diff revision 3) > + // Tracks the last time the user cancelled the master password prompt, > + // to avoid spamming master password prompts on autocomplete searches. > + _lastMPLoginCancelled: Math.NEGATIVE_INFINITY, So one thing this doesn't take into account is Sync requesting MP login… I guess that can be done in a follow-up, if necessary since this will still improve the situation. ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:43 (Diff revision 3) > > + // Tracks the last time the user cancelled the master password prompt, > + // to avoid spamming master password prompts on autocomplete searches. > + _lastMPLoginCancelled: Math.NEGATIVE_INFINITY, > + > + _searchAndDedupeLogins(formSubmitURL, hostname) { Nit: I'm trying to phase out the `formSubmitURL` and `hostname` names since they are inaccurate so please use `actionOrigin` and `formOrigin` instead. ::: toolkit/components/passwordmgr/LoginManagerParent.jsm:43 (Diff revision 3) > + _searchAndDedupeLogins(formSubmitURL, hostname) { > + let logins; Please re-order the two arguments since `hostname` is the important one and `formSubmitURL` may be removed one day. (It's been talked about many times)

Comment on attachment 8865274 [details] Bug 1348791 - Add a test for autocomplete master password timeout. https://reviewboard.mozilla.org/r/136916/#review139882 ::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:15 (Diff revision 1) > + is(dialog.args.title, "Password Required"); > + dialog.ui.button1.click(); > + }); > +} > + > +add_task(function*() { Nit: I think it makes sense to always name test tasks since the name shows in output and makes it easier to see what's going on. It would be good to have to test name and/or a comment explaining what this test is trying to verify: Prevent a MP prompt for autocomplete after being cancelled. ::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:24 (Diff revision 1) > + // Add a master password. > + let pk11db = Cc["@mozilla.org/security/pk11tokendb;1"].getService(Ci.nsIPK11TokenDB); > + let token = pk11db.getInternalKeyToken(); > + token.changePassword("", "masterpass"); This can be `LoginTestUtils.masterPassword.enable()` ::: toolkit/components/passwordmgr/test/browser/browser_master_password_autocomplete.js:27 (Diff revision 1) > + token.changePassword("", "masterpass"); > + You should probably have a registerCleanupFunction with `LoginTestUtils.masterPassword.disable();` to avoid a MP interfering with future tests?

Comment on attachment 8865274 [details] Bug 1348791 - Add a test for autocomplete master password timeout. https://reviewboard.mozilla.org/r/136916/#review139882 > Nit: I think it makes sense to always name test tasks since the name shows in output and makes it easier to see what's going on. It would be good to have to test name and/or a comment explaining what this test is trying to verify: Prevent a MP prompt for autocomplete after being cancelled. I agree! Just forgot about it :) > You should probably have a registerCleanupFunction with `LoginTestUtils.masterPassword.disable();` to avoid a MP interfering with future tests? Good catch!

Pushed by jhofmann@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9576d7dc7efc Add a timeout to master password prompt when searching logins. r=MattN https://hg.mozilla.org/integration/autoland/rev/dd246ec28146 Add a test for autocomplete master password timeout. r=MattN

https://hg.mozilla.org/mozilla-central/rev/9576d7dc7efc https://hg.mozilla.org/mozilla-central/rev/dd246ec28146

Please request uplift on this when you get a chance.

This makes using the master password kind of annoying.

Yup, we should have enough Nightly bake time now. I'll make the request.

I don't think tracking 55 makes sense though, given that it's fixed already.

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. Approval Request Comment [Feature/Bug causing the regression]: Bug 1334026 [User impact if declined]: Users basically can not cancel the master password prompt on certain pages, they have to enter the right one, probably causing some of them disable master password completely. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: - Use a master password - Go to e.g. http://http-login.badssl.com/ - Enter some credentials and submit the form - Save the credentials in the password manager - Restart the browser, don't enter the master password on startup - Go to e.g. http://http-login.badssl.com/ - You should only be prompted for your master password once until reloading the page, even if you enter things in the input fields [List of other uplifts needed for the feature/fix]: There is an accompanying test for this in attachment 8865274 [details]. Might want to land that, too. [Is the change risky?]: Medium [Why is the change risky/not risky?]: We touch password manager frontend code, if something goes wrong we could potentially break login autofill but it's all covered with automated tests so we should be fine. [String changes made/needed]: None

Track 54+ as regression. Hi Alice, Can you help check if this issue is fixed in the latest Nightly?

I verified that the problem was fixed in [1]current Nightly55. [1] https://hg.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c Mozilla/5.0 (Windows NT 10.0; WOW64; rv:55.0) Gecko/20100101 Firefox/55.0 ID:20170523030206

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. Fix a regression related to Master Password dialog and was verified. Beta54+. Should be in 54 beta 11.

https://hg.mozilla.org/releases/mozilla-beta/rev/7d5ea2d3503b https://hg.mozilla.org/releases/mozilla-beta/rev/fd12d3d5b412

I'd like to wait a few more days to catch unexpected regressions before uplifting this to ESR52

We reproduced the initial issue using affected builds (Fx 53.0.2 and Fx 54beta8), verified that the issue is fixed using Firefox 54 beta 11 build 2 across platforms (Windows 7 64bit, Windows 10 32bit, macOS 10.12.5 and Ubuntu 16.04 64bit).

Comment on attachment 8849530 [details] Bug 1348791 - Add a timeout to master password prompt when searching logins. This is a core scenario, fix has been verified on Nightly55 and Beta54, ESR52+

https://hg.mozilla.org/releases/mozilla-esr52/rev/4a52b2cfc49c https://hg.mozilla.org/releases/mozilla-esr52/rev/27083ff87fb4