592710 - Account manager permanently disables unrecognized extension-added account

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Kent James (:rkent)]

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.

STR:

  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.

Comment 1

[WADA:World Anti-bad-Duping Agency]

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
    mail.account.accountA.identities=idB
    mail.account.accountA.server=serverC
(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
    mail.identity.idB.xxx   : entries are kept
    mail.server.serverC.xxx : entries are kept
(3) Restart Tb, define an POP3 account.
    => accountA = idB + serverC is created 
    => mail.identity.idB.xxx   : re-used and modified
       mail.server.serverC.xxx : re-used and modified
       note: mail.server.serverC.directory-rel 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), mail.identity.idB.xxx and mail.server.serverC.xxx 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 mail.identity.idB.xxx and mail.server.serverC.xxx 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.

Note: 
Garbage of mail.server.serverC.deferred_to_account=accountN may produce problem, when entries of mail.server.serverC.xxx are re-used.

Comment 2

[Kent James (:rkent)]

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.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(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, server.name etc. only) 
    - Enable "delete account" only for the account of unkown server type.

Comment 4

[Kent James (:rkent)]

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.

Comment 5

[Kent James (:rkent)]

Attachment: Rev1

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.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 650311 [details] [diff] [review]
Rev1

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 <[e-mail]>
> + */
> +const am = Components.classes["@mozilla.org/messenger/account-manager;1"]

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

@@ +11,5 @@
> + */
> +const am = Components.classes["@mozilla.org/messenger/account-manager;1"]
> +                     .getService(Components.interfaces.nsIMsgAccountManager);
> +
> +const prefs = Components.classes["@mozilla.org/preferences-service;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", "pop3.host.org");
> +  prefs.setCharPref("mail.server.server2.type", "invalid");
> +  prefs.setIntPref("mail.server.server2.secondsToLeaveUnavailable", 2);  

nit: trailing space

Comment 7

[Kent James (:rkent)]

"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?

Comment 8

[Mark Banner (:standard8)]

(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));

Comment 9

[Kent James (:rkent)]

Attachment: Rev2: address review comments

Comment 10

[Kent James (:rkent)]

Attachment: unbitrotted patch landed

Checked in http://hg.mozilla.org/comm-central/rev/5faefe594378