880602 - if the current default account is not suitable to be default, pick a better default server as soon as it becomes available

Status: ASSIGNED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

Currently, if there is no good account to become the default server, nsMsgAccountManager::GetDefaultAccount chooses any first account to be the default and sticks to it later, even if a better account becomes available (is created) later. This attributes to problems like bug 854098.

I propose to change it so that this happens:

1. if only Local Account exists, it is reported as default.
2. if a rss/news account is created, still Local Accounts is the default (the first account)
3. if a pop3/imap4 account is created now TB will automatically switch to it as the default account.
4. if any new pop3/imap4 accounts are created after that, the account from step 3. is still the default.

Bug 880464 and bug 437139 is fixing this in the account wizard level.

I propose to also add some logic for this inside nsMsgAccountManager::GetDefaultAccount so in case this is not caught in the first line of defense (account wizard) we fix the default account later (or of a better account is created bypassing the wizard).

Comment 1

[Wayne Mery (:wsmwk)]

aceman, can we improve bug summary to something like intelligently/rationally set default server?

Comment 2

[:aceman]

Sure, probably clicked too fast :)

Comment 3

[rsx11m]

> 2. if a rss/news account is created, still Local Accounts is the default

This should be covered by bug 880464. Once bug 342632 is fixed, a better value would be "null" rather than Local Folders to indicate that no suitable default account is available.

> 3. if a pop3/imap4 account is created now TB will automatically switch to it
> as the default account.

Covered in bug 880464 as well, after moving that hunk over from bug 437139.

> 4. if any new pop3/imap4 accounts are created after that, the account from
> step 3. is still the default.

I thought this is by design? If you have a suitable default account defined and create a new mail account, the current account should stay the default unless you explicitly flip it.

Comment 4

[rsx11m]

(In reply to rsx11m from comment #3)
> > 4. if any new pop3/imap4 accounts are created after that, the account from
> > step 3. is still the default.
> I thought this is by design?

Eh, stupid me - you aren't proposing to change that, just reiterating the desired behavior which also happens to be the current behavior... :-)

Thus, the main point to consider here is to ensure that Local Folders is picked when no default account is available and no account has canBeDefaultServer set, or to find the first(?) server which has canBeDefaultServer set and make it the new default server.

Comment 5

[rsx11m]

(Quoting Florian Quèze from bug 739258 comment #2)
> (In reply to :aceman from bug 739258 comment #1)
> 
> > Instead of picking the one at '0', what about iterating over the accounts
> > and using the first that has canBeDefaultServer = true?
> 
> It's what the 20 or so lines above the code I've quoted do, see
> http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsMsgAccountManager.cpp#756

So, that part seems to be covered as well already.

Comment 6

[:aceman]

I said already in the description that bug 880464 also fixes this in AW :) I try to add another line of defense here for cases where somehow the user (or extension) bypasses AW.

Comment 7

[rsx11m]

Yeah, I seem to be missing a lot of things today... :-\

Comment 8

[WADA:World Anti-bad-Duping Agency]

Is this bug for "defaultaccount setting upon account creation" only.
No plan to cover "defaultaccount setting upon or after account deletion(and modification too)"?

Comment 9

[:aceman]

Yes exactly, this is ALSO about account deletion. I plan to check for a better account any time the Default account is queried.

Comment 10

[:aceman]

Attachment: patch

So this is my idea. I am not sure if the NS_ADDREF is needed, but I think all tests passed for me.
The change of m_defaultAccount to defaultAccount in nsMsgAccountManager::GetSortOrder fixes a crash after I tried to create a newsserver as the first account in TB. Before the patch m_defaultAccount and GetDefaultAccount would return the same value so it didn't matter. But now they differ. The change seems to be the right thing to do. The function probably originally wanted to use defaultAccount otherwise it wouldn't call GetDefaultAccount and used m_defaultAccount immediately. It also checks defaultAccount for !null, not m_defaultAccount, so that seems inconsistent.

Comment 11

[[e-mail]]

Comment on attachment 8425792 [details] [diff] [review]
patch

>-  if (m_defaultAccount.get() == aAccount)
>+  if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
This change makes no sense at all.

>-          SetDefaultAccount(firstAccount);
>+          // In this case we intentionally do not store the account
>+          // into m_defaultAccount so that GetDefaultAccount re-evaluates
>+          // the accounts each time it is called in the hope the user
>+          // sets up a better account soon.
>+          NS_ADDREF(*aDefaultAccount = firstAccount);
>+          return NS_OK;
Sorry, I don't think this is the right approach, because this could make GetDefaultAccount return different accounts without sending out notifications as the "default" account changes. I suppose it would be OK for GetDefaultAccount to look for a server that can be default if the current default account's server can't be default though.

Comment 12

[:aceman]

(In reply to [e-mail] from comment #11)
> >-  if (m_defaultAccount.get() == aAccount)
> >+  if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
> This change makes no sense at all.
Prevents dereferencing null as m_defaultAccount now can be null. Just a safety measure, no specific observed crash yet.

> >-          SetDefaultAccount(firstAccount);
> >+          // In this case we intentionally do not store the account
> >+          // into m_defaultAccount so that GetDefaultAccount re-evaluates
> >+          // the accounts each time it is called in the hope the user
> >+          // sets up a better account soon.
> >+          NS_ADDREF(*aDefaultAccount = firstAccount);
> >+          return NS_OK;
> Sorry, I don't think this is the right approach, because this could make
> GetDefaultAccount return different accounts without sending out
> notifications as the "default" account changes. I suppose it would be OK for
> GetDefaultAccount to look for a server that can be default if the current
> default account's server can't be default though.
So I can make it send the notifications (do the parts of SetDefaultServer that are relevant). Would that be enough?

Comment 13

[[e-mail]]

(In reply to aceman from comment #12)
> (In reply to comment #11)
> > >-  if (m_defaultAccount.get() == aAccount)
> > >+  if (m_defaultAccount && (m_defaultAccount.get() == aAccount))
> > This change makes no sense at all.
> Prevents dereferencing null as m_defaultAccount now can be null. Just a
> safety measure, no specific observed crash yet.
m_defaultAccount can be assigned to null, but that just changes the value of .get() to return null. (And in fact the .get() is mostly useless as the equality operator does a .get() automatically.)

> I can make it send the notifications (do the parts of SetDefaultServer
> that are relevant). Would that be enough?
How would you know what the previously returned default server was?

Comment 14

[:aceman]

(In reply to [e-mail] from comment #13)
> > I can make it send the notifications (do the parts of SetDefaultServer
> > that are relevant). Would that be enough?
> How would you know what the previously returned default server was?
So may I have a new variable?

Comment 15

[[e-mail]]

(In reply to aceman from comment #14)
> (In reply to comment #13)
> > > I can make it send the notifications (do the parts of SetDefaultServer
> > > that are relevant). Would that be enough?
> > How would you know what the previously returned default server was?
> So may I have a new variable?
Two variables to hold the real and virtual default server would be silly, but a flag of some sort would be OK I guess, it would save you from having to query the current default server to see if it was unsuitable each time.

Comment 16

[:aceman]

Attachment: patch v2

OK, let's try this.

Comment 17

[[e-mail]]

Comment on attachment 8429467 [details] [diff] [review]
patch v2

>+  if (m_defaultAccountTemporary) {
...
>+    if (NS_FAILED(rv) || !m_defaultAccount || m_defaultAccountTemporary) {
Won't m_defaultAccountTemporary still be true here anyway?

>   if (!m_defaultAccount) {
>     // Absolutely no usable account found. Error out.
>     NS_ERROR("Default account is null, when not expected!");
>+    SetDefaultAccount(nullptr);
(Why is this needed?)

>+  m_defaultAccountTemporary = !aDefaultAccount;
(This is for the deleting default account case?)

Comment 18

[:aceman]

(In reply to [e-mail] from comment #17)
> >   if (!m_defaultAccount) {
> >     // Absolutely no usable account found. Error out.
> >     NS_ERROR("Default account is null, when not expected!");
> >+    SetDefaultAccount(nullptr);
> (Why is this needed?)
So that m_defaultAccountTemporary is set inside SetDefaultAccount(nullptr) without open-coding it here :)


> >+  m_defaultAccountTemporary = !aDefaultAccount;
> (This is for the deleting default account case?)
Too, but also for the SetDefaultAccount(nullptr) calls I added.

Comment 19

[[e-mail]]

(In reply to aceman from comment #18)
> (In reply to comment #17)
> > >   if (!m_defaultAccount) {
> > >     // Absolutely no usable account found. Error out.
> > >     NS_ERROR("Default account is null, when not expected!");
> > >+    SetDefaultAccount(nullptr);
> > (Why is this needed?)
> So that m_defaultAccountTemporary is set inside SetDefaultAccount(nullptr)
> without open-coding it here :)
But how did m_defaultAccount become null in the first place?

Comment 20

[:aceman]

Attachment: patch v3

Another try, now with a test.

Comment 21

[[e-mail]]

Comment on attachment 8496470 [details] [diff] [review]
patch v3

So, this doesn't look as if it will quite work when all the accounts are unsuitable, but the preference is set to one anyway.

The first time you call GetDefaultAccount it will notice the unsuitable account set as default, and try to find a better account.

The second time you call GetDefaultAccount it will notice the current account is unsuitable, and fall back to the first account.

>+            if (!firstAccount)
firstAccount is always null here.

>+        SetDefaultAccount(m_defaultAccount);
This looks confusing, as it's not clear you're just clearing m_defaultAccountTemporary.

Comment 22

[:aceman]

Attachment: patch v4

Comment 23

[:aceman]

(In reply to [e-mail] from comment #21)
> Comment on attachment 8496470 [details] [diff] [review]
> patch v3
> 
> So, this doesn't look as if it will quite work when all the accounts are
> unsuitable, but the preference is set to one anyway.
Yes, I set the pref so that we save the account for next start and keep it until a better one is found. If we didn't save it some other "first" account could take over next time.

I hope I solved the other found problems, thanks.

Comment 24

[[e-mail]]

Comment on attachment 8503649 [details] [diff] [review]
patch v4

>     uint32_t count = m_accounts.Length();
>     if (!count) {
>-      *aDefaultAccount = nullptr;
>+      NS_WARNING("No valid default account found. No accounts are set up.");
>+      SetDefaultAccount(*aDefaultAccount = nullptr);
[Not sure how you can have a default account that you need to clear when there are no accounts.]

>+    nsCOMPtr<nsIMsgAccount> firstAccount;
>+
>+    if (!m_defaultAccount) {
...
>+    } else {
>+      // The current default account is only temporary.
>+      // If we find no better one later on, keep this one to avoid random
>+      // default account changes.
>+      firstAccount = m_defaultAccount;
>+    }
The logic is very confusing here. Better would be
// We prefer to have a suitable account as the default account.
// But if we don't have a suitable account, prefer to keep the existing unsuitable account as the temporary default account.
nsCOMPtr<nsIMsgAccount> firstAccount = m_defaultAccount;
if (!firstAccount) {
...
}

>+      nsCString defaultKey;
>+      rv = m_prefs->GetCharPref(PREF_MAIL_ACCOUNTMANAGER_DEFAULTACCOUNT, getter_Copies(defaultKey));
>+      if (NS_SUCCEEDED(rv)) {
>+        rv = GetAccount(defaultKey, getter_AddRefs(m_defaultAccount));
>+        if (NS_SUCCEEDED(rv)) {
>+          nsCOMPtr <nsIMsgIncomingServer> server;
>+          // server could be null if created by an unloaded extension
>+          (void) m_defaultAccount->GetIncomingServer(getter_AddRefs(server));
>+
>+          bool canBeDefaultServer = false;
>+          if (server)
>+          {
>+            (void) server->GetCanBeDefaultServer(&canBeDefaultServer);
>+            // If we find no better default account later, use this one.
>+            firstAccount = m_defaultAccount;
>+          }
>+          if (!canBeDefaultServer)
>+            m_defaultAccount = nullptr;
>+        }
>+      }
>+      if (m_defaultAccount)
>+        m_defaultAccountTemporary = false;
And this is confusing too; if the preferred account can be default, why not just call SetDefaultAccount and call it a day?

Comment 25

[:aceman]

Attachment: patch v5

Maybe this is a bit better?

Comment 26

[:aceman]

Neil: ping :)

Comment 27

[[e-mail]]

Comment on attachment 8530949 [details] [diff] [review]
patch v5

>+        rv = GetAccount(defaultKey, getter_AddRefs(m_defaultAccount));
This should be firstAccount. If it can be the default it will become m_defaultAccount when you call SetDefaultAccount.

>+          bool canBeDefaultServer = false;
>+          if (server) {
>+            (void) server->GetCanBeDefaultServer(&canBeDefaultServer);
>+            // If we find no better default account later, use this one.
>+            firstAccount = m_defaultAccount;
>+          }
>+          if (!canBeDefaultServer)
>+            m_defaultAccount = nullptr;
>+          else
>+            SetDefaultAccount(m_defaultAccount);
You can then move this code inside the if (server) block.

>+  m_defaultAccount = nullptr;
[Should you set m_defaultAccountTemporary here?]

>+  bool m_defaultAccountTemporary;
Please put this near other bools otherwise it will waste alignment space.

Comment 28

[[e-mail]]

Comment on attachment 8530949 [details] [diff] [review]
patch v5

>+  // We prefer to have a suitable account as the default account.
>+  // But if we don't have a suitable account, prefer to keep the existing
>+  // unsuitable account as the temporary default account.
>+  nsCOMPtr<nsIMsgAccount> firstAccount = m_defaultAccount;
Also I think it might be possible to move this nearer its use.

Comment 29

[Kent James (:rkent)]

Is there any reason this patch died?

Comment 30

[:aceman]

It was just postponed as I did not want to push it into TB38 so late. I can resume finishing it now.

Comment 31

[:aceman]

Attachment: patch v5.1

(In reply to [e-mail] from comment #27)
> Comment on attachment 8530949 [details] [diff] [review]
> patch v5
> >+  m_defaultAccount = nullptr;
> [Should you set m_defaultAccountTemporary here?]
I think it is still at "true" so that is what it should stay at here. The other branch sets it to false in SetDefaultAccount().

Aryx, please push to try server, all tests.

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fe029579068d

Comment 33

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed again because previous build failed due to m-c changes: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=060ae869c9f0

Comment 34

[Wayne Mery (:wsmwk)]

Is this worth having someone test a try build?

Comment 35

[:aceman]

I think not. There is a test outlining the steps to check. And the test does it so I don't think manual testing is needed.

But it could use a repush to try server as the previous run had many unrelated failures. Hopefully today the trunk is more green.

Comment 36

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

New Try push: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6045eb86eb48

Comment 37

[:aceman]

Attachment: patch v5.2

Interestingly only the Windows debug builds did crash in my test. I found the test pointed 2 accounts to the same server object. It seems it crashed when setting up the storage for one of the accounts. The accounts were of different types (IMAP and POP) so there could be some clash. And Windows does not like if open files are deleted.

Aryx please push the new version.

Comment 38

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

New Try push: https://hg.mozilla.org/try-comm-central/rev/ccbf0af72f01

Comment 39

[Wayne Mery (:wsmwk)]

(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #38)
> New Try push: https://hg.mozilla.org/try-comm-central/rev/ccbf0af72f01

next step ?

Comment 40

[:aceman]

Attachment: 880602-test.patch

If bug 342632 is implemented first, that quite simplifies the problem and there is actually no further change needed to GetDefaultAccount(). There is no longer an unusable account returned as default that we would need to track (as some temporary) and replace with a better one once a new one is available.

So I just add the test here that should prove the feature.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=208213396&revision=8dca8c5b99bc4a973f60e00fedfec63eaa2d4172

Comment 41

[Ben Bucksch (:BenB)]

Comment on attachment 9020581 [details] [diff] [review]
880602-test.patch

Review of attachment 9020581 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the test.

Comment 42

[Ben Bucksch (:BenB)]

And yes, the approach makes sense to me.

Comment 43

[:aceman]

Attachment: 880602.patch

So this will now pick a new default account if there is none yet and a new account is created. But it must wait only after incomingServer is set to check if it is usable.

The check in LoadAccounts() is for the case we have a default stored in the prefs at startup but it is not usable (e.g. due to unloaded addon). Then m_defaultAccount stays at nullptr and we will automatically choose another default.

Try run with all the parts:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=212721397&revision=7fbe2ab075cf4acb8d3db0f982ac8bd398f2b50f

Well, there could be a case where the account is OK a and we suddenly set its server to an unusable type. I still need to cover that case.

Comment 44

[:aceman]

Attachment: 880602-AC.patch

The account creation hunk again, where we do not need to set the default account explicitly as it will be set automatically in the SetIncomingServer().

Comment 45

[:aceman]

Attachment: 880602-test.patch

The test again.

Comment 46

[Ben Bucksch (:BenB)]

Comment on attachment 9026285 [details] [diff] [review]
880602-AC.patch

I like the idea that you're setting the default acccount in SetIncomingServer()

Comment 47

[Ben Bucksch (:BenB)]

Comment on attachment 9026286 [details] [diff] [review]
880602-test.patch

Review of attachment 9026286 [details] [diff] [review]:
-----------------------------------------------------------------

This is a really nice and useful unit test. I like the cases you implemented, and the story you told (the comments), with the explanations.

How tests should look like! Good work!

::: mailnews/base/test/unit/test_accountDefault.js
@@ +24,5 @@
> +  let nntpAccount = MailServices.accounts.createAccount();
> +  nntpAccount.incomingServer = nntpServer;
> +  nntpAccount.addIdentity(identity);
> +
> +  Assert.equal(nntpAccount.key, "account1");

This is not a useful check. It if the key was "nttpAccountForFred", it's not a bug.
Same below.

(The length check makes sense, because it ensures that the account was created.)

@@ +54,5 @@
> +
> +  Assert.equal(MailServices.accounts.accounts.length, 3);
> +  Assert.equal(imapAccount.key, "account3");
> +  // Now IMAP is much better and can be default. We switch to it automatically.
> +  Assert.equal(MailServices.accounts.defaultAccount, imapAccount);

Good

@@ +70,5 @@
> +  popAccount.addIdentity(identity);
> +
> +  Assert.equal(MailServices.accounts.accounts.length, 4);
> +  Assert.equal(popAccount.key, "account4");
> +  // POP3 is no better if we already have IMAP so IMAP is still default.

spelling NIT: comma before "so"
// POP3 is no better than the IMAP account that we already have, so IMAP is still the default.
same below: another ", so"
ditto below: comma before "but", twice

Comment 48

[Ben Bucksch (:BenB)]

(r+ of course under the condition that my 2 small remarks are fixed, everywhere in the file.)

Comment 49

[:aceman]

(In reply to Ben Bucksch (:BenB) from comment #47)
> This is a really nice and useful unit test. I like the cases you
> implemented, and the story you told (the comments), with the explanations.
> 
> How tests should look like! Good work!

Thanks :)
 
> ::: mailnews/base/test/unit/test_accountDefault.js
> @@ +24,5 @@
> > +  let nntpAccount = MailServices.accounts.createAccount();
> > +  nntpAccount.incomingServer = nntpServer;
> > +  nntpAccount.addIdentity(identity);
> > +
> > +  Assert.equal(nntpAccount.key, "account1");
> 
> This is not a useful check. It if the key was "nttpAccountForFred", it's not
> a bug.

We made a fix in the past (bug 485839) to make the account keys unique for the lifetime of the profile. If suddenly the keys started to appear like nttpAccountForFred, which may not be unique, we would like to detect it. This may not be the right test file to do it, so I can remove this. It seems we have a test for this behaviour in test_acctRepair.js.

Comment 51

[Jorg K (CEST = GMT+2)]

(In reply to Ben Bucksch (:BenB) from comment #47)
> spelling NIT: comma before "so"
> ditto below: comma before "but", twice
Check https://www.grammarly.com/blog/comma-before-so/. Yes, this sentence should have one:
POP3 is no better than the IMAP account that we already have, so IMAP is still the default.
I looked at the cases below, all should have commas as Ben requested.

Comment 52

[:aceman]

Attachment: 880602-test.patch v1.1

Thanks.

Comment 53

[Magnus Melin [:mkmelin]]

Comment on attachment 9026284 [details] [diff] [review]
880602.patch

Review of attachment 9026284 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, r=mkmelin

Comment 54

[:aceman]

Ben, do you want to check the main patch (the feedback request)?

Comment 55

[Ben Bucksch (:BenB)]

aceman,
I didn't comment on this, because I don't feel well about this particular patch. There are a number of changes in there that to me looks somewhat dangerous in the sense that they might break something, but I cannot pinpoint an exact situation.

Also, I do not understand why you make these changes in the first place, what the concept behind it is.

What remains is the feeling that this patch might break stuff, but it remains a feeling and nothing factual. I don't want to be resisting stuff just because I don't understand it, so I stayed silent.

Concretely, the parts that came up as red flags for me:

• // Nothing usable was found.
• return SetDefaultAccountInternal(nullptr);

Why would you do that? You can't do anything meaningful, so why not leave things as they are? Apparently, they must be working, so why touch the working status quo (of a certain default account in the profile) and "fix" it with a null account? It worked so far, so you're not really improving much, but you could break stuff, if your algo goes wrong or you overlooked a situation.

• if (m_defaultAccount.get() == aAccount)
• AutosetDefaultAccount();

• if (m_defaultAccount.get() == aAccount) {
• rv = AutosetDefaultAccount();
• NS_ENSURE_SUCCESS(rv, rv);

Why this ENSURE_SUCCESS being added? You're in Remove account? Are you sure that you want to prevent the removal of the account in case this was the default and that was the last suitable account?
If you are sure that you want to prevent it, make sure you a) leave things in a working state and b) that you inform the user why he cannot remove this account. You're not doing either of those. Specifically, it seems that the user would end up with a half-removed account and a broken setup. That's why I think this particular change is wrong.

(Personally, I'd prefer to keep code style changes and logic changes in separate patches. I find these irrelevant changes distracting.)

4. You're calling GetDefaultAccount() and if there's no default account autoset default account on every load. Do we need that? Is that the core of what you're trying to achieve here, that we ensure on startup that we set a proper default account?
   If that's your goal, please describe that clearly, why you think we should do that, which specific situation this improves, and limit the patch only to this one single change.

---

I think you could ignore my comment 4. and 1. if you have a good reason, 3. is style and I think 2. is important to fix.
I'm going to give r- for point 2. specifically, because it really concerns me.

Sorry for being so negative about the patch. I didn't want to be negative and wasn't sure, so I stayed silent, but since you insisted on my comment...

Comment 56

[Ben Bucksch (:BenB)]

Comment on attachment 9026284 [details] [diff] [review]
880602.patch

r- due to comment 2. above.