Closed Bug 592710 Opened 14 years ago Closed 12 years ago

Account manager permanently disables unrecognized extension-added account


(MailNews Core :: Account Manager, defect)

Windows 7
Not set


(Not tracked)

Thunderbird 17.0


(Reporter: rkent, Assigned: rkent)



(1 file, 2 obsolete files)

When I add a new account type using an extension, it stops working if I disable, then re-enable the extension. This is because the Account Manager will remove any accounts that cannot create their server. Instead, the account manager should leave those accounts, but just not use them in the current session.


  1) Load an extension that create a new account type, and create such an account
  2) Disable the extension, and restart Thunderbird
  3) Re-enable the extension, and restart Thunderbird

Actual results:
  The previously added account does not exist
Expected results:
  The extension-added account should exist

If the extension-added account is account2/server2/id1 then the preferences that are removed (or modified) are:

mail.account.account2.identities  id1
mail.account.account2.server   server2
mail.accountmanager.accounts  account1,account2

Adding them back manually fixed the problem.
Phenomenon is observed with Tb 3.1.2, by manual change of prefs.js setting of mail.server.serverC.type(pop3=>Xpop3X).
(0) mail.accountmanager.accounts=...,accountA
(1) Change mail.server.serverC.type : pop3 => Xpop3X, restart Tb
    => accountA is not shown at folder pane, account manager
(2) Terminate Tb.
    mail.accountmanager.accounts     : accountA is removed from list
    mail.account.accountA.identities : entry is deleted
    mail.account.accountA.server     : entry is deleted   : entries are kept : entries are kept
(3) Restart Tb, define an POP3 account.
    => accountA = idB + serverC is created 
    =>   : re-used and modified : re-used and modified
       note: is not modified.
             i.e. directory for previous account is used.

According to step (3), step (2) is delete of account of unknown server type. And, as seen in step (3), and are re-used by new account, although some of the are re-used without expected modification.
I think Tb's behaviour is consistent, even though remained and at step (2) is very confusing for user. If "deletion of account of unknown server type" is not done at step (2), account/id/server defined by add-on will never be cleared once add-on is disabled/removed.

Garbage of mail.server.serverC.deferred_to_account=accountN may produce problem, when entries of are re-used.
Thanks for working to confirm this.

"I think Tb's behaviour is consistent"

I can see how the behaviour makes sense, but it does make life difficult for extensions that add account types. In my existing extension, I have a partial workaround, but I would like to collect points like this one for the future where I hope the core will make it easier to add new account types without difficulties like this one and others.
(In reply to comment #2)
> but it does make life difficult for extensions that add account types.

I agree with you. There is need to improve Tb's behaviour for add-ons.
(1) Never deletes "account of unkown server type" automatically, silently.
(2) Provide user a way to force delete of garbled account by remove of add-on. 
    - Show account of unkown server type via account manager,
      with minimum attributes display (sever.type, etc. only) 
    - Enable "delete account" only for the account of unkown server type.
I've really go to get this done for TB 17 so that it gets included in ESR as it causes great grief in new account type addons.
Assignee: nobody → kent
Attached patch Rev1 (obsolete) — Splinter Review
This patch adds hooks that would be used for an extension that added a new account type. At least in theory, it should not change any behavior for existing code.

The mailSession->RemoveFolderListener(this) is needed to prevent an assertion in the tests. AFAICT it is also correct to do that in the few uses in core code for UnloadAccounts(), but it is possible to work around that in js if it makes you nervous.
Attachment #650311 - Flags: review?(mbanner)
Comment on attachment 650311 [details] [diff] [review]

Review of attachment 650311 [details] [diff] [review]:

Sorry for the delay, I think this is generally fine, but I'm not too keen on the do ... while(false) constructs that you've got in there. Maybe you could take a look and discuss on irc?

::: mailnews/base/src/nsMsgAccountManager.cpp
@@ +369,5 @@
> +void
> +nsMsgAccountManager::GetUniqueServerKey(nsACString& aResult)
> +{
> +  nsCAutoString prefResult;
> +  do { // break on error to use internal scan

I'm not sure I really like this construct as it isn't really clear that it isn't actually a loop. Also, I think that if someone changed it later, they would be at risk of not realising it not being a loop.

Could you maybe use a flag variable instead?

@@ +383,5 @@
> +    if (NS_FAILED(rv))
> +      break;
> +    for (PRInt32 lastKey = 1; ; lastKey++)
> +    {
> +      nsCAutoString type;

type and typeKey should be placed outside the loop to avoid re-initialising/allocating memory each time.

@@ +392,5 @@
> +      typeKey.AppendLiteral(".type");
> +      prefBranchServer->GetCharPref(typeKey.get(), getter_Copies(type));
> +      if (type.IsEmpty())
> +        return;
> +    } 

nit: trailing space.

@@ +810,5 @@
>          if (NS_SUCCEEDED(rv)) {
>            // get incoming server
>            nsCOMPtr <nsIMsgIncomingServer> server;
> +          account->GetIncomingServer(getter_AddRefs(server));
> +          // server could be null if created by an unloaded extension

The comment needs to be on the line above, and I think you should case the result to (void) to indicate that this is intentional.

@@ +1470,5 @@
> +    // noticed that the server was unavailable. After a period of time
> +    // set in a preference, if the server is still unavailable then delete
> +    // the associated account.
> +    //
> +    // Preference definitions:

I think you should put some of this documentation into nsIMsgAccount - it says there an account incorporates a server as well, so I think you should make it clear that it is possible not to have servers.

@@ +1501,5 @@
>      nsCOMPtr<nsISupportsArray> identities;
>      account->GetIdentities(getter_AddRefs(identities));
> +    rv = account->CreateServer();
> +
> +    do // break to skip account delete

Again, I'm not too keen on this construct. Maybe a flag instead?

::: mailnews/base/test/unit/test_accountMgrCustomTypes.js
@@ +8,5 @@
> + *  is unloaded.
> + *
> + * Adapted from test_AccountMgr.js by Kent James <>
> + */
> +const am = Components.classes[";1"]

You can drop the const, and just use Services.accounts

@@ +11,5 @@
> + */
> +const am = Components.classes[";1"]
> +                     .getService(Components.interfaces.nsIMsgAccountManager);
> +
> +const prefs = Components.classes[";1"]

Drop the const, and just use Services.prefs

@@ +33,5 @@
> +  //  one month, not 2 seconds!) to tell the core code to leave this alone
> +  //  for awhile if the extension is unloaded.
> +  prefs.setCharPref("mail.server.server2.hostname", "");
> +  prefs.setCharPref("mail.server.server2.type", "invalid");
> +  prefs.setIntPref("mail.server.server2.secondsToLeaveUnavailable", 2);  

nit: trailing space
"I think you should case the result to (void) to indicate that this is intentional."

I don't understand what you want here - could you please elaborate?
(In reply to Kent James (:rkent) from comment #7)
> "I think you should case the result to (void) to indicate that this is
> intentional."
> I don't understand what you want here - could you please elaborate?

Change the line to be:

(void) account->GetIncomingServer(getter_AddRefs(server));
Attached patch Rev2: address review comments (obsolete) — Splinter Review
Attachment #650311 - Attachment is obsolete: true
Attachment #650311 - Flags: review?(mbanner)
Attachment #653865 - Flags: review?(mbanner)
Attachment #653865 - Flags: review?(mbanner) → review+
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.