Last Comment Bug 485839 - Refactoring need into nsMsgAccountManager::getUniqueAccountKey to avoid that newly created account takes id of a deleted one (and mails of old account show the new account name)
: Refactoring need into nsMsgAccountManager::getUniqueAccountKey to avoid that ...
Status: RESOLVED FIXED
[datalossy][see comment #9]
:
Product: MailNews Core
Classification: Components
Component: Account Manager (show other bugs)
: Trunk
: All All
: P1 major (vote)
: Thunderbird 16.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-29 16:39 PDT by Andreas Wagner [:TheOne]
Modified: 2012-07-02 15:12 PDT (History)
9 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (3.29 KB, patch)
2012-05-24 14:20 PDT, :aceman
mozilla: feedback+
Details | Diff | Splinter Review
patch v2 (4.07 KB, patch)
2012-05-26 09:10 PDT, :aceman
mozilla: review-
Details | Diff | Splinter Review
patch v3 (6.66 KB, patch)
2012-05-31 13:33 PDT, :aceman
mozilla: review-
Details | Diff | Splinter Review
patch v4 (6.73 KB, patch)
2012-06-01 14:43 PDT, :aceman
mozilla: review-
Details | Diff | Splinter Review
check for errors looking for numbers in account pref name (5.60 KB, patch)
2012-06-19 14:00 PDT, David :Bienvenu
mozilla: review+
acelists: feedback+
Details | Diff | Splinter Review
patch, check for errors looking for numbers in account pref name v2 [checked in, comment 38] (5.61 KB, patch)
2012-07-01 07:36 PDT, :aceman
acelists: review+
Details | Diff | Splinter Review
patch, move the pref to mailnews.js (1.80 KB, patch)
2012-07-02 11:53 PDT, :aceman
neil: review+
Details | Diff | Splinter Review

Description Andreas Wagner [:TheOne] 2009-03-29 16:39:17 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.9.0.8) Gecko/2009032609 Firefox/3.0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; de; rv:1.8.1.17) Gecko/20080914 Lightning/0.9 Thunderbird/2.0.0.17 Mnenhy/0.7.5.666 - Build ID: 2009030215

It seems that the id of a deleted account is reused when creating a new account.
This can produce a weird bug that mails from the deleted account show now the new account in the account column.

Reproducible: Always

Steps to Reproduce:
1. Create an account and receive some mails from it. Account column should show the right account name.
2. Delete this account and create a new one. The mails received in step 1) are now showing the new account in the account column.


Expected Results:  
Either: Keep the old account name
Or: Show no account name (empty entry in account column)
Comment 1 WADA 2009-03-29 17:12:11 PDT
It's current restriction hard to resolve or improve...

String in "Account" column is obtained from account definition using account number held in X-Account-Key: header. So, following phenomena can occur.
(A) Local mail folder(POP3/Local Folders) :
    If account numbering in prefs.js is altered, phenomenon like yours occurs.
(B) IMAP folder:
    Because no X-Account-Key: header, nothing is displayed in "Account" column.

> Either: Keep the old account name

If you want old account name in "Account" column, you should't have delete account definition of old account.

> Or: Show no account name (empty entry in account column)

If you want "empty entry in account column", you need to remove X-Account-Key: header from mail data, or need to alter data in X-Account-Key: header to non-existent account number.

Required recovery procedure, if you want "Keep the old account name":
(assume "accountX" is used in prefs.js/X-Mozilla-Keys: for old & new accounts.)
(call 2 accounts New-Account and Old-Account.) 

(1) Alter account number of New-Account to "accountY" in pres.js.
(2) Define Old-Account again, alter account number to "accountX" in prefs.js.
(3) Alter "X-Account-Key: accountX" in mail source of mails for New-Account
    to "X-Account-Key: accountY".
Comment 2 Andreas Wagner [:TheOne] 2009-03-30 03:12:42 PDT
Yes sorry, I was talking about Local Folders (POP3) only.

>     If account numbering in prefs.js is altered, phenomenon like yours occurs.
But I never altered the prefs.js

> If you want old account name in "Account" column, you should't have delete
> account definition of old account.
No, there are cases in which you haven't an account anymore but there are still some important emails from this account, and you still need them.

> If you want "empty entry in account column", you need to remove X-Account-Key:
> header from mail data, or need to alter data in X-Account-Key: header to
> non-existent account number.
Yes, that might work, but going through all my folders would take hours or even days. Not a practical way.

What about using something like an autoincrement for the account X-Account-Key? If I create a new account, TB first gets the current maximal X-Account-Key and then increases it by 1. Of course that would not affect the current issues but it would avoid future ones.

And imo this is really important to fix cause it shows wrong information to the user, produced by lazy account handling.
Comment 3 WADA 2009-03-30 18:01:20 PDT
(In reply to comment #2)
> > If account numbering in prefs.js is altered, phenomenon like yours occurs.
> But I never altered the prefs.js

I didn't say "You directly altered it".
As (a) you deleted Old_Account(accountX) and (b) the accountX was used by Tb upon creation of New_Account, the phenomenon occurred.
If (a)==false or (b)==false, the phenomenon doesn't occur.
Because (b) is current behavior of Tb when the accountX is smallest not-used account number, I said "you shouldn't have made (a)==true".
And you are requesting "make (b)==false always".
Comment 4 Andreas Wagner [:TheOne] 2009-03-31 04:49:04 PDT
> I didn't say "You directly altered it".
> As (a) you deleted Old_Account(accountX) and (b) the accountX was used by Tb
> upon creation of New_Account, the phenomenon occurred.

Sorry I misunderstood you.

> And you are requesting "make (b)==false always".

Yes that is what I actually would like to see. And it should not be too hard to implement it, and I can currently not see any side effects.
Comment 5 Andreas Wagner [:TheOne] 2009-04-20 10:57:17 PDT
I request this as wanted for TB3, as it has high impact for the user. But I think the fix for that would be rather easy.
Comment 6 Ludovic Hirlimann [:Usul] 2009-04-21 02:10:46 PDT
David where should one look to implement comment #2 ?
Comment 7 Mark Banner (:standard8) 2009-04-21 02:25:10 PDT
I don't understand this bug.

> Steps to Reproduce:
> 1. Create an account and receive some mails from it. Account column should show
> the right account name.
> 2. Delete this account and create a new one. The mails received in step 1) are
> now showing the new account in the account column.

So from comment 2 this is a pop account you've created in step 1.

First question: are you using global inbox?

Second question: in step 2 are you deleting and recreating the account with exactly the same settings? (if so, why?)

If the answer to the second question is yes, then it sounds more like the bug is the fact that we're not deleting the old account data, rather than the fact we're showing them again when the account is recreated.
Comment 8 Andreas Wagner [:TheOne] 2009-04-21 02:39:14 PDT
(In reply to comment #7)
> First question: are you using global inbox?

Yes.

> Second question: in step 2 are you deleting and recreating the account with
> exactly the same settings? (if so, why?)

No. I created a completely new pop account. Different email-address, different email-server, different provider.
Comment 9 Andreas Wagner [:TheOne] 2009-04-21 05:04:33 PDT
Maybe an example will make this more clearly:

Let's say I created a pop account for my address "me@mycompany.com" once. I now have some emails from people sent to this address.

Now, I don't work with this company anymore, so I delete the account from the account manager. But I have still some important emails (in my global inbox or a subfolder) from the account related to "me@mycompany.com" that I don't want to delete.
As expected, the "account"-column for this emails is empty now, cause the account itself does not exist anymore.

When I now create another account, let's say "one@myself.com", the emails that belonged once to the account of "me@mycompany.com" have now "one@myself.com" in their account-column.

The cause of this is already said here, let me explain it again:
Let's say "me@mycompany.com" had account number 2. The X-Account-Key for emails from this account is account2 (And it stays that way even after deletion of the account, which is not a problem itself).

The problem is, that the account for "one@myself.com" gets account number 2, too. Because that's the smallest number that is _currently_ not used. But it was used before! So, emails that have this account number in their header will be assigned to "one@myself.com" of course, even if they acutally belong to "me@mycompany.com"

And IMO the easiest way to fix this, is using every account number just once. Instead of getting the smallest unused account number, just get the highest used account number and increase it by once.
That would cause these old emails to have an empty account-column, but this should be nothing to worry about. It's far more better than having a wrong account displayed.

Hopefully, I could explain it a bit better now.
Comment 10 David :Bienvenu 2009-04-21 06:36:32 PDT
(In reply to comment #6)
> David where should one look to implement comment #2 ?

We could store a pref for the high water account number, I suppose, and check it in the code in nsMsgAccountManager that hands out new account numbers...see nsMsgAccountManager::getUniqueAccountKey
Comment 11 Andreas Wagner [:TheOne] 2009-04-21 06:48:16 PDT
Would be cool if someone of you could do that, I'm convinced it's easy for you, but for me, it's not.
Comment 12 Andreas Wagner [:TheOne] 2009-06-13 02:22:05 PDT
How's the chance to get this into TB3?
Comment 13 [:Aureliano Buendía] 2010-08-01 04:15:46 PDT
Set as New because I have observed this issue in my profile in the past!

Changed title, according to comment #9
Comment 14 :aceman 2012-05-24 02:47:15 PDT
The point of this bug is to make the "accountX" ids unique in time too, so that newly created accounts never get a value that may have been used previously by any account already deleted.

It seems David Bienvenu is in favor of this so I can look at this. I try to implement comment 2 and comment 10 using a pref containing the max account value created so far, which would increase at account creation only and never be decreased.
Comment 15 :aceman 2012-05-24 07:20:51 PDT
One outstanding issue here is what to do with existing profiles. The pref will be unset. So when nsMsgAccountManager::getUniqueAccountKey detects this it should initialize the pref to some good value. The problem is how to determine it. Can it just go through existing account ids and find the max value? I'd really not want to go through existing message to see what max id was ever used.
Comment 16 David :Bienvenu 2012-05-24 07:43:10 PDT
(In reply to :aceman from comment #15)
> One outstanding issue here is what to do with existing profiles. The pref
> will be unset. So when nsMsgAccountManager::getUniqueAccountKey detects this
> it should initialize the pref to some good value. The problem is how to
> determine it. Can it just go through existing account ids and find the max
> value? 
Yes, you could iterate over the existing mail.account pref branch looking for the highest value of mail.account.accountX - using the prefs instead of the loaded accounts will allow you to avoid stale accounts that may have been left in prefs.
Comment 17 :aceman 2012-05-24 07:50:55 PDT
Don't we actually want to see also the stale accounts that may have once existed? If there were accounts that are not there now, we'd like to skip their IDs.
What are loaded accounts?
Comment 18 David :Bienvenu 2012-05-24 07:56:33 PDT
(In reply to :aceman from comment #17)
> Don't we actually want to see also the stale accounts that may have once
> existed? 
Yes, we do. that's why I suggested looking at the prefs, not the list of accounts in the account manager pref.
If there were accounts that are not there now, we'd like to skip
> their IDs.
> What are loaded accounts?
the accounts in "mail.accountmanager.accounts" are the loaded accounts. There may be other mail.account.accountXX prefs where accountXX doesn't occur in the list defined by mail.accountmanager.accounts.

I agree that iterating over the existing messages looking for the biggest account is not a great idea either. Though if we had to do it, we could just special case the global inbox inbox, if there is one, and iterate over the db msg hdrs looking at the account property. But I don't think it's really worth it.
Comment 19 :aceman 2012-05-24 08:09:23 PDT
(In reply to David :Bienvenu from comment #18)
> the accounts in "mail.accountmanager.accounts" are the loaded accounts.
> There may be other mail.account.accountXX prefs where accountXX doesn't
> occur in the list defined by mail.accountmanager.accounts.

I understand now, thanks. This seems good.

Note to myself:
Services.prefs.getBranch("mail.account").
 getChildList(in string aStartingAt,
             [optional] out unsigned long aCount,
             [array, size_is(aCount), retval] out string aChildArray);
Comment 20 :aceman 2012-05-24 14:20:58 PDT
Created attachment 626964 [details] [diff] [review]
patch

This works for me. But it surely can be optimized in some ways, so I just request feedback. E.g. I'm not sure why I couldn't use m_prefs member so I had to get the pref service myself.

Open issue: I have left some NS_ENSUREs there but are they necessary? What happens if those pref calls fail?
Comment 21 David :Bienvenu 2012-05-24 17:30:42 PDT
Comment on attachment 626964 [details] [diff] [review]
patch

haven't tried running it, but it looks right.

if (StringHead(prefName, strlen(prefix)).Equals(prefix)) {

StringBeginsWith(prefName, nsDependentSting(prefix)) might be clearer.

It's a little goofy to be using prefix, since it only really works because the prefs look like mail.account.accountXX. prefix is always "account". We're pretending that this method is general, but that illusion is a bit shattered because we're now using a single pref to store the largest value for account key. We could either use "prefix" in the "lastKey" and GetBranch calls, or just say that this method is only used by one caller, remove the "prefix" param, and replace it with "account". I'm in favor of the latter, since this code is ancient, and we've never had anyone want to use it for different kinds of keys. Hope this makes sense!
Comment 22 :aceman 2012-05-24 22:39:42 PDT
I could add the prefix into the pref name, like this: mail.account.lastKey.account .
On the other hand, it is not sure the potential other prefixes would also want this same rule for the key (unique in time).
Comment 23 :aceman 2012-05-26 04:01:56 PDT
What about the returns in case of failure to get the prefs?
Comment 24 David :Bienvenu 2012-05-26 06:52:00 PDT
(In reply to :aceman from comment #23)
> What about the returns in case of failure to get the prefs?

I'm not sure what GetPrefBranch does if there are no prefs with the branch name. That is worth checking for the initial call to getUniqueAccountKey in a new profile. It might be cleaner to check if lastAccountKey is set before doing anything else (e.g., getting the pref branch).

If the pref service is broken, you could fall back on the old code that iterates over the accounts. But if the pref service is broken, it's unlikely anything else will work.
Comment 25 :aceman 2012-05-26 09:10:49 PDT
Created attachment 627490 [details] [diff] [review]
patch v2

nsDependentString didn't work so I used NS_LITERAL_CSTRING.
The rest should hopefully be according to your comment.
Comment 26 David :Bienvenu 2012-05-30 15:40:12 PDT
(In reply to :aceman from comment #22)
> I could add the prefix into the pref name, like this:
> mail.account.lastKey.account .
> On the other hand, it is not sure the potential other prefixes would also
> want this same rule for the key (unique in time).

I'd much rather get rid of the prefix parameter that no one has used for 10 years and simplify the code.
Comment 27 David :Bienvenu 2012-05-30 15:45:43 PDT
Comment on attachment 627490 [details] [diff] [review]
patch v2

yeah, sorry if I wasn't clear before. I'd get rid of the prefix param.

you need to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(prefCount, prefList);

once you're done with the ChildList, in order not to leak memory.
Comment 28 :aceman 2012-05-30 23:39:07 PDT
OK, thanks.

Can I still use the ACCOUNT_PREFIX define internally in the  getUniqueAccountKey function (for the old prefs, not the new key)?
Comment 29 :aceman 2012-05-31 13:33:04 PDT
Created attachment 628882 [details] [diff] [review]
patch v3

With the new way of determining the key the function would not even need to get "accounts" passed but if we want to keep the old way around then it is needed.

I had to update a test that expects a newly created account will be account1. Now it is account7. But I only tried the xpcshell tests.
Comment 30 David :Bienvenu 2012-06-01 14:05:09 PDT
Comment on attachment 628882 [details] [diff] [review]
patch v3

I think you missed this comment:

you need to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY(prefCount, prefList);

once you're done with the ChildList, in order not to leak memory.
Comment 31 :aceman 2012-06-01 14:43:12 PDT
Created attachment 629350 [details] [diff] [review]
patch v4

Yes, sorry.
Comment 32 :aceman 2012-06-19 07:26:32 PDT
David?
Comment 33 David :Bienvenu 2012-06-19 12:25:37 PDT
Comment on attachment 629350 [details] [diff] [review]
patch v4

sorry for the delay. This mostly looks reasonable, but I think you might want a bit of bulletproofing in the code that tries to get the number out of the pref name. If there's a pref with a name of the form mail.account.accountABC, your code will fall down a bit, both because "dotPos" will be -1, and atoi will fail. I'll see if I can find a cleaner way of doing this and update the bug with my results.
Comment 34 David :Bienvenu 2012-06-19 14:00:02 PDT
Created attachment 634582 [details] [diff] [review]
check for errors looking for numbers in account pref name

this shows what I had in mind. aceman, if you want to give this patch a try, I'd appreciate it, and if it's ok for you, I'll mark it r+ and it can go in.
Comment 35 David :Bienvenu 2012-06-19 14:04:39 PDT
Comment on attachment 634582 [details] [diff] [review]
check for errors looking for numbers in account pref name

marking r+, but waiting for aceman's feedback...
Comment 36 :aceman 2012-06-19 14:30:40 PDT
Comment on attachment 634582 [details] [diff] [review]
check for errors looking for numbers in account pref name

Thanks, that does seem to work on the bad pref key you imagined.

nit: shouldn't we use kNotFound instead of -1 ?
Comment 37 :aceman 2012-07-01 07:36:00 PDT
Created attachment 638188 [details] [diff] [review]
patch, check for errors looking for numbers in account pref name v2 [checked in, comment 38]

Only changed -1 to kNotFound. Using r=bienvenu, this can be checked in.
Comment 38 Ryan VanderMeulen [:RyanVM] 2012-07-01 09:33:48 PDT
https://hg.mozilla.org/comm-central/rev/3b4116610032
Comment 39 Philip Chee 2012-07-01 21:36:49 PDT
https://hg.mozilla.org/comm-central/rev/3b4116610032#l1.15

> +
> +
> +// Last used account key value
> +pref("mail.account.lastKey", 0);

You should either add this pref to seamonkey or move it to mailnews/mailnews.js
Comment 40 :aceman 2012-07-02 11:53:15 PDT
Created attachment 638439 [details] [diff] [review]
patch, move the pref to mailnews.js
Comment 41 Ryan VanderMeulen [:RyanVM] 2012-07-02 15:12:15 PDT
https://hg.mozilla.org/comm-central/rev/93113d33421c

Note You need to log in before you can comment on or make changes to this bug.