Closed Bug 485839 Opened 15 years ago Closed 12 years ago

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)

Categories

(MailNews Core :: Account Manager, defect, P1)

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 16.0

People

(Reporter: TheOne, Assigned: aceman)

Details

(Whiteboard: [datalossy][see comment #9])

Attachments

(2 files, 5 obsolete files)

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)
Version: unspecified → 2.0
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".
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.
(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".
> 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.
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.
Flags: wanted-thunderbird3?
David where should one look to implement comment #2 ?
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.
(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.
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.
(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
Would be cool if someone of you could do that, I'm convinced it's easy for you, but for me, it's not.
How's the chance to get this into TB3?
Set as New because I have observed this issue in my profile in the past!

Changed title, according to comment #9
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Hardware: x86 → All
Summary: newly created account takes id of a deleted one -> mails of old account show the new account name → 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)
Whiteboard: [see comment #9]
Version: 2.0 → unspecified
Whiteboard: [see comment #9] → [datalossy][see comment #9]
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.
Product: Thunderbird → MailNews Core
QA Contact: account-manager → account-manager
Version: unspecified → Trunk
Flags: wanted-thunderbird3?
Priority: -- → P1
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.
Assignee: nobody → acelists
(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.
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?
(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.
(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);
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #626964 - Flags: feedback?(dbienvenu)
Status: NEW → ASSIGNED
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!
Attachment #626964 - Flags: feedback?(dbienvenu) → feedback+
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).
What about the returns in case of failure to get the prefs?
(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.
Attached patch patch v2 (obsolete) — Splinter Review
nsDependentString didn't work so I used NS_LITERAL_CSTRING.
The rest should hopefully be according to your comment.
Attachment #626964 - Attachment is obsolete: true
Attachment #627490 - Flags: review?(dbienvenu)
(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 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.
Attachment #627490 - Flags: review?(dbienvenu) → review-
OK, thanks.

Can I still use the ACCOUNT_PREFIX define internally in the  getUniqueAccountKey function (for the old prefs, not the new key)?
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #627490 - Attachment is obsolete: true
Attachment #628882 - Flags: review?(dbienvenu)
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.
Attachment #628882 - Flags: review?(dbienvenu) → review-
Attached patch patch v4 (obsolete) — Splinter Review
Yes, sorry.
Attachment #628882 - Attachment is obsolete: true
Attachment #629350 - Flags: review?(dbienvenu)
David?
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.
Attachment #629350 - Flags: review?(dbienvenu) → review-
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.
Attachment #629350 - Attachment is obsolete: true
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...
Attachment #634582 - Flags: review+
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 ?
Attachment #634582 - Flags: feedback+
Only changed -1 to kNotFound. Using r=bienvenu, this can be checked in.
Attachment #634582 - Attachment is obsolete: true
Attachment #638188 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/3b4116610032
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
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
Attachment #638439 - Flags: review?
Attachment #638188 - Attachment description: patch, check for errors looking for numbers in account pref name v2 → patch, check for errors looking for numbers in account pref name v2 [checked in, comment 38]
Attachment #638439 - Flags: review? → review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: