Closed Bug 505465 Opened 15 years ago Closed 15 years ago

local folders account shown duplicated (multiple accountN.server points server1 for "Local Folders" account)

Categories

(Thunderbird :: Folder and Message Lists, defect)

x86
Windows 7
defect
Not set
major

Tracking

(blocking-thunderbird3.0 .2+, thunderbird3.0 .2-fixed)

VERIFIED FIXED
Thunderbird 3.1b1
Tracking Status
blocking-thunderbird3.0 --- .2+
thunderbird3.0 --- .2-fixed

People

(Reporter: private, Assigned: Bienvenu)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, Whiteboard: [223 migration][blocks major update][duptome][gs][gssolved])

Attachments

(2 files, 5 obsolete files)

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1.1) Gecko/20090715 Firefox/3.5.1 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; de; rv:1.9.1b3pre) Gecko/20090223 Thunderbird/3.0b2 The local folder is shown twice. All Preference are same (include path). This ist only happend in Thunderbird 3 (beta). Thunderbird 2.xx shared Prefs with 3.xx. No "double Folders" in 2.xx Reproducible: Always Steps to Reproduce: 1. Delete .msf files and Directories in Profil Dir (local folders) 2. Rename profil Dir, but dont edit the path in Prefs; Thunderbird create a new, virgin Dir - same Problem 3. Actual Results: no results Expected Results: no expected results nothing importend - working well
can you add a screenshot ?
Keywords: regression
sure...
Summary: local folder shown twice → local folders account shown duplicated
Experiencing the same problem on Vista. Installed Th 3 into a separate directory using Th 2 profile. Looks like this is not OS specific see Bug 517886.
Problem similar to bug 303542? Check account/server related entries in prefs.js.
ok. just i', just a layman user, but managed to find the following in prefs.js: user_pref("mail.account.account1.server", "server1"); user_pref("mail.account.account2.server", "server1"); user_pref("mail.accountmanager.accounts", "account1,account2,account4,account5,account6,account7,account8"); where server1 is localfolders. Removed by hand and all is ok again. I haven't edited prefs.js before, so this must either have been ignored by th2 or caused by th3 migration.
(In reply to comment #5) > where server1 is localfolders. Removed by hand and all is ok again. What is your action of "Removed by hand"? Deleted mail.account2.xxx? Before the remove, what was defined as account1 and account2?
"by hand" as in edited prefs.js. account1 referred(and still does) to localfolders. no idea about account2. there were only 2 occurrences of account2 in prefs.js - the ones I've listed in comment5.
(In reply to comment #7) > account1 referred(and still does) to localfolders. > no idea about account2. > there were only 2 occurrences of account2 in prefs.js - the ones I've listed in comment5. Did you simply delete next line only? And problem disappeared? > user_pref("mail.account.account2.server", "server1");
yes I've removed the line: user_pref("mail.account.account2.server", "server1"); and changed user_pref("mail.accountmanager.accounts", "account1,account2,account4,account5,account6,account7,account8"); to user_pref("mail.accountmanager.accounts", "account1,account4,account5,account6,account7,account8");
Hi everybody, The manual method to fix it editing the prefs.js seems to work! I had the same bug after updated from TB 2.0 to TB 3.0 final through the regular TB update process (WinXP SP3) and I had the following lines in the prefs.js (because of a lot of accounts that are registered in my TB, I place some "[...]" into the text): " user_pref("mail.account.account1.server", "server1"); user_pref("mail.account.account10.identities", "id18"); user_pref("mail.account.account10.server", "server10"); [...] user_pref("mail.account.account16.identities", "id30"); user_pref("mail.account.account16.server", "server16"); user_pref("mail.account.account17.server", "server1"); user_pref("mail.account.account18.identities", "id1"); user_pref("mail.account.account18.server", "server17"); user_pref("mail.account.account19.server", "server18"); user_pref("mail.account.account2.identities", "id2"); user_pref("mail.account.account2.server", "server2"); [...] user_pref("mail.account.account8.identities", "id14"); user_pref("mail.account.account8.server", "server8"); user_pref("mail.account.account9.identities", "id16"); user_pref("mail.account.account9.server", "server9"); user_pref("mail.accountmanager.accounts", "account1,account2,account3,account4,account5,account6,account7,account8,account9,account10,account11,account12,account13,account14,account15,account16,account17,account18,account19"); " I saw a problem with the line "user_pref("mail.account.account17.server", "server1");". So I deleted the lines for accounts #17, #18 and #19 and I suppress the from the line "user_pref("mail.accountmanager.accounts")". Also, I added a line "user_pref("mail.account.account1.identities", "id1");" to match all other accounts in the list. This gives me this: " user_pref("mail.account.account1.identities", "id1"); user_pref("mail.account.account1.server", "server1"); user_pref("mail.account.account10.identities", "id18"); user_pref("mail.account.account10.server", "server10"); [...] user_pref("mail.account.account16.identities", "id30"); user_pref("mail.account.account16.server", "server16"); user_pref("mail.account.account2.identities", "id2"); user_pref("mail.account.account2.server", "server2"); user_pref("mail.account.account3.identities", "id4"); user_pref("mail.account.account3.server", "server3"); [...] user_pref("mail.account.account9.identities", "id16"); user_pref("mail.account.account9.server", "server9"); user_pref("mail.accountmanager.accounts", "account1,account2,account3,account4,account5,account6,account7,account8,account9,account10,account11,account12,account13,account14,account15,account16"); " At the next start of my TB 3.0, I don't have any duplicate of the Local Folders and my TB seems to work fine... Maybe someone would developp an automatic process in case of updating from any TB to TB 3.0 final to clean the prefs.js to make it match TB 3.0's rules... Regards, Johan
(In reply to comment #10) > user_pref("mail.account.account1.server", "server1"); > user_pref("mail.account.account17.server", "server1"); Two accounts point server(usually for "Local Folders"). > Also, I added a line "user_pref("mail.account.account1.identities", "id1");" to match all other accounts in the list. Please don't corrupt prefs.js. As "Local Folders" is special account(pseudo account), accountN.xxx for "Local Folders" doesn't have accountN.identities. Remove it if server1 is for "Local Folders". > user_pref("mail.account.account1.server", "server1"); > user_pref("mail.account.account10.identities", "id18"); > user_pref("mail.account.account10.server", "server10"); > [...] > user_pref("mail.account.account16.identities", "id30"); > user_pref("mail.account.account16.server", "server16"); > user_pref("mail.account.account17.server", "server1"); <= duplicate > user_pref("mail.account.account18.identities", "id1"); > user_pref("mail.account.account18.server", "server17"); > user_pref("mail.account.account19.server", "server18"); > user_pref("mail.account.account2.identities", "id2"); > user_pref("mail.account.account2.server", "server2"); > [...] > user_pref("mail.account.account8.identities", "id14"); > user_pref("mail.account.account8.server", "server8"); > user_pref("mail.account.account9.identities", "id16"); > user_pref("mail.account.account9.server", "server9"); If account2 is first account defined via UI of Tb 2 or before, account2 is usually defined as next(server1 is for "Local Folders"). > user_pref("mail.account.account2.identities", "id1"); > user_pref("mail.account.account2.server", "server2"); And next rarely happens. (account number==id number=server number). > accountN.identities = idN > accountN.server = serverN Do you repated delete/re-definition of accouts in the past? Did you edit prefs.js manually in the past? > user_pref("mail.accountmanager.accounts", "account1,account2,account3,account4,account5,account6,account7,account8,account9,account10,account11,account12,account13,account14,account15,account16,account17,account18,account19"); " If repeated delete/re-define of accounts via UI of Tb, accounts is never order by account number. Did you edit prefs.js manually in the past? As all reported cases to this bug and bug 517886 is "server1 is pointed by two accounts, and server1=Local Folders", problem is possibly next. By bug of old Tb, server1 was misused for server10. (10 to 19 was mapped to 1 to 9).
(In reply to comment #11) WADA > Thank you for your answer. Now I understand better how TB manages accounts ;-)... > > Also, I added a line "user_pref("mail.account.account1.identities", "id1");" to match all other accounts in the list. > > Please don't corrupt prefs.js. > As "Local Folders" is special account(pseudo account), accountN.xxx for "Local > Folders" doesn't have accountN.identities. Remove it if server1 is for "Local > Folders". Done. > Do you repated delete/re-definition of accouts in the past? I may have done that, but a few times and a long time ago and only with TB 2.x (that I use since its very early official releases; I never used TB 1.x)... > Did you edit prefs.js manually in the past? No, never.
(Correction of comment #11) (10 to 19 was mapped to 1 to 9) => (10 to 19 was mapped to 1, 20 to 29 was mapped 2, ...) (In reply to comment #12) > > Did you edit prefs.js manually in the past? > No, never. mail.accountmanager.accounts is ordered by account number. It indicates you finally defined accounts in account number order. And, as account2 uses id2/server2, and as account17 points server1 only(no id), and as account18 points id1/server17, and as Tb searches account/id/server number from 1 upon account/identity creation, I guess events like next happened: 1. Define first account with multiple identities at new profile. account1=>server1(Local Foders), account2=>id1,id2/server2, ... 2. Additional accounts(account3 to account16) are defined. as you use multiple identites, accountN=>idY/serverN where Y>N for N=3 to 16. 3. Tb's bug is produced. 4. You try to create account17, but Tb fails to create properly. account17=>server1 (by bug, not displayed), and the garbage remails. as account17 is not displayed by Tb2, account17 will never be deleted. 5. account2 is deleted => id1/id2 and server2 is deleted. 6. multiple identities of other account is defined. id1 is used. 7. account2 is re-defined. as id1/server1 is already used, account2 uses id2/server2. 8. id1 is deleted by removing identity of an account who uses id1. 9. Tb's bug is resolved. 10. account18 is defined. account18=>id1/server17 11. After upgarede to Tb3, account17=>server1 is exposed by Tb3.
Confirming per attached prefs.js data.
Status: UNCONFIRMED → NEW
Ever confirmed: true
We should try to fix this before massively updating 2.x users to 3.
blocking-thunderbird3.0: --- → ?
(In reply to comment #13) > [...], I guess events like next happened: > 1. Define first account with multiple identities at new profile. > account1=>server1(Local Foders), account2=>id1,id2/server2, ... > 2. Additional accounts(account3 to account16) are defined. > as you use multiple identites, accountN=>idY/serverN where Y>N > for N=3 to 16. > 3. Tb's bug is produced. > 4. You try to create account17, but Tb fails to create properly. > account17=>server1 (by bug, not displayed), and the garbage remails. > as account17 is not displayed by Tb2, account17 will never be deleted. > 5. account2 is deleted => id1/id2 and server2 is deleted. > 6. multiple identities of other account is defined. id1 is used. > 7. account2 is re-defined. > as id1/server1 is already used, account2 uses id2/server2. > 8. id1 is deleted by removing identity of an account who uses id1. > 9. Tb's bug is resolved. > 10. account18 is defined. account18=>id1/server17 > 11. After upgarede to Tb3, account17=>server1 is exposed by Tb3. Yes, that's this but with a very small difference: some months ago, I created an account (the last I've created for the moment) that I deleted just a few days after to re-create it properly. My TB 2.0 did not show any visible failure at any time.
I will try to write some code to eliminate the duplicate identical accounts.
Assignee: nobody → bienvenu
Status: NEW → ASSIGNED
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
This cleans out duplicate accounts, and adds a helper routine for recalculating and writing out the account list pref. Now to try to write a unit test...
is this the situation which leads some users to try deleting one of the accounts via UI? (sounds similar, but not quite same)
Severity: normal → major
Flags: in-testsuite?
Whiteboard: [223 migration]
Version: unspecified → 3.0
blocking-thunderbird3.0: ? → needed
Whiteboard: [223 migration] → [223 migration][has potential patch, needs test][blocks major update]
Attached patch now with unit test (obsolete) (deleted) — Splinter Review
The unit test tests a couple kinds of duplication of accounts, and for good measure, makes sure that removing an account correctly sets the account list pref.
Attachment #417773 - Attachment is obsolete: true
Attachment #418044 - Flags: superreview?(bugzilla)
Attachment #418044 - Flags: review?(bugzilla)
Whiteboard: [223 migration][has potential patch, needs test][blocks major update] → [223 migration][needs r/sr standard8][blocks major update]
hi all i also upgraded my tb2 to tb3 on my linux kubuntu karmic... have also this duplicate "local forders" issue. Is there now a fix/update for tb to get away the second "local folders"? thanks a lot thomi
Comment on attachment 418044 [details] [diff] [review] now with unit test I'm going to defer the review to Blake, if that's OK with him, since he's somewhat familiar with the account stuff from the autoconfig work.
Attachment #418044 - Flags: review?(bugzilla) → review?(bwinton)
I'll be happy to review the code.
Comment on attachment 418044 [details] [diff] [review] now with unit test >+++ b/mailnews/base/src/nsMsgAccountManager.cpp >@@ -620,31 +620,25 @@ nsMsgAccountManager::RemoveAccount >- // order is important! >- // remove it from the prefs first >- nsCString key; >- rv = aAccount->GetKey(key); >+ PRBool accountRemoved = m_accounts->RemoveElement(aAccount); >+ >+ rv = OutputAccountsPref(); >+ // If we couldn't write out the pref, restore the account. >+ if (NS_FAILED(rv) && accountRemoved) >+ m_accounts->AppendElement(aAccount); >+ Should we save the index of the removed item, and attempt to re-insert it at that point, or are we okay with potentially re-ordering the account list? > NS_ENSURE_SUCCESS(rv, rv); Also, if we handle the case where it failed, does it still make sense to call NS_ENSURE_SUCCESS? I also think we might want to add a return in the "if (NS_FAILED(rv) && accountRemoved)" block, rather than continuing on to unset the default account, and remove the server, and calling "aAccount->ClearAllValues()" down at the bottom of that function (as seen just below). >@@ -692,55 +686,37 @@ nsMsgAccountManager::RemoveAccount(nsIMs > aAccount->ClearAllValues(); > return NS_OK; > } > >-// remove the account with the given key. >-// note that this does NOT remove any of the related prefs >-// (like the server, identity, etc) It seems like a bit of a shame to remove the documentation instead of replacing it with better docs. :) Particularly, in this case, I think it might be good to mention that this is an O(n) algorithm, so that we aren't tempted to call it every time in a loop that adds or removes accounts. > nsresult >-nsMsgAccountManager::removeKeyedAccount(const nsCString& key) >+nsMsgAccountManager::OutputAccountsPref() […] >+ for (PRUint32 index = 0; index < numAccounts; index++) There's a trailing space at the end of this line. >+ account->GetKey(accountKey); >+ if (index) >+ mAccountKeyList.Append(','); I think we want to use ACCOUNT_DELIMITER here, not that we're ever going to change it, but for consistency. > return m_prefs->SetCharPref(PREF_MAIL_ACCOUNTMANAGER_ACCOUNTS, >- newAccountList.get()); >+ mAccountKeyList.get()); Shouldn't that be indented two spaces less, to line up with the "P"? And did we want to change createKeyedAccount to also use this to output the list of accounts? On the one hand, it would be a little bit slower, but on the other hand, it would make all the pref-writing code live in one place. My only other concern with this function is that we used to read the pref, and write it back, so if something happened to m_accounts, we wouldn't lose all the other accounts. That's probably unlikely, but I might feel better if we checked m_accountsLoaded, and if the accounts weren't loaded, we bailed out of this function. >@@ -1243,16 +1219,19 @@ nsMsgAccountManager::LoadAccounts() >+ nsCOMPtr<nsIPrefService> prefservice(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); Why are you using prefservice instead of getPrefService() and m_prefs? (I understand why you moved it out of the if statement, but I'm not sure why not just use the one that's there.) >@@ -1334,41 +1310,78 @@ nsMsgAccountManager::LoadAccounts() >+ for (PRUint32 i = 0; i < accountsArray.Length(); i++) { >+ // if we've already seen this account, note that we have a duplicate >+ // and advance to the next account. >+ if (accountsArray.IndexOf(accountsArray[i]) != i) >+ continue; We aren't really noting anything here, so we could probably make the comment simpler. (Maybe we could note that we found a duplicate, but I don't think we need to. :) >+++ b/mailnews/base/src/nsMsgAccountManager.h >@@ -126,22 +126,22 @@ private: >- /* internal destruction routines - fixes prefs */ >- nsresult removeKeyedAccount(const nsCString& key); >- > // sets the pref for the default server > nsresult setDefaultAccountPref(nsIMsgAccount *aDefaultAccount); > >+ // Write out the accounts pref from the m_accounts list of accounts. >+ nsresult OutputAccountsPref(); >+ I don't think it's wrong, but why did you move the location of the method declaration? >+++ b/mailnews/base/test/unit/test_accountMgr.js >+/** >+ * This tests that we cleanup the account prefs when the account manager is loaded. >+ * This entails removing duplicate accounts from mail.accountmanager.accounts list, >+ * and removing duplicate accounts with the same server. >+ */ >+const am = Components.classes["@mozilla.org/messenger/account-manager;1"] >+ .getService(Components.interfaces.nsIMsgAccountManager); >+ >+const prefs = Components.classes["@mozilla.org/preferences-service;1"] >+ .getService(Components.interfaces.nsIPrefBranch); >+ >+function run_test() >+{ >+ // Create account prefs with both kinds of duplication. >+ Every line after this ends in ^M, and probably shouldn't. >+ prefs.setCharPref("mail.account.account1.identities", "id1"); >+ prefs.setCharPref("mail.account.account1.server", "server1"); >+ prefs.setCharPref("mail.account.account4.identities", "id2"); […] >+ // Just make sure this doesn't throw an exception, because we did remove the >+ // default account. >+ let defaultAccount = am.defaultAccount; Do we know what the new default account will be, and if so, should we make sure that's what it is? (And that it's not, say, nil?) >+ // before it gets removed or else we'll assert, because we're This line ends in an extra space. >+ // not comletely initialized... typo: "comletely" -> "completely" >+ let flags = server.rootFolder.flags; >+ >+ am.removeAccount(am.FindAccountForServer(server)); >+ do_check_eq(am.accounts.Count(), 2); >+ do_check_eq(prefs.getCharPref("mail.accountmanager.accounts"), "account1,account5"); >+} Finally, there are a few lines that are longer than 80 characters that I think could be split without loss of readability. I'm going to call this an r-, but if you can answer my questions, I'ld be happy to switch it to an "r+ with nits". Thanks, Blake.
Attachment #418044 - Flags: review?(bwinton) → review-
blocking-thunderbird3.0: needed → .1+
Summary: local folders account shown duplicated → local folders account shown duplicated (multiple accountN.server points server1 for "Local Folders" account)
(In reply to comment #26) Will your patch resolve bug 534382's case? serverP.server=serverQ (P=a to b, Q=x to z) All serverQ(Q=x to z) has same hostname="Local Folders",userName="nobody". Will your patch resolve Bug 303542's case? serverP.server=serverQ (P=a to b, Q=x to z) All serverQ(Q=x to z) has same hostname="a.b.c",userName="xyz". Or this bug's case only? multiple accounts points same serverX. accountN.server=server1 (N=a to c) server1 is for "Local Folders" (account1.server=server1)
Comment on attachment 418044 [details] [diff] [review] now with unit test I'm going to have to redo this patch to make sure we're not ending up with deferred servers pointing at a removed account.
Attachment #418044 - Flags: superreview?(bugzilla)
I'm not sure if issues other than the one in this bug will be resolved by the new patch, but I'll try.
Whiteboard: [223 migration][needs r/sr standard8][blocks major update] → [223 migration][blocks major update]
There are four kinds of duplicate accounts, (a) multiple accounts point same serverX entry (a-1) this bug(bug 505465) serverX : hostname="Local Folders", userName=nobody, type=none (a-2) garbage by/after Bug 303542 (not reported yet) serverX : hostname="a.b.c", userName=pqr@xxx.yyy.zzz, type=not "none" (b) accounts use own serverX entry, but multiple serverX has same properties accountA.server=serverX | | accountC.server=serverZ (b-1) bug 534382 serverX : hostname="Local Folders", userName=nobody, type=none | serverZ : hostname="Local Folders", userName=nobody, type=none All above has same hostname/userName/type. (b-2) garbage by Bug 303542 serverX : hostname="a.b.c", userName=pqr@xxx.yyy.zzz, type=not "none" | serverZ : hostname="a.b.c", userName=pqr@xxx.yyy.zzz, type=not "none" All above has same hostname/userName/type. and other garbled account. (c) serverX is pointed by accountN.server=serverX (c-1) but ...serverX... doesn't exist (not reported yet) (c-2) some garbage entries exist for ...serverX (not reported yet) (manual edit of prefs.js or modification of prefs.js may produce it) If defferered_to_account was (a-1) or (b-1) which was removed, replacing by localfoldersserver is effective repair of defferered_to_account. If defferered_to_account was (a-2) or (b-2) which was removed, I think deletion of mail.server.serverP.defferered_to_account is sufficient, because user can be aware of something wrong by "hidden account appears on Tb3". Is next like process possible? (1) localfoldersserver=serverL, largerst serverX number=LL Keep account number pointed by deferred_to_account, and keep hostname/userName/type of the deferred_to_account (2) change ...serverL.... to ...server0... (to place top) (3) Remove duplicate serverX entry (for P=0 to LL) } If serverP doesn't exists, continue; HN=serverP.hostname; UN=serverP.userName; TP=serverP.type; (for Q=P+1 to LL) { If serverQ doesn't exists, continue; HNX=serverQ.hostname; UNX=serverQ.userName; TPX=serverQ.type; If (HN==HNX && UN=UNX && TP=TPX) { delete ...serverQ.xxx } } } (4) Remove accountN who point non-exsitent serverX (5) Remove duplicate accountN who points same serverX (6) change back ...server0... to ...serverL.... (7) If no one points serverX, delete ...serverX... (8) If deferred_to_account doesn't exist If hostname/userName/Type was one for "Local Folders" deferred_to_account=accountN who has accountN.server=serverL If hostname/userName/Type was not one for "Local Folders" delete deferred_to_account entry
Step (4)/(5)/(6) should have been next. (4) Remove duplicate accountN who points same serverX (5) change back ...server0... to ...serverL.... (6) Remove accountN who points non-exsitent serverX
Another garbage: (d) serverX is pointed by no one, but some ...server.serverx... exist. This sutuation was generated by Tb3 in bug 534382 for server2 by removing account2.server=server2. And Tb3 generated ...server2... for "Smart Folders", and garbage of server2.directory-rel was used for account2/server2.
Attached patch wip (obsolete) (deleted) — Splinter Review
this addresses some of the comments, but I need to fix the underlying issues as well.
Attachment #418044 - Attachment is obsolete: true
Re the prefService vs m_prefs question, I need nsIPrefService to call GetDefaultBranch; m_prefs is an nsIPrefBranch and thus doesn't have a GetDefaultBranch method.
Whiteboard: [223 migration][blocks major update] → [223 migration][blocks major update][duptome]
Whiteboard: [223 migration][blocks major update][duptome] → [223 migration][blocks major update][duptome][needs patch]
Attached patch wip 2 (obsolete) (deleted) — Splinter Review
this patch basically works, but I noticed that it doesn't remove the bogus server prefs, which it should do to help avoid future confusion. I also need to make sure the xpcshell test still passes...
Attachment #420198 - Attachment is obsolete: true
blocking-thunderbird3.0: .1+ → needed
I solved my problem with duplicate. Went into the account settings and simply deleted the accounts that were duplicated. Now all is well. Edduden
Whiteboard: [223 migration][blocks major update][duptome][needs patch] → [223 migration][blocks major update][duptome][needs patch][gs]
Flags: blocking-thunderbird3.1?
Flags: blocking-thunderbird3.1?
Attached patch proposed fix with unit test (obsolete) (deleted) — Splinter Review
This patch makes sure that when we remove duplicate accounts, we fixup accounts that were deferred to the duplicate accounts. I believe I've addressed most of your comments along the way, apologies if I overlooked some...
Attachment #420570 - Attachment is obsolete: true
Attachment #421519 - Flags: superreview?(bugzilla)
Attachment #421519 - Flags: review?(bwinton)
Attachment #421519 - Flags: review?(bwinton) → review+
Comment on attachment 421519 [details] [diff] [review] proposed fix with unit test A few general points: Sometimes you put the {s on a new line, and sometimes you put them on the end of the current line. I appreciate that the file itself is inconsistent, but I’ld feel a little better if you just picked a style for the code you're adding, and stuck to it. ;) There are also a few lines with trailing spaces. (I found 6.) And some of the lines are >80 characters which could be fairly easily split. Now, on to the more specific points: >+++ b/mailnews/base/src/nsMsgAccountManager.cpp >@@ -2088,16 +2141,101 @@ hashRemoveListener(nsCStringHashKey::Key >+/** >+ * This method gets called for every incoming server, and is passed a duplicate So, that's starting to trigger my n**2 sensor. Would it make sense to pass all the duplicate accounts at the same time so that we only call this once per server? On the other hand, n is going to be really kinda small, so it probably doesn't matter, if you would rather not change it. >+static PLDHashOperator >+hashCleanupDeferral(nsCStringHashKey::KeyType aKey, nsCOMPtr<nsIMsgIncomingServer>& aServer, void* aClosure) […] >+ nsCOMPtr<nsIPrefBranch> prefBranch( >+ do_GetService(NS_PREFSERVICE_CONTRACTID, &rv)); You don't check rv here. >+ rv = prefBranch->GetCharPref(accountPref.get(), getter_Copies(dupAccountServerKey)); >+ NS_ENSURE_SUCCESS(rv, PL_DHASH_NEXT); >+ nsCOMPtr<nsIPrefBranch> serverPrefBranch; >+ NS_ENSURE_SUCCESS(rv, PL_DHASH_NEXT); Is creating serverPrefBranch really going to set rv, or should this line be moved up two? >+ nsCString serverKeyPref(PREF_MAIL_SERVER_PREFIX); >+ serverKeyPref.Append(dupAccountServerKey); >+ serverKeyPref.Append('.'); >+ rv = prefservice->GetBranch(serverKeyPref.get(), >+ getter_AddRefs(serverPrefBranch)); So the serverKeyPref branch really ends in a '.'? (If it does, then please change to "s around the ".".) >+++ b/mailnews/base/test/unit/test_accountMgr.js […] >+ // Set the default account to one we're going to get rid of. The account >+ // manager should recover relatively gracefully. There are two spaces at the start of the line above. Aside from that, I'm pretty happy with it. r=me, with the nits fixed. Later, Blake.
(In reply to comment #39) > (From update of attachment 421519 [details] [diff] [review]) > A few general points: > > Sometimes you put the {s on a new line, and sometimes you put them on the end > of the current line. Yikes, I can't believe I used K&R braces - I must need a treatment! > > >+ * This method gets called for every incoming server, and is passed a duplicate > > So, that's starting to trigger my n**2 sensor. Would it make sense to pass all > the duplicate accounts at the same time so that we only call this once per > server? This is only called if there are dup accounts, and we clean up the dup accounts, so it should only ever get called once per profile, basically. And n is going to be small, as you say. > > >+ nsCString serverKeyPref(PREF_MAIL_SERVER_PREFIX); > >+ serverKeyPref.Append(dupAccountServerKey); > >+ serverKeyPref.Append('.'); > >+ rv = prefservice->GetBranch(serverKeyPref.get(), > >+ getter_AddRefs(serverPrefBranch)); > > So the serverKeyPref branch really ends in a '.'? > (If it does, then please change to "s around the ".".) Yes, it really did need a '.', which surprised me greatly. thx for the review; I'll clean up the nits and attach an other patch soonish.
Attached patch fix addressing Blake's comments (deleted) — Splinter Review
Attachment #421519 - Attachment is obsolete: true
Attachment #422543 - Flags: superreview?(bugzilla)
Attachment #422543 - Flags: review+
Attachment #421519 - Flags: superreview?(bugzilla)
Hi all have updated to TB 3.0.1 and this bug is still available.. double "Local Folders" entry.. Sorry, in above codes/fixes i don't see any manuall fix, how to solve that problem... When will it be fixed?.... no rush :) kind regards from switzerland thomi
The patch to fix this is still going through the review process, but we hope to fix it for 3.02, which will come out in a month or six weeks or so (we don't have a schedule yet)
I have three instances of local folder, i installed TB3 on XP into new folder, TB3 automatically imported my currently used TB2 profile.
blocking-thunderbird3.0: needed → .2+
Comment on attachment 422543 [details] [diff] [review] fix addressing Blake's comments ok, this looks fine. Although this kind of removal always gets me nervous. sr=Standard8. It'd be good to see if we can get some testers to try this out either before or when it gets onto 3.0.2.
Attachment #422543 - Flags: superreview?(bugzilla) → superreview+
fixed on trunk. Yeah, I'll try to recruit some tests...If anyone on the cc list wants to try a tb 3.1pre build from tomorrow and report back if it cleans up their duplicate accounts, that would be very helpful.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [223 migration][blocks major update][duptome][needs patch][gs] → [223 migration][blocks major update][duptome][gs]
Target Milestone: --- → Thunderbird 3.1b1
Will a case like this be fixed? mail.account.account1.server;server2 mail.account.account2.identities;id1 mail.account.account2.server;server1 mail.accountmanager.accounts;account1,account1,account1,account1,account1,account1,account1,account1,account1,account1,account1,account1,account1,account1,account2,account1,account1,account1,account1,account1,account1,account2 mail.accountmanager.defaultaccount;account2 mail.accountmanager.localfoldersserver;server2
yes, it should repair duplicates in the account list.
(In reply to comment #46) > fixed on trunk. Yeah, I'll try to recruit some tests...If anyone on the cc list > wants to try a tb 3.1pre build from tomorrow and report back if it cleans up > their duplicate accounts, that would be very helpful. I asked one user to try the 3.1 nightly, and he responded that it fixed his problems with multiple copies of the same account. http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_creates_multiple_copies_of_accounts
thx for the feedback, Jesper.
Attachment #422543 - Flags: approval-thunderbird3.0.2?
Whiteboard: [223 migration][blocks major update][duptome][gs] → [223 migration][blocks major update][duptome][gs][needs branch approval]
not yet fixed for 3.02
Attachment #422543 - Flags: approval-thunderbird3.0.2? → approval-thunderbird3.0.2+
Comment on attachment 422543 [details] [diff] [review] fix addressing Blake's comments Based on comment 49, I think we should go with this and get it out for a bit wider testing.
fixed for 3.02 - changeset: 4713:8c6630f83694
This bug has been around ever since TB3 became available for download. When will this be resolved? Enough is enough. If this has been fixed, when will this update become available?
There is an update available today that tries to repair this.
bingo.. :) thank you for your work... cheers thomi
or not?!! now i haven't any local folders.. they are gone :( in account settings they are also gone.. no local folder settings.. ??
oh.. self fixed.. look this.. after installing 3.0.2 i get this section in prefs.js: user_pref("mail.account.account10.server", "server9"); user_pref("mail.account.account2.server", "server1"); user_pref("mail.account.account3.identities", "id2"); user_pref("mail.account.account3.server", "server2"); user_pref("mail.account.account4.identities", "id3"); user_pref("mail.account.account4.server", "server3"); user_pref("mail.account.account5.identities", "id4"); user_pref("mail.account.account5.server", "server4"); user_pref("mail.account.account6.identities", "id6"); user_pref("mail.account.account6.server", "server5"); user_pref("mail.account.account7.identities", "id8"); user_pref("mail.account.account7.server", "server6"); user_pref("mail.account.account8.identities", "id10"); user_pref("mail.account.account8.server", "server7"); user_pref("mail.account.account9.identities", "id12"); user_pref("mail.account.account9.server", "server8"); user_pref("mail.accountmanager.accounts", "account3,account4,account5,account6,account7,account8,account9,account10"); user_pref("mail.accountmanager.defaultaccount", "account3"); user_pref("mail.accountmanager.localfoldersserver", "server1"); i compared prefs.js with a backup one and found, that in the line user_pref("mail.accountmanager.accounts", "account3,account4,account5,account6,account7,account8,account9,account10"); is account2 missing... so i changed/tested and voilà local folders are back.. fixed part from prefs.js: user_pref("mail.account.account10.server", "server9"); user_pref("mail.account.account2.server", "server1"); user_pref("mail.account.account3.identities", "id2"); user_pref("mail.account.account3.server", "server2"); user_pref("mail.account.account4.identities", "id3"); user_pref("mail.account.account4.server", "server3"); user_pref("mail.account.account5.identities", "id4"); user_pref("mail.account.account5.server", "server4"); user_pref("mail.account.account6.identities", "id6"); user_pref("mail.account.account6.server", "server5"); user_pref("mail.account.account7.identities", "id8"); user_pref("mail.account.account7.server", "server6"); user_pref("mail.account.account8.identities", "id10"); user_pref("mail.account.account8.server", "server7"); user_pref("mail.account.account9.identities", "id12"); user_pref("mail.account.account9.server", "server8"); user_pref("mail.accountmanager.accounts", "account2,account3,account4,account5,account6,account7,account8,account9,account10"); user_pref("mail.accountmanager.defaultaccount", "account3"); user_pref("mail.accountmanager.localfoldersserver", "server1"); thomi
Depends on: 548735
thomi, do you still have the backup prefs file? It would be really helpful if I could see that to figure out why we removed that account in an attempt to cleanup the duplicate accounts.
Thomas, this is now being worked on in bug 548735.
David, sorry, don't have the backed up one... still created a new backup after fixing it. thomi
thomi, thx anyway, I figured it out by staring at the code for a while...
Whiteboard: [223 migration][blocks major update][duptome][gs][needs branch approval] → [223 migration][blocks major update][duptome][gs][needs branch approval][gssolved
Status: RESOLVED → VERIFIED
Whiteboard: [223 migration][blocks major update][duptome][gs][needs branch approval][gssolved → [223 migration][blocks major update][duptome][gs][needs branch approval][gssolved]
Whiteboard: [223 migration][blocks major update][duptome][gs][needs branch approval][gssolved] → [223 migration][blocks major update][duptome][gs][gssolved]
Please, can you be more clear for an inexperience user like me? I do not know as much as you. Jaja Thanks. I tried to migrate from Postbox to Thunderbird. I backed it up the profile folder from postbox. Now I can`t see my local folders where a archived all my email older than a week. Can you help me? What do I have to do to show them on the left pane like it was before? Thanks Carlos
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: