+++ This bug was initially created as a clone of Bug #516464 +++

The patch in Bug 516464 fails to test either the subject argument (to get the right server) or the data argument (for type of change) on a "passwordmgr-storage-changed" notification.

Hmm, looks like I messed up with the review back then. Please contact the manager in charge now. Unbelievable that no one has complained since TB 60.

It undoubtedly manifests as 'cannot get new messages' or 'cannot sign into imap' and the connection is very non obvious.

I just deleted a stored IMAP password for one account and it seems to work fine. It prompts me for the password for new folder connections for that account and doesn't seems to affect other accounts. Am I missing something?
In my testing I often do this type of thing and haven't noticed a problem.

In console, issue gFolderDisplay.displayedFolder.server.QueryInterface(Ci.nsIImapServerSink).userAuthenticated;
while each (of 2 for example) account's Inbox folder is selected; the result for both will be true if signed in.
Then remove from password manager one of the accounts, test again. Both will be false. And if you look at the code it's obvious that both instances of nsIMsgIncomingServer have an observer that gets the notification and does not test who it's for. And it's then necessary to restart, as getting new messages or reselecting the folders doesn't wfm.

(In reply to alta88 from comment #0)

The patch in Bug 516464 fails to test either the subject argument (to get the right server) or the data argument (for type of change) on a "passwordmgr-storage-changed" notification.

Looking at
https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/toolkit/components/passwordmgr/LoginHelper.jsm#1519
and
https://searchfox.org/mozilla-central/search?q=notifyStorageChanged&redirect=false
data appears to be "add/remove/modifyLogin", etc. and the subject is a nsILoginMetaInfo:
https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/storage-json.js#214
which has an origin:
https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/toolkit/components/passwordmgr/storage-json.js#254.

So as far as I can can see, the code snippet needed is:

nsCOMPtr<nsILoginMetaInfo> login = do_QueryInterface(subject);
nsAutoString origin;
login->GetOrigin(origin);

and then dump out that origin and see whether we can compare it to a property of nsIMsgIncomingServer. And if it doesn't match, ignore the call.

Can you try that, Gene?

This patch shows that comment #5 was mostly wrong. Yes, we get a nsILoginMetaInfo, but that's not useful for anything:
https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/nsILoginMetaInfo.idl
It does not have an origin attribute.

I've run out of ideas here. Any hints welcome.

Isn't it like this: The ForgetSessionPassword() removes the password from some local "cache". Since the observer is called for all servers, that affects all servers, so as per the bug summary, they all go to unauthenticated. That's not an issue for servers storing the password in the password manager, they will just fetch it again. Only users who don't use the password manager will be affected. (Disclaimer: This could be totally wrong.)

The notification sends an nsLoginInfo which certainly has an origin and it should be very easy to compare to the server url.

https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/storage-json.js#275
https://searchfox.org/mozilla-central/source/toolkit/components/passwordmgr/LoginInfo.jsm#16

You should also check that data is "removeLogin". It could conceivably be an add or change also. Also note that imap overrides ForgetSessionPassword().

Well, does the overriding matter? If we don't call it, we don't call it, overridden or not. What am I missing?
As for the action: In case of a change we want to forget the cached password, no? I don't know how the add case would look like. The user adds the server string and a password in the password manager? Then we won't have anything cached so far, so forgetting what we don't have won't hurt.

It may not matter, but is more.. robust. The override, just for imap, does one thing: sets userAuthenticated to false.

Sorry, but I don't see what needs to be more robust here. If we get a notification that targets our server, we forget anything we have by calling ForgetSessionPassword(). With the patch that won't be called unnecessarily any more, so no resetting of userAuthenticated will take place. That fixes the bug you reported.

Updated as per comment #10.

What I'm saying is that for "passwordmgr-storage-changed" notification, you're assuming it will always mean ForgetSessionPassword() in both the base case and override case. Have you considered whether that is always the correct action, even if the data value is not "removeLogin"? So that we can avoid having a Third attempt here - what happens when an "addLogin" is potentially notified?

Hmm, I believe I have considered the actions which aren't "removeLogin" (comment #12 and #14). In case of a notification of a change, we equally need to forget out value. "addLogin" seems hypothetical, the user would have to go into the password manager and fill in the provider, username and password. Even if they do, forgetting what we have (nothing) is not going to do any harm. I don't want to distinguish the different notification cases. Which danger do you see in "addLogin"?

Can you please set "checkin-needed-tb" for me, I can't edit the flags.

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/a70d4c11b52f
Check server before resetting password. r=benc

Looks like the dreaded "third attempt" was necessary after all. Apologies!
Please set the checkin flag after the review.

The JS code matches the C++ code:

+    nsCOMPtr<nsILoginInfo> loginInfo = do_QueryInterface(aSubject);
+    if (loginInfo) {
[snip]
+      if (!fullName.Equals(NS_ConvertUTF16toUTF8(hostnameInfo))) return NS_OK;

The logic is that if we don't receive a nsILoginInfo here, we proceed as before resetting the password. One could argue that in case we don't receive the nsILoginInfo targeting our server, we shouldn't reset. In practice we will always receive the nsILoginInfo so it doesn't matter.

Ben, please let me know if you prefer alta88's variation and I'll roll the change to both JS and C++ code into the follow-up.

Let's fix it now then.

Third time lucky?

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/f1ca6ee69568
Follow-up: Password manager stores mailbox: instead of pop3:. Also check more specifically. r=benc

Sigh, that now causes test failures. In test_smtpServer.js we have

  // Trigger the change event of password manager.
  Services.logins.setLoginSavingEnabled("smtp://localhost", false);
  equal(server.password, "", "Password should be cleared.");

Looks like this now no longer triggers the password reset. I'll take a look.

OK, dumping out this dump(=== ${subject instanceof Ci.nsILoginInfo} |${subject.hostname}| |${this.hostname}| ${data}\n); gives:
=== false |undefined| || hostSavingDisabled
So we have no nsILoginInfo. I suggest we go back to my initial suggestion to only check the hostname when we can.

Whoever comes here first:
Please back out https://hg.mozilla.org/comm-central/rev/f1ca6ee69568 and land the earlier version in attachment 9206859 [details] [diff] [review].

P.S.: BMO is painfully slow right now :-( - Acutally, I get a 503.

Backout: https://hg.mozilla.org/comm-central/rev/29775cd7a079efccda9ad38b8bfcfd988ae2a196

Is there any issue with landing attachment 9206859 [details] [diff] [review]? See comment #24 for the review. Unfortunately Ben's suggestion was implemented and caused the test failure, so we need that original follow-up patch now. Without it, resetting a password for a POP3 account doesn't work at all.

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/3a4a3d4aa393
Follow-up: Password manager stores mailbox: instead of pop3:. r=benc

Comment on attachment 9206859 [details] [diff] [review]
1695703-follow-up-mailbox-pop3.patch (landed, comment #32)

This should conclude the work here. In case anyone isn't happy with this solution, please file a follow-up bug.