When someone changes the hostname and/or username on an account, we keep the original values and use those to internally reference the account, but we communicate on the new values. As I'm currently doing password manager work (and was reminded of this) I thought it best to write a couple of tests so that a) we check the general idea works, b) we check that we don't regress getting already stored passwords in these situations. Here are the tests for POP3 and News. The test_*Password2.js files are basically copies of their test_*Password.js files with a couple of changes for the slightly different hostname/username set ups. There is also a new password file as well. Not doing IMAP at the moment as we don't have any password tests for that, and with most of the code being common, will be covered by the POP3/News tests.

Comment on attachment 354327 [details] [diff] [review] The fix This doesn't quite work in the toolkit password manager case. It looks as if toolkit is removing the login when the hostname is changed, when wallet was not. I want to understand this a bit better, so cancelling review for now.

This is better. The problem was that when we change username or hostname in account manager, mailnews forgets the password stored. As I was using the same technique in the tests, toolkit password manager was removing the login that I was trying to test against. I think wallet version didn't remove it probably because the categories (i.e. to hook onto the login-failed notification) aren't being registered correctly in unit tests. This version of the tests sets up the accounts via preferences and then loads them in and performs the tests. This works for both versions of the password manager code.

Comment on attachment 354682 [details] [diff] [review] The fix v2 I did a make check in mailnews and it hung after this: TEST-PASS | ../../../mozilla/_tests/xpcshell-simple/test_news/unit/test_filter.js | all tests passed TEST-PASS | ../../../mozilla/_tests/xpcshell-simple/test_news/unit/test_nntpPassword.js | all tests passed test_nntpPassword2.js.log is as follows: *** test pending Directory request for: MFCaF that we (mailDirService.js) are not handling, leaving it to another han dler. Directory request for: NewsD that we (mailDirService.js) are not handling, leaving it to another han dler. WARNING: NS_ENSURE_TRUE(msgWindow) failed: file /Users/davidbienvenu/tbirdhg/mailnews/base/util/nsMs gProtocol.cpp, line 465 WARNING: NS_ENSURE_TRUE(msgPrompt) failed: file /Users/davidbienvenu/tbirdhg/mailnews/base/util/nsMs gProtocol.cpp, line 410 It's possible but not likely that I have some other changes in my tree affecting this. some nits: "realusername and realhostname are different to username" should be "different from" + + // Copy the file to the profile directory for a PAB + signons.copyTo(gProfileDir, "signons.txt"); probably just lose the "for a PAB" part. + // Subscribe to certain posts I know this just moved, but I think of a post as an individual message, so I would just say "newsgroups" here. + // Note, the uri is for hostname invalid which is the original uri. See This is easier to parse if you quote invalid, i.e, hostname "invalid". + var transaction; maybe move the decl to where transaction is used a little bit later?

Revised patch for comments. I've tried this on a mac build with no patches, and a clobbered linux build and not had any problems.

I'll try this on my windows build...

Comment on attachment 354727 [details] [diff] [review] The fix v3 this patch has a bunch of extra stuff in it...

Comment on attachment 354682 [details] [diff] [review] The fix v2 ok, this patch works for me on my Windows build, so r=me, modulo the comment nits I mentioned, which you've already addressed. I don't know what happened with my mac build;

Comment on attachment 354727 [details] [diff] [review] The fix v3 clearing request, as it doesn't seem like the right patch for this bug.

This is the one I meant to attach, but it just fixes the nits anyway, so carrying forward r+.

Checked in: http://hg.mozilla.org/comm-central/rev/da0ceaa26390