My last 6 news accounts changed their names by inserting multiple times the string 'hafi'. All of these accounts were closed. hafi@i5_64 ~/.mozilla/seamonkey/yjmshkey.default $ grep 'name", "hafi' prefs.js user_pref("mail.server.server12.name", "hafinewshafi1.open-news-hafinetwork.orghafi"); user_pref("mail.server.server13.name", "hafireadhafi.news.telefohafinica.dehafi"); user_pref("mail.server.server14.name", "hafinewshafi.alice-dsl.dhafiehafi"); user_pref("mail.server.server5.name", "hafifreehafinews.netfronhafit.nethafi"); user_pref("mail.server.server8.name", "hafinewshafi.tota-refugihafium.dehafi"); user_pref("mail.server.server9.name", "hafinewshafi.online.dehafi"); Only mail.server.server??.name is changed, all other mail.server.server??.* remain unchanged. The names of prior news servers are not affected. BTW, hafi is my user name. I seem to remember that a similar thing has occured in the past, several years ago when i used i686. See also the comment of Jens: https://bugzilla.mozilla.org/show_bug.cgi?id=695309#c84 What have i done before the names changed? Some experiments trying to find a solution for another bug after making a backup. In this backup the names are still unchanged. No protocol of the experiments, unfortunately.

(In reply to Hartmut Figge from bug 695309 comment #83) > I have just seen something related. The names of the last 6 of my news > accounts are changed by inserting multiple times the string 'hafi'. > > (...) If you rewrite the strings, the issue becomes clearer: hafi news hafi 1.open-news- hafi network.org hafi hafi read hafi .news.telefo hafi nica.de hafi hafi news hafi .alice-dsl.d hafi e hafi hafi free hafi news.netfron hafi t.net hafi hafi news hafi .tota-refugi hafi um.dehafi hafi news hafi .online.de hafi So in between "hafi"s there are parts of certain lengths (4, up to 12, rest) of the old account name, and "hafi" is both at the beginning and end of all strings. As I already noted on the newsgroups (somewhere...), I've seen this bug years ago but was never able to reproduce it. Thanks to your list of broken strings (heh) I was able to track it down this time: I'm 99% sure nsMsgIncomingServer::OnUserOrHostNameChanged is the culprit. As the name implies, a username or password change triggers a renaming of the account name (prettyName internally): "replace all occurrences of old name in the acct name with the new one". It seems the code is not always doing the right thing (maybe if oldName or newName are empty?).

I guess the bug occurs if the old username was empty and the new one is not (e.g. "hafi"). Looking at the code, I guess the Find method finds matches for an empty string at position 0, <length of new username>, <length of username * 3 (or length of username - 1 ?)> and so on, plus the end of the string. Someone who knows C++ better than me should probably confirm what the DefaultComparator (memcmp?) does. The fix for this bug might very well be to just not do anything in the fourth step if oldName is empty. http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgIncomingServer.cpp#1230

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #2) > I guess the bug occurs if the old username was empty and the new one is not > (e.g. "hafi"). Looking at the code, I guess the Find method finds matches > for an empty string at position 0, <length of new username>, <length of > username * 3 (or length of username - 1 ?)> and so on, plus the end of the > string. Someone who knows C++ better than me should probably confirm what > the DefaultComparator (memcmp?) does. The fix for this bug might very well > be to just not do anything in the fourth step if oldName is empty. > > http://mxr.mozilla.org/comm-central/source/mailnews/base/util/ > nsMsgIncomingServer.cpp#1230 This leads to a workaround, or rather, a protective measure: if you make sure your newsgroup servers have a nonempty (and nondefault) display name (e.g. if you give news.mozilla.org a display name of "Mozilla News"; similarly for other servers), you won't get the bug in the future.

Oh, is it the username ("hafi") or the servername (defaulting to "news1.open-news-network.org" if no explicit display name was given for the server)? Maybe I misunderstood.

(In reply to Tony Mechelynck [:tonymec] from comment #4) > Oh, is it the username ("hafi") or the servername (defaulting to > "news1.open-news-network.org" if no explicit display name was given for the > server)? Username, as I wrote. > Maybe I misunderstood. Yes.

To clarify, hafi is not the user name of any of my news servers. It is for the POP3 server of my mail account, localhost, and it is also my login name on my machine. hafi@i5_64 ~ $ whoami hafi

I've also had this happen when changing the User Name field under server settings in Account Manager. If your account name is fred.bloggs@acme.com with a user name of fred, and you change the user name to george, then the account name changes to george.bloggs@acme.com! cc'ing :aceman as they have been digging into the depths of the account manager code recently.

(In reply to Ian Neal from comment #7) > If your account name is fred.bloggs@acme.com with a user name of fred, and > you change the user name to george, then the account name changes to > george.bloggs@acme.com! Yes, looking at the code suggests this is what it is supposed to do. First there's the question how sensible this approach really is (i.e. whether it should be there at all or maybe be restricted to cases like <username>@<hostname>) and secondly there's the issue that this bug is really about (which I think occurs when the old username is empty for some reason). We /could/ fix both issues here, but what I'm mainly concerned with is the latter part.

Is this only in news account? Also, is the component correct?

(In reply to Hartmut Figge from comment #6) > To clarify, hafi is not the user name of any of my news servers. It is for > the POP3 server of my mail account Oh? Interesting. If that is true, there might be more broken here (e.g. a proper connection between the account that triggered a username change and the account(s) that get changed as a consequence). FWIW, AFAICS OnUserOrHostNameChanged only gets called from nsMsgIncomingServer::SetRealHostname and nsMsgIncomingServer::SetRealUsername. > and it is also my login name on my machine. I'm pretty sure this is unrelated. (In reply to :aceman from comment #9) > Is this only in news account? We don't have proper STR yet (Ian's comment comes closest), only symptoms and some indicators that OnUserOrHostNameChanged caused the issues. > Also, is the component correct? Not totally sure, maybe Backend instead. I was looking at the other "Networking: xy" components and decided to go with the main one since the problematic function is in nsMsgIncomingServer.

(In reply to Ian Neal from comment #7) > I've also had this happen when changing the User Name field under server > settings in Account Manager. > If your account name is fred.bloggs@acme.com with a user name of fred, and > you change the user name to george, then the account name changes to > george.bloggs@acme.com! Is this actually wrong? :) What would you expect it to become? (In reply to Jens Hatlak (:InvisibleSmiley) from comment #10) > Not totally sure, maybe Backend instead. I was looking at the other > "Networking: xy" components and decided to go with the main one since the > problematic function is in nsMsgIncomingServer. I would have thought Account manager would be good. I reproduced it on a POP3 account with STR from Ian. I actually can't find where you put username for news account.

(In reply to :aceman from comment #11) > I reproduced it on a POP3 account with STR from Ian. I actually can't find > where you put username for news account. News accounts do not usernames; instead, the username is saved in the password manager.

(In reply to :aceman from comment #11) > (In reply to Ian Neal from comment #7) > > I've also had this happen when changing the User Name field under server > > settings in Account Manager. > > If your account name is fred.bloggs@acme.com with a user name of fred, and > > you change the user name to george, then the account name changes to > > george.bloggs@acme.com! > > Is this actually wrong? :) What would you expect it to become? I don't think it should be done automatically. Another example:# Say the account name is Candian and your username is ian, then you change the username to neal. The account name becomes Candneal! If nothing else the algorithm is needs some work. > > (In reply to Jens Hatlak (:InvisibleSmiley) from comment #10) > > Not totally sure, maybe Backend instead. I was looking at the other > > "Networking: xy" components and decided to go with the main one since the > > problematic function is in nsMsgIncomingServer. > > I would have thought Account manager would be good. > Moved to Account Manager component.

Would be better if I could spell Canadian, sorry.

(In reply to Joshua Cranmer [:jcranmer] from comment #12) > (In reply to :aceman from comment #11) > > I reproduced it on a POP3 account with STR from Ian. I actually can't find > > where you put username for news account. > > News accounts do not usernames; instead, the username is saved in the > password manager. In Sea Monkey at least, News accounts use the server name as the account name, e.g. news.mozilla.com

(In reply to Ian Neal from comment #13) > I don't think it should be done automatically. Another example:# > Say the account name is Candian and your username is ian, then you change > the username to neal. The account name becomes Candneal! If nothing else the > algorithm is needs some work. This one is ugly, now I understand the problem :)

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10) > (In reply to Hartmut Figge from comment #6) > > To clarify, hafi is not the user name of any of my news servers. It is for > > the POP3 server of my mail account > > Oh? Interesting. If that is true,... I have verified this with the Password Manager. Now i have entered hafi in about:config and detected realuserName. What ever that may be. :) hafi@i5_64 ~/.mozilla/seamonkey/yjmshkey.default $ grep realuserName prefs.js user_pref("mail.server.server10.realuserName", "hafi"); user_pref("mail.server.server11.realuserName", "hafi"); user_pref("mail.server.server12.realuserName", "hafi"); user_pref("mail.server.server13.realuserName", "hafi"); user_pref("mail.server.server14.realuserName", "hafi"); user_pref("mail.server.server17.realuserName", "hafi"); user_pref("mail.server.server2.realuserName", "hafi"); user_pref("mail.server.server3.realuserName", "hafi"); user_pref("mail.server.server4.realuserName", "hafi"); user_pref("mail.server.server5.realuserName", "hafi"); user_pref("mail.server.server6.realuserName", "hafi"); user_pref("mail.server.server7.realuserName", "hafi"); user_pref("mail.server.server8.realuserName", "hafi"); user_pref("mail.server.server9.realuserName", "hafi");

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #10) > (In reply to Hartmut Figge from comment #6) [hafi] > > and it is also my login name on my machine. > > I'm pretty sure this is unrelated. It could be this realuserName. :-P

This would probably need some UI spec what should actually happen. What account name mangling is actually wanted.

(In reply to :aceman from comment #19) > This would probably need some UI spec what should actually happen. What > account name mangling is actually wanted. Provided that we want to keep the mangling *at all*, that is. If we do not remove this "feature" completely, then we should at least make sure the main issue of this bug (the faulty mangling in case of what we think is due to an empty old user name) gets fixed.

As I said in bug 281308, actually I think this should be solved in such a way that only a "<user>@<server>" account name is updated by a user name change, and for safety check that both the old and new user name are non-empty. aceman: The bug is not in JS code but C++, see comments 1, 2, and 10. The good news is that the code is quite simple/short, does not have many users/callers, and both the old and new name is available inside. The bad news is that it's currently one method for two things (username and hostname) and you currently cannot tell which one changed. IOW, this needs some (simple) refactoring. Unfortunately I'm very bad at C++, but it should be easy for someone with some more skill.

Yeah, and Mozilla's C++ is over the top, so many abstraction levels :) I can try this. Are you able to test a patch?

(In reply to :aceman from comment #22) > Are you able to test a patch? I can compile and run on Linux and Windows, and I can try to test, but I'll have to see whether the comments suffice for reliable steps to reproduce; didn't check yet. Anyway, if you can up with a patch (WIP even), by all means go ahead! :-)

So this is what you wished for. However it has these problems: - doesn't work for account types that do not have username (like news) - doesn't work if you change 'user1' on server 'yyy' on account 'user1@xxx' to 'user2' on server stays at 'yyy'. Nothing is changed then. Yes, you wished it that way, but I do not like it. I'd say if the username part of account name matches old username as a whole, then change that part to new name. The same for hostname part. If username is empty then consider whole account name as hostname. What about that?

So this is what I would like it to do, as I described in comment 24.

OK, I had the chance to quickly test both patches (had to make distclean mailnews once once to get the stupid buildenv to recognize the new third param). I cannot go into actual details right now because it's late here. More to come tomorrow. Feel free to provide improved patches in the mean time! :-) First, let me thank you very much for working on this! Second, STR! :-) a) POP3 (user name or host name changes) 1. create a POP account, using defaults mostly (I used SeaMonkey's wizard) so the account name will be <outgoing user name>@<server name> 2. open Account Settings and go to the new POP3 server's Server Settings pane 3. change either Server Name or User Name b) NNTP (host name changes only) 1. create news account similar to above so the account name will be <server name> 2. again as 2./3. above, but you can only change Server Name Actual results: The account name will get messed up: any occurrences of what you changed (outgoing user name and/or server name) will get replaced anywhere in the account name, but in an ill fashion (strings are cut off, cf. initial comment). Expected results: Debatable. :-) In any case, no cut-offs must happen, and the result should be predictable and logical. If the user chose a custom account name, it should not be messed with. So, now on to the two patches! Option a ("what I suggested") didn't really work for me. But that's not too bad, because I think option b ("what you suggested") is better (unless I got your intentions wrong, which we'll see). Basically my main intention is what I wrote above under "Expected results", and I think both approaches could fulfill that. Let's compare: a) is supposed to only ever touch cases where A@B is changed to C@D (where A/C are old/new outgoing user name and B/D are old/new server name) b) is supposed to change A@B to C@D if A was changed to C, or B to D. Also, X should be changed to Y if X was the old account=server name and Y is the new server name (news account case). Common part: If the format is neither A@B nor X, no change is made, and no pseudo-random in-string cut-off replacements are made. I agree that b) is the better option. Unfortunately, the news server case didn't work for me (the account was not renamed from my.old.server.name to my.new.server.name). If you fix this and I don't find any regressions, I'll happily give you f+. :-) In any case, what you provided is already a huge improvement! Keep it up!

Thanks for the analysis! I have specifically tested the news case where account name=server name and it worked for me. I'll check again what can be missing there. (I didn't need to distclean, just make sure the full build is running as it is not enough to rebuild nsIncomingServer.cpp. The parameter change is also in the .idl file so that must be recompiled too (and who knows what else)). For potential future reviewers: I added the third parameter for safety. Theoretically it would be possible to guess from oldName and newName whether username (oldname=username) or hostname (oldname=hostname) is changing. But that assumes username!=hostname. And as this bug (and bug 281308) is actually about strange cases and dummy accounts, those could have username and hostname the same or empty or something weird. So better play it safe and do not guess.

So I don't know, it works for me on News. What account/hostname strings have you tried?

Comment on attachment 600140 [details] [diff] [review] implementation of comment 21 I think we agree we don't want this, so let's make this formal.

Comment on attachment 600192 [details] [diff] [review] implementation of comment 24 (In reply to :aceman from comment #28) > So I don't know, it works for me on News. Never mind, maybe I had an incomplete build yesterday or didn't pay enough attention at the time (as I said, it was late). It works for me now. Maybe others can test, too (hafi, tonymec) now that we have both a good patch and some reliable STR. I don't feel well today (headache) so I'll skip the deeper analysis (which should be done by a real reviewer anyway) and just give you f+ right away. Thanks again! [I guess the TB devs will ask you to create a test for this. If they do, I won't be much help regarding the coding but maybe for constructing test cases (both ones that should have an effect and ones that should not alter anything). Let me know if I can help there.] [You might want to be clearer in the code comments, e.g. "was equal to the part before/after the @ in the account name" so anyone reading it knows we're not messing with substring matches anymore.]

Comment on attachment 600192 [details] [diff] [review] implementation of comment 24 Thanks for the test. I'll fix the comments. I got the impression the devs would gladly drop this mangling altogether so hopefully they would not insist on a test for this. /Wishful thinking/ :) I would not be able to create a test. I am stuck on that in bug 580349 too :( So let's see.

aceman, maybe you should request code review. First, because I'm not sure this needs ui-r at all, and because code review is probably more important. Third, this can easily be parallelized. :-)

Comment on attachment 600192 [details] [diff] [review] implementation of comment 24 Sorry I bit rotted you in bug 491843

fix bitrot

Comment on attachment 602601 [details] [diff] [review] implementation of comment 24, v2 >+ // get previous username >+ nsCString userName; >+ if (hostnameChanged) > { >+ rv = GetRealUsername(userName); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } else { >+ userName.Assign(oldName); > } > >+ // get previous hostname >+ nsCString hostName; >+ if (hostnameChanged) >+ { >+ hostName.Assign(oldName); >+ } else { >+ rv = GetRealHostName(hostName); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } Can't these two if statements be combined? >+ >+ // switch corresponding part of the account name to the new name... >+ nsString acctPart; >+ if (!hostnameChanged && (atPos != kNotFound)) >+ { // ...if username changed and the previous username was equal to the part of the account name before @ Nit: this comment is longer than 80 characters and should be split over two lines. Usually { is on its own line with the comment on the following line. >+ acctName.Left(acctPart, atPos); >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(userName))) >+ acctName.Replace(0, userName.Length(), NS_ConvertASCIItoUTF16(newName)); >+ } >+ if (hostnameChanged) >+ { // ...if hostname changed and the previous hostname was equal to the part of the account name after @ >+ // or as whole account name Nit: the first line of this comment is longer than 80 characters and part of it should be moved onto the second lines. Usually { is on its own line with the comment on the following line. >+ if (atPos == kNotFound) >+ atPos = -1; Doesn't atPos already equal -1 anyway? Is it worth just increasing atPos by 1 to make the statements below simpler? (+= 1) >+ acctName.Right(acctPart, acctName.Length() - (atPos + 1)); >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(hostName))) >+ acctName.Replace(atPos + 1, acctName.Length() - (atPos + 1), NS_ConvertASCIItoUTF16(newName)); Nit: line is longer than 80 characters so 3rd argument should be on the line below. >+ } r=me with those addressed/answered

(In reply to Ian Neal from comment #35) > >+ if (atPos == kNotFound) > >+ atPos = -1; > Doesn't atPos already equal -1 anyway? Is it worth just increasing atPos by > 1 to make the statements below simpler? (+= 1) Yes, kNotFound is -1 NOW. But conceptually it is just an arbitrary special constant (haven't looked at the definition but I handle it this way). I think it could change any time so we can't rely on it in numerical computations. For my computation the -1 value of atPos is handy, so I just made sure it is always -1. But as you found out, it is even handier to bump it by 1 so it will be disconnected from kNotFound anyway.

Comment on attachment 602711 [details] [diff] [review] implementation of comment 24, v3 r=me

Standard8: Ping for review

I forgot to change the UUID :(

Comment on attachment 608951 [details] [diff] [review] implementation of comment 24, v3 thx for the patch, sorry for the delay. xpcshell tests for this would be useful, and not too hard, I wouldn't think. A new test would go in mailnews/base/test/unit, I think. Some of the lines where bool hostnameChanged got added should now be wrapped. I'll try the patch and see how it works.

Comment on attachment 608951 [details] [diff] [review] implementation of comment 24, v3 r=me, modulo previous comments.You'll need sr as well since you're changing an interface. When I renamed a news server, I essentially hung in the deadlock detector, but that happened w/o your patch as well, and I verified that the account name was changed correctly in the debugger.

:InvisibleSmiley, can you verify the patch in today's nightlies?

Comment on attachment 622465 [details] [diff] [review] implementation of comment 24, v4 Notes on string usage for future reference: >+ acctName.Left(acctPart, atPos); >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(userName))) if (StringHead(acctPart, atPos).Equals(...)) >+ if (atPos == kNotFound) >+ atPos = 0; >+ else >+ atPos += 1; [I wonder whether the compiler can optimise this, given that kNotFound == -1] >+ acctName.Right(acctPart, acctName.Length() - atPos); >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(hostName))) { if (Substring(acctName, atPos).Equals(...))

(In reply to neil@parkwaycc.co.uk from comment #47) > Notes on string usage for future reference: > >+ acctName.Left(acctPart, atPos); > >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(userName))) > if (StringHead(acctPart, atPos).Equals(...)) > >+ acctName.Right(acctPart, acctName.Length() - atPos); > >+ if (acctPart.Equals(NS_ConvertASCIItoUTF16(hostName))) { > if (Substring(acctName, atPos).Equals(...)) Where are these documented? > >+ if (atPos == kNotFound) > >+ atPos = 0; > >+ else > >+ atPos += 1; > [I wonder whether the compiler can optimise this, given that kNotFound == -1] Maybe, but the source is intentionally done this way without assuming what value kNotFound has, see comment 36.

(In reply to :aceman from comment #46) > :InvisibleSmiley, can you verify the patch in today's nightlies? All is well. :)

Test in bug 810763.