516464 - Get Mail after Remove Password (in pw manager) doesn't prompt for new password (remembers/keeps deleted/old/bad password in cache)

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Thomas D. (:thomas8)]

Get Mail after Remove Password (in pw manager) doesn't prompt for new password (in the current session), it should. Otherwise, why is user removing the password in the first place?
Note that we currently can't edit saved passwords. In the case where user removes a wrong/outdated password we shouldn't continue to use it.

Not sure if this is also a privacy issue, but user can exit TB as a workaround.

Comment 1

[WADA:World Anti-bad-Duping Agency]

With which build? (Bug 508381 was fixed on 2009-09-06)
POP3? IMAP? Or both?

Deleted password was correct one? Or wrong one because password was changed at server?
If former, I think it's resolved by Bug 508381.
If latter, and if server side RFC violation exists, see Bug 346944 Comment #11. See also bugs listed in dependency tree for Bug 428611.

Comment 2

[Thomas D. (:thomas8)]

Attachment: POP3 Log including re-use of bad password after removal, no prompt

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090914 Shredder/3.1a1pre

Hi Wada,

it's POP3, deleted password was wrong one because I entered it wrong, server side RFC violation may exist, don't know, but it doesn't matter because as soon as I remove the password from pw manager it should not be used any longer by TB and should be removed from cache. I'm running -safe-mode -p with pop3 log (attached).

What I did for the log was start from scratch (no pw saved), initial getmail: enter wrong pw, save it, then remove it, thereafter(!): getmail again: TB reuses bad pw from cache and doesn't prompt me in any way.

Comment 3

[Thomas D. (:thomas8)]

Wada, you've got a mail.

Comment 4

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> POP3 Log including re-use of bad password after removal, no prompt

AUTH LOGIN -> -ERR -> -> Silent kill of connection by server -> Try next USER -> No response -> Waits forever.
Same phenomenon as Bug 428611(AUTH PLAIN, -ERR, try next AUTH LOGIN, no response).

Because of try of other available authentication mechanism, and because -ERR doesn't always mean "wrong password"(client can't know reason of -ERR), Tb looks to keep old/wrong cached password. (It seems no chance to discard cached password.)
If Tb checks password manager entry at least when re-execution of downloading by "Get Msgs" like operation, manual recovery is possible, even if server side RFC violation exists, and even if Tb still fails to detect silent kill of connection by server.

> Gecko/20090914 Shredder/3.1a1pre

I don't know whether all patches are timely landed on Geck 1.9.3 build or not.
Can you reproduce problem with Gecko 1.9.1 build(3.0a4pre)?
> http://ftp.mozilla.org/pub/mozilla.org/thunderbird/nightly/latest-comm-1.9.1/

Comment 5

[Thomas D. (:thomas8)]

(In reply to comment #4)
> (In reply to comment #2)
> > POP3 Log including re-use of bad password after removal, no prompt
> Because of try of other available authentication mechanism, and because -ERR
> doesn't always mean "wrong password"(client can't know reason of -ERR),

Well, with some creativity, TB could know, just filter for keywords "user" / "password" in the -ERR description:

> 0[1828140]: RECV: -ERR Username or password incorrect.

> If Tb checks password manager entry at least when re-execution of downloading
> by "Get Msgs" like operation, manual recovery is possible, even if server side
> RFC violation exists, and even if Tb still fails to detect silent kill of
> connection by server.

If TB *checked* password manager entry each time when executing Get Mail, then manual recovery would be possible, and this bug would be solved, you're right. Currently it doesn't check, therefore the bug.

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> (In reply to comment #4)
> Well, with some creativity, TB could know, just filter for keywords "user" /
> "password" in the -ERR description:
> 
> > 0[1828140]: RECV: -ERR Username or password incorrect.

That doesn't work for servers written in different languages and could be prone to failure, e.g.

"-ERR Username rejected due to server error."

(so I don't think you'd get that exact message but something similar could be created which would break the rule).

Comment 7

[Thomas D. (:thomas8)]

(In reply to comment #6)

> > > 0[1828140]: RECV: -ERR Username or password incorrect.
> That doesn't work for servers written in different languages and could be
> prone to failure, e.g.
> "-ERR Username rejected due to server error."
> (so I don't think you'd get that exact message but something similar could be
> created which would break the rule).

You are right, it is fuzzy. But error messages that contain the words "user" or "password" don't usually report server errors. I don't know about languages, I thought servers always communicate in English.

But that's all irrelevant with respect to this bug: If there's no password stored in password manager (e.g. because I have just removed it), TB should ask me for the password no matter what and not continue to use an outdated cached password.

Comment 8

[WADA:World Anti-bad-Duping Agency]

(Brief summary)

There are four issues.

(1) Server silently kills connection(close socket at server side, probably). This is apparently RFC violation by server, and root cause of the problem.

(2) It looks that Tb's mail code can't(or doesn't) detect connection loss(socket close), even after OS(TCP level) detects connection loss(no connection status by netstat of MS Win), and even though Tb's socket interface code detects socket close(hooked by nsSocketTransport:5 of NSPR logging). I think this is major cause of "Tb waits response forever when (1) exists" after (a) next AUTH LOGIN(Bug 428611), (b) USER(this bug), (c) CAPA(Bug 429069). See Bug 490140 Comment #12.

(3) When Bug 428611 and this bug, cached old/wrong password by Tb(not one held in DB of password manager) is not cleared, because it's fall backing to other authentication mechanism.
For Bug 428611 and this bug, dialog like next can be a solution.
> Error in an authentication mechanism. Retry other mechanism? Reenter password"
But "falling back to other authentication mechanism" is mainly for inappropriately setup server. So such dialog will produce much annoyance of some users. I think number of the "some users" is far larger than number of victims of Bug 428611 and this bug.

(4) Another issue was exposed by this bug.
- After remove of password entry by Passwors Manager, Tb won't detect it.
- Even after remove of password, Tb continues to use cached old/wrong password.
If this 4-th problem is resolved, manual recovery is possible in Bug 428611's case and this bug's case, even if (1) exists and (2) still remains and (3) won't be implemented.

Comment 9

[Thomas D. (:thomas8)]

(In reply to comment #8)
Thanks Wada for your detailed analysis.
4) Shows that this bug is clearly a problem of TB that should be fixed.

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 400693 [details]
POP3 Log including re-use of bad password after removal, no prompt

Marking private at the request of the poster, as it contains non-public information.  Thomas, can you upload a sanitized version?

Comment 11

[Ronald Killmer]

Can we get an updated log attachment that does not contain private data.

Comment 12

[Thomas D. (:thomas8)]

(In reply to Ronald Killmer from comment #11)
> Can we get an updated log attachment that does not contain private data.

I don't think the log will help anything, because you'll just see that TB sent a wrong password. The problem occurs before that:

After user removes stale password via TB's password manager, the stale password will still be used by TB (as confirmed by WADA in comment 8).

I don't have time for this now, but here's the STR:

1) Pop3 account with saved password in TB, "oldpassword"
2) on your mail server, change the password to "newpassword"
3) start TB with "automatically download new messages at startup" etc.
-> download will fail because of wrong password
4) do *not* provide a new password at this stage, just cancel
5) from TB's password manager, remove "oldpassword" (so that you have NO password saved for that account)
6) in the same TB session, click "Get messages" for that account

Actual result

TB still uses the "oldpassword" even though it has already been deleted by user

Expected result

TB should not use "oldpassword" because it has already been deleted by user
TB should prompt for new password, and offer to save it

(I don't have time to actually test this right now)

Comment 13

[Josiah Bruner [:jsbruner]]

I can verify the STR above using IMAP (rather than Pop3).

I will take a look at this.

Comment 14

[Josiah Bruner [:jsbruner]]

Attachment: Bug 516464 - Remove passwords from cache when password manager contents are changed to prevent stale password attempts.

Review commit: https://reviewboard.mozilla.org/r/236670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236670/

Comment 15

[Josiah Bruner [:jsbruner]]

https://reviewboard.mozilla.org/r/236670 provides a patch which "fixes" this issue on IMAP. That is, performing the STR in comment 12 now prompts for new password, rather than attempting the old one.

To do this, nsImapProtocol::GetPassword() now always attempts to get the password from the password manager (or user), and ignores cached values. For this to work nsImapIncomingServer::AsyncGetPassword() now also starts by clearing the existing password from cache so that a re-attempt is made.

So although this works, it is quite subpar. In an ideal world we should get notified that an account password has been removed from the password manager, so that we can call nsImapProtocol::ForgetPassword(). However, the TB-specific password manager code seems to only open the toolkit password manager, so I'm not sure how we would get such a notification. Maybe the reviewer will some idea?

Comment 16

[Josiah Bruner [:jsbruner]]

(I should mention I only tested on macOS. I will try to trigger a try-build, but I'm not confident I still can.)

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967977 [details]
Bug 516464 - Remove passwords from cache when password manager contents are changed to prevent stale password attempts.

Gene, can you please test this. If you're not familiar with MozReview, your best bet is to
hg qimport https://reviewboard-hg.mozilla.org/comm-central/rev/a14cd9da7db2675977642675940f4f1ea6d5c4c4

The system says to use "hg import", which you will have to "hg out + hg strip -r" later. If you have qimportbz installed, the line above will get you a patch you can apply.

Joshua, this is a fix for IMAP, so how about POP, SMTP, NNTP? According to comment #12 it's also an issue for POP. Clearing the review for now since the patch is incomplete.

If I read the patch correctly, it removes the buffered password with the effect that it will always be obtained from the password manager which might not strictly be a performance enhancer.

As for getting notification from the password manager: Could you please document your findings so far. I've looked briefly and there is a bunch of stuff in toolkit/components/passwordmgr/, but I can't see anything that would send a notification if a password is removed.

You can always find the peer of the module from https://wiki.mozilla.org/Modules/Core (well, I can't see anything with passwords listed there) or a frequent author in that directory and ask them.

In fact, I'll do that now:
Gijs, I know very little about how passwords are stored :-( Apparently TB caches passwords once retrieved from the password manager, so when a password is removed, we don't notice it. Is there any notification available for password removals/changes? Or could you give us any other hint or tip?

Comment 18

[Josiah Bruner [:jsbruner]]

(In reply to Jorg K (GMT+1) from comment #17)
> Comment on attachment 8967977 [details]
> Bug 516464 - Workaround stale IMAP server passwords by making
> nsImapProtocol::GetPassword() always get from pswdmgr or from user input
> 
> Gene, can you please test this. If you're not familiar with MozReview, your
> best bet is to
> hg qimport
> https://reviewboard-hg.mozilla.org/comm-central/rev/
> a14cd9da7db2675977642675940f4f1ea6d5c4c4
> 
> The system says to use "hg import", which you will have to "hg out + hg
> strip -r" later. If you have qimportbz installed, the line above will get
> you a patch you can apply.
> 
> [Josiah], this is a fix for IMAP, so how about POP, SMTP, NNTP? According to
> comment #12 it's also an issue for POP. Clearing the review for now since
> the patch is incomplete.

Good question, I forgot to mention that. I started with IMAP to see if such a workaround would be acceptable at all (I forgot about the feedback flag, my apologies). As you hint at below, this will almost certainly come with (at least some) performance penalty. In addition, if there are test cases that depend on passwords being cached, a bunch of tests may now fail. I will need a try build to verify this (lots of xpc-shell test cases fail for me locally, but it is not obvious that my patch causes them. I will need to try again with a clean tree). Finally I don't have POP, SMTP, or NNTP accounts, so I will need to spend more work testing those.

I'll try to implement this workaround for those other account types....


> 
> If I read the patch correctly, it removes the buffered password with the
> effect that it will always be obtained from the password manager which might
> not strictly be a performance enhancer.
> 
> As for getting notification from the password manager: Could you please
> document your findings so far. I've looked briefly and there is a bunch of
> stuff in toolkit/components/passwordmgr/, but I can't see anything that
> would send a notification if a password is removed.

That is where I looked as well, and did not see anything. In toolkit/components/passwordmgr/content's Startup() I can see:

  // be prepared to reload the display if anything changes
  Services.obs.addObserver(signonReloadDisplay, "passwordmgr-storage-changed");


Where signonReloadDisplay is defined as:


let signonReloadDisplay = {
  observe(subject, topic, data) {
    if (topic == "passwordmgr-storage-changed") {
      switch (data) {
        case "addLogin":
        case "modifyLogin":
        case "removeLogin":
        case "removeAllLogins":
          if (!signonsTree) {
            return;
          }
          signons.length = 0;
          LoadSignons();
          // apply the filter if needed
          if (filterField && filterField.value != "") {
            FilterPasswords();
          }
          signonsTree.treeBoxObject.ensureRowIsVisible(signonsTree.view.selection.currentIndex);
          break;
      }
      Services.obs.notifyObservers(null, "passwordmgr-dialog-updated");
    }
  }
};

Which seems promising, but I have no idea how that works in practice. Maybe Gijs can clarify.

I also know that we open the password manager from showPasswords() in components/preferences/security.js:

    if (this._loadInContent) {
      gSubDialog.open("chrome://passwordmgr/content/passwordManager.xul");
    } else {
      document.documentElement
              .openWindow("Toolkit:PasswordManager",
                          "chrome://passwordmgr/content/passwordManager.xul",
                          "", null);
    }

Based on this I'm also not sure how we would receive a notification, even if one is sent.

> 
> You can always find the peer of the module from
> https://wiki.mozilla.org/Modules/Core (well, I can't see anything with
> passwords listed there) or a frequent author in that directory and ask them.

Good call, I appreciate the pointer.

> 
> In fact, I'll do that now:
> Gijs, I know very little about how passwords are stored :-( Apparently TB
> caches passwords once retrieved from the password manager, so when a
> password is removed, we don't notice it. Is there any notification available
> for password removals/changes? Or could you give us any other hint or tip?

Comment 19

[gene smith]

I just download the diff and applied the patch with "patch -p1 < rbXXXX.patch". Patch merged in fine and builds.

I had noticed that a change to the password in password manager didn't seem to reliably take effect. This was  while troubleshooting another bug several months ago (probably the yahoo plain auth bug). With this patch, it now does. 

I tested like this starting without the patch in place:

1. Bring up account X with good password. All working fine
2. On server setting for X, change imap port to something wrong.
3. Attempt to access folders on X until all indicate "can't connect" in pop-up, then wait a while longer.
4. Go to password mgr and change the password there to something wrong.
5. Set the account X imap port back to correct value.
6  Click on account X folders and observe with wireshark (or imap.log) that authentication succeeds with the wrong password entered in password mgr.

7. Now shutdown tb, apply the patch and rebuild and run tb

8. Ignore/cancel the prompt to enter correct password for account X
9. Go to password mgr and enter the correct password there for account x
10.Click on account X folder and observe with wireshark (or imap.log) that authentication now succeeds and/or folders are accessible.
11.Repeat steps 2-5 and goto step 12.
12.Should see prompt to enter correct password. Enter the correct password and verify all OK on account X 

So without the patch, it seems that passwords changed (and deleted) in password mgr only take effect on tb restart. With the patch, they take effect without a restart.

As a final test, with patch in place and all connection to account X timed out, I deleted the account X entry from password mgr. Access to folders on account X popped up the password prompt. So deleting the password in password mgr also takes effect immediately.
 
P/S: What is "STR"?

Comment 20

[Jorg K (CEST = GMT+2)]

(In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for responses) from comment #18)
> I will
> need a try build to verify this (lots of xpc-shell test cases fail for me
> locally, but it is not obvious that my patch causes them. I will need to try
> again with a clean tree).
Our tree looks pretty good these days, on Linux and Mac all Xpcshell tests pass, on Windows there are some failing tests due to file locking issues. I can do a try push if you want.

> Finally I don't have POP, SMTP, or NNTP accounts,
> so I will need to spend more work testing those.
Surprising, do you send e-mail at all? That would go out via SMTP ;-)

> I'll try to implement this workaround for those other account types....
Maybe we'll wait a moment to assess the notification idea.

>   // be prepared to reload the display if anything changes
>   Services.obs.addObserver(signonReloadDisplay,
> "passwordmgr-storage-changed");
Looks like you can register an observer for "passwordmgr-storage-changed". If that were the case, you could remove all cached passwords. Clean solution without any test failures :-)

We use plenty of observers, see:
https://dxr.mozilla.org/comm-central/search?q=Services.obs.addObserver&redirect=false
Maybe stick this one into mailGlue.js and see what fires. I guess in the final solution each protocol could register such an observer and clear its password cache. The code would be pretty much the same. We also have C++ examples:
https://dxr.mozilla.org/comm-central/search?q=AddObserver&redirect=false

> Which seems promising, but I have no idea how that works in practice. Maybe
> Gijs can clarify.
Yes please.

(In reply to gene smith from comment #19)
> P/S: What is "STR"?
Steps To Reproduce (a problem). Everyone loves them ;-)

Comment 21

[Josiah Bruner [:jsbruner]]

(In reply to Jorg K (GMT+1) from comment #20)
> (In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for
> responses) from comment #18)
> > I will
> > need a try build to verify this (lots of xpc-shell test cases fail for me
> > locally, but it is not obvious that my patch causes them. I will need to try
> > again with a clean tree).
> Our tree looks pretty good these days, on Linux and Mac all Xpcshell tests
> pass, on Windows there are some failing tests due to file locking issues. I
> can do a try push if you want.

I would appreciate that, just in case the notification method doesn't end up working out. (I should have level 3 access, but I need IT to re-enable it)

> 
> > Finally I don't have POP, SMTP, or NNTP accounts,
> > so I will need to spend more work testing those.
> Surprising, do you send e-mail at all? That would go out via SMTP ;-)

Haha. Wow that's embarrassing. I did know that I swear. ;)

> 
> > I'll try to implement this workaround for those other account types....
> Maybe we'll wait a moment to assess the notification idea.

Sure thing. 

> 
> >   // be prepared to reload the display if anything changes
> >   Services.obs.addObserver(signonReloadDisplay,
> > "passwordmgr-storage-changed");
> Looks like you can register an observer for "passwordmgr-storage-changed".
> If that were the case, you could remove all cached passwords. Clean solution
> without any test failures :-)
> 
> We use plenty of observers, see:
> https://dxr.mozilla.org/comm-central/search?q=Services.obs.
> addObserver&redirect=false
> Maybe stick this one into mailGlue.js and see what fires. I guess in the
> final solution each protocol could register such an observer and clear its
> password cache. The code would be pretty much the same. We also have C++
> examples:
> https://dxr.mozilla.org/comm-central/search?q=AddObserver&redirect=false
> 

Hmm okay neat. This approach does seem nice if possible. I'll try it and see what happens. 

> > Which seems promising, but I have no idea how that works in practice. Maybe
> > Gijs can clarify.
> Yes please.
> 
> (In reply to gene smith from comment #19)
> > P/S: What is "STR"?
> Steps To Reproduce (a problem). Everyone loves them ;-)

Comment 22

[:Gijs (he/him)]

(In reply to Jorg K (GMT+1) from comment #17)
> Gijs, I know very little about how passwords are stored :-( Apparently TB
> caches passwords once retrieved from the password manager, so when a
> password is removed, we don't notice it. Is there any notification available
> for password removals/changes? Or could you give us any other hint or tip?

There's a "passwordmgr-storage-changed" observer topic, see https://dxr.mozilla.org/mozilla-central/rev/e96685584bf7d3c1d7a4c1861716da89fd650c51/toolkit/components/passwordmgr/LoginHelper.jsm#743-762

Comment 23

[Jorg K (CEST = GMT+2)]

Great, let's use that one then. Either experiment with mailGlue.js first to get a feel for the received notifications, or stick it right into the C++ code for one of the protocols. I think the constructor and destructor of the protocols or the incoming server code, for example in nsImapIncomingServer.cpp or nsMsgIncomingServer.cpp, could handle it. Sounds like an interesting bug ;-)

Comment 24

[Josiah Bruner [:jsbruner]]

Comment on attachment 8967977 [details]
Bug 516464 - Remove passwords from cache when password manager contents are changed to prevent stale password attempts.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236670/diff/1-2/

Comment 25

[Josiah Bruner [:jsbruner]]

I implemented the observer in nsMsgIncomingServer.cpp, so in theory this works for all of IMAP, POP, etc. I only tested IMAP, however.

When I perform the STR in comment 12, things work as expected (we prompt for a new password, rather than trying the old one).

JorgK, would you be willing to push my change to try for me? I'd really appreciate it.

Comment 26

[Jorg K (CEST = GMT+2)]

Here you go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=395a31687171258642bd87a66fb955ab085828b9
Note that we currently have three Mozmill failures (image insertion, multipart related, LW themes) which will hopefully clear tonight.

I doubt that you patch will break anything since as far as I know, we don't change passwords in test, but hey, I'm surprised over and over how much test coverage we have (and also how much is lacking).

As for the patch: It looks nice and sweet. Why do yo listen to "passwordmgr-dialog-updated" when the suggestion so far was to use "passwordmgr-storage-changed"? Can you point me to the code/documentation?

Also, would you mind very much not to use Review Board, but simply to attach a patch?

Comment 27

[Josiah Bruner [:jsbruner]]

Thanks Jorg. 

The reason for using a different observer was evidently due to me not paying attention. I copy-pasted the event in the file I was looking at. 

I'll switch to the other one and reupload a patch. 

Does Thunderbird not use Review board btw?

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for responses) from comment #27)
> Does Thunderbird not use Review board btw?
Mostly not, I also know an M-C person whose BMO nick states "please no mozreview requests".

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8967977 [details]
Bug 516464 - Remove passwords from cache when password manager contents are changed to prevent stale password attempts.

Wow, heaps of test failures. You know how to run them locally, right? Looks like it's crashing in the DTOR. Hmm, what's wrong there?

Comment 30

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

Okay, this patch switches to the documented "passwordmgr-storage-changed" observer, rather than the internal "passwordmgr-dialog-updated" one.

It also will (hopefully) fix the crashes. After looking at the logs, it seems the issue was that I was trying to call a function using a null nsCOMPtr. This happens when mozilla::services::GetObserverService(); returns NULL.

To resolve this crash, I now null-check the resulting pointer. Keep in mind I did not investigate why GetObserverService() returns NULL sometimes. However, I don't see a reason to abort or throw an exception, so instead I let things continue normally and assume a password manager will not be used (i.e. don't register an observer at all in that case)

Jorg, can you push to try again for me?

Comment 31

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bfba30a0b4198e4e45a217517a265affc8a173c1

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for responses) from comment #30)
> I did not investigate why GetObserverService() returns NULL sometimes.
Well, that would be good to know.

Comment 33

[Jorg K (CEST = GMT+2)]

Looks like you still have some crashes in Xpcshell debug.

Comment 34

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

Ah, so it turns out placing an observer in the constructor is bad practice as it doesn't play well with the garbage collector. See https://bugzilla.mozilla.org/show_bug.cgi?id=325392 for another example of this being an issue and causing similar crashes.

I now use a dedicated Init() function which is added to the factory.

Before this change, I could reproduce the xpshell crashes (namely mailnews/base/test/unit/test_accountMgr.js) caused by my patch. After the change, the tests pass successfully.

The STR from comment 12 still yield the "proper" results on a local Debug build. Jorg, can I please have one more try-push? Hopefully things work this time. I'll request review after I know things pass try.

Comment 35

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fa2ad94e7536651e2850bacee43f2f5d7f34b3b5
We had a pretty green tree before the last M-C merge 20 minutes ago, so let's hope this isn't busted.

Comment 36

[Jorg K (CEST = GMT+2)]

PROCESS-CRASH | comm/mailnews/imap/test/unit/test_imapProtocols.js | application crashed [@ mozalloc_abort(char const*)]

Comment 37

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

nsMsgIncomingServer inherits from nsSupportsWeakReference, so I needed to tell that to nsIObserverService. (See: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService#addObserver() )

This newest patch appears to fix the last crash.

JorgK, could I bother you for (yet) another try run?

Comment 38

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d73a18c3f9926589fc12b9320b1aac8c4b784e93

Thanks for the persistence!

Comment 39

[Josiah Bruner [:jsbruner]]

Things look much better. 

There is one mozmill crash, but on the surface it looks unrelated to my change (the crash is in nsNSSComponent::IsCertContentSigningRoot(CERTCertificateStr*,)

In addition, I was not able to reproduce any failures in content-tabs locally. I also retriggered the mozmill tests many times, and they have all passed since. 

I'm going to request review on my patch now, but I'm interested in what Jorg things of that one failure.

Comment 40

[Josiah Bruner [:jsbruner]]

(In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for responses) from comment #39)
> I'm going to request review on my patch now, but I'm interested in what Jorg
> things of that one failure.

thinks, rather.

Comment 41

[Jorg K (CEST = GMT+2)]

That failure is pre-existing, for example here:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=fa20f27405ecf6530dcdab09c4cb01c2b0c1e9a5&selectedJob=176433003

Comment 42

[Jorg K (CEST = GMT+2)]

I have a few questions related to the last version.

I can see that the pattern NS_GENERIC_FACTORY_CONSTRUCTOR_INIT() together with an Init() method is used elsewhere and some register an observer for "xpcom-shutdown". I think it would be best to use NS_XPCOM_SHUTDOWN_OBSERVER_ID. In fact, nsMsgSendLater::Observe() is pretty much what you're doing here.

Looking at similar code, do you still think |if (observerService)| is right, or should that be |NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);| now?

Can you enlighten me on how it all works. Who calls that Init() function and is it called for every instance of nsMsgIncomingServer? I believe there can be more than one server object. What happens if the object is destroyed before XPCOM shutdown?

Comment 43

[Josiah Bruner [:jsbruner]]

(In reply to Jorg K (GMT+1) from comment #42)
> I have a few questions related to the last version.
> 
> I can see that the pattern NS_GENERIC_FACTORY_CONSTRUCTOR_INIT() together
> with an Init() method is used elsewhere and some register an observer for
> "xpcom-shutdown". I think it would be best to use
> NS_XPCOM_SHUTDOWN_OBSERVER_ID. In fact, nsMsgSendLater::Observe() is pretty
> much what you're doing here.

I agree with you. I see that NS_XPCOM_SHUTDOWN_OBSERVER_ID is defined to be "xpcom-shutdown" (https://dxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPCOM.h?q=NS_XPCOM_SHUTDOWN_OBSERVER_ID&redirect_type=direct#366)

> 
> Looking at similar code, do you still think |if (observerService)| is right,
> or should that be |NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);|
> now?

I think you're right; it should be changed now. When it was in the ctr/dtr, NS_ENSURE_* couldn't be used since it returns an nsresult, but in Init and Observe it will likely work. I'll try it and see if there are any obvious issues.

> 
> Can you enlighten me on how it all works. 

Heh, I'll try my best, but I will admit it isn't crystal clear to me either. I was also unable to find "nice" documentation explaining how to *use* observers *properly*.

> Who calls that Init() function and is it called for every instance of nsMsgIncomingServer? I believe there can be more than one server object.

It seems to be automatically called when it in constructed by defining https://dxr.mozilla.org/comm-central/source/mailnews/build/nsMailModule.cpp#605

For brief info on how that works, search for "NS_GENERIC_FACTORY_CONSTRUCTOR" here https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Creating_components/Using_XPCOM_Utilities_to_Make_Things_Easier

Also, it must be called for every instance because ::Init passes an instance of `this` to the observer service, who then later calls ::Observe using that pointer. This is what allows us to forget the password, which is specific to each server instance.

(I did just realize an issue with my patch though. I updated the factory constructor for IMAP-only evidently. I need to do the same for SMTP.)

>  What happens if the object is destroyed before XPCOM shutdown?

I wondered the same thing and never settled on a final answer. From experimentation, I think the answer is "nothing bad".

While TB was running I removed an entire account, and things seemed okay (i.e no crash). I guess in the worst case it would cause an observer to not be unsubscribed. Resolving this, though, it not obvious (I attempted it previously).

If in ::~nsMsgIncomingServer you try doing something like:

    if !m_hasAlreadySeenXPCOM_Shutdown
        // Remove Observers...

I found that many tests would cause it to crash because we "try to access an XPCOM object after shutdown". This is almost certainly due to a concurrency issue. My guess is something like:

1. dtr is called for arbitrary reason (before we get the xpcom-shutdown message)
2. It evaluates !m_hasAlreadySeenXPCOM_Shutdown 
3. We then get interrupted by the "xpcom-shutdown" message (this assumes observers ::Observe in a separate thread. I have no idea if this is true).
4. We then immediately switch context back to the dtr, who tries to get the observer, but at this point xpcom is gone and mozilla::services::GetObserverService() throws.

This is very hard to be sure about without thoroughly understanding how observers work. Maybe someone else knows more?

In the meantime, I might be able to resolve this with a mutex. I can try and see what happens.

On the other hand, nsMsgSendLater doesn't try to handle this either. What do you think?

Comment 44

[Josiah Bruner [:jsbruner]]

(In reply to Josiah Bruner [:jsbruner] (prev. :JosiahOne) (needinfo for responses) from comment #43)
> I did just realize an issue with my patch though. I updated the factory
> constructor for IMAP-only evidently. I need to do the same for SMTP.)

Err, POP, rather.

Comment 45

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

This fixes everything except the potential "dtr called before xpcom-shutdown which keeps observers alive" issue, since I'm not sure if that's actually an issue.

Note that although POP3 should now also work properly, I did not test it. Does anyone use POP who could test?

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973451 [details] [diff] [review]
bug516464fix.patch

Review of attachment 8973451 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks very good now. I can fix the blank line issue before checking it in.

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +74,5 @@
> +  // We need to know when the password manager changes.
> +  nsCOMPtr<nsIObserverService> observerService =
> +           mozilla::services::GetObserverService();
> +
> +  NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);

Nit: The blank line should be after the NS_ENSURE_TRUE(), not before.

@@ +107,5 @@
> +    // Now remove ourselves from the observer service as well.
> +    nsCOMPtr<nsIObserverService> observerService =
> +      mozilla::services::GetObserverService();
> +
> +    NS_ENSURE_TRUE(observerService, NS_ERROR_UNEXPECTED);

Same here.

Comment 47

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973451 [details] [diff] [review]
bug516464fix.patch

Forgot to check the box.

Comment 48

[Josiah Bruner [:jsbruner]]

Attachment: Final Patch

Thanks Jorg. Here's a version with the whitespace fix just so that it doesn't get forgotten.

Carrying review flag....

Comment 49

[Jorg K (CEST = GMT+2)]

I should have studied this comment a bit closer before :-(

(In reply to Josiah Bruner [:jsbruner] (needinfo for responses) from comment #43)
> (I did just realize an issue with my patch though. I updated the factory
> constructor for IMAP-only evidently. I need to do the same for SMTP.)
Hmm, you added it for POP3. However, there are also other incoming servers, I can see RSS, NNTP and "no". Also, the question is, SMTP isn't an incoming server, however, it also has a password that the user can modify. I think all this is missing from your patch. You can actually test SMTP by sending an e-mail.

> On the other hand, nsMsgSendLater doesn't try to handle this either. What do
> you think?
I think we should we should go with the mechanism we have now.

Comment 50

[Jorg K (CEST = GMT+2)]

Comment on attachment 8973470 [details] [diff] [review]
Final Patch

I think we need to look at the other servers we support as well.

Comment 51

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

This patch adds support for all account types that inherit from nsMsgIncomingServer (so IMAP, POP3, NNTP, RSS, jsAccount, Movemail, etc.)

Before I do the SMTP change, I would like another try server build just to make sure things are okay up to this point. Jorg, could you start another try build for me?

Comment 52

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=09649c89ee74023cb89147f0a3da5d6b280b2bc2

Thanks for the persistence.

Comment 53

[Josiah Bruner [:jsbruner]]

Perfect, build looks great. I'll do SMTP later this week and hopefully we'll then be good to go.

Comment 54

[Josiah Bruner [:jsbruner]]

So I actually want to fix the SMTP variant of this in another bug since they are two distinct portions of the code (the SMTP stuff being in compose/ rather than base/). That also will help narrow down things in case of unforeseen regressions.

I will create another bug later/tomorrow for SMTP and then request review on this patch when Jorg gets back from PTO. needinfo'ing myself as a reminder.

Comment 55

[Jorg K (CEST = GMT+2)]

I've come to try this out as a final "review". For a POP account I edited the stored password to something invalid. I expected TB to throw an error on retrieving e-mail, but it didn't. So it must have used the old correct password. I restarted and then got the expected error. I corrected the password, but still got the error.

Next I tried IMAP and the result was the same.

Next I added:
  if (strcmp(aTopic, "passwordmgr-storage-changed") == 0)
  {
    printf("=== nsMsgIncomingServer::Observe - ForgetSessionPassword\n");
    rv = ForgetSessionPassword();
    NS_ENSURE_SUCCESS(rv,rv);
  }
The print never showed up.

What am I missing? The patch is applied and I did do a build :-(

Comment 56

[Josiah Bruner [:jsbruner]]

(In reply to Jorg K (GMT+2) (contract suspended, PM if urgent) from comment #55)
> I've come to try this out as a final "review". For a POP account I edited
> the stored password to something invalid. I expected TB to throw an error on
> retrieving e-mail, but it didn't. So it must have used the old correct
> password. I restarted and then got the expected error. I corrected the
> password, but still got the error.
> 
> Next I tried IMAP and the result was the same.
> 
> Next I added:
>   if (strcmp(aTopic, "passwordmgr-storage-changed") == 0)
>   {
>     printf("=== nsMsgIncomingServer::Observe - ForgetSessionPassword\n");
>     rv = ForgetSessionPassword();
>     NS_ENSURE_SUCCESS(rv,rv);
>   }
> The print never showed up.
> 
> What am I missing? The patch is applied and I did do a build :-(

Hmm. Are you trying on trunk? What platform? I may need to recheck on the latest trunk. 

Can you also try changing the last parameter of the addObserver call to use false instead of true. In the SMTP bug I discovered I needed it to be false for some unknown reason.

Comment 57

[Jorg K (CEST = GMT+2)]

Latest trunk, compiled this morning on Windows. It works with the said parameter set to true. I guess it needs another try run then since we ran a no-op.

Comment 58

[Josiah Bruner [:jsbruner]]

Interesting. Yeah I'll create a new patch and ask for a try push. 

I also need to research that function more, because I don't understand how it works enough for me to be satisfied.

Comment 59

[Josiah Bruner [:jsbruner]]

Attachment: bug516464fix.patch

Okay, this switches the params to false. At first, this still caused crashes, but then I realize this was because although I declared the nsIObserver functions, I never implemented it via adding it to NS_IMPL_ISUPPORTS().

This patch makes that change as well and it resolved my local crashes (actually assertions).

I'm going to need another try push though.

Comment 60

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=32aa74e3dd8c14403c703870a0e10bf3dd2c4a2d

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 8975680 [details] [diff] [review]
bug516464fix.patch

These failures are expected right now, so this is good to go, right? Why do you include include "nsThreadUtils.h"?

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f2405b6ea062
Remove passwords from cache when password manager contents are changed to prevent stale password attempts. r=jorgk

Comment 63

[Jorg K (CEST = GMT+2)]

I removed the unnecessary include before landing.

Comment 64

[Josiah Bruner [:jsbruner]]

Thanks Jorg. The extra include was something left from debugging (checking whether we were the main thread. Spoiler: The answer is yes)

Comment 65

[Jorg K (CEST = GMT+2)]

TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/7e1066ec6885c3f403ed58bbc58a37e1b40ce984

Comment 66

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/6badc1f1ac53