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

ASSIGNED
Assigned to

Status

--
minor
ASSIGNED
5 years ago
6 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
aceman, can we improve bug summary to something like intelligently/rationally set default server?
(Assignee)

Comment 2

5 years ago
Sure, probably clicked too fast :)
Summary: if the current default server → if the current default account is not suitable to be default, pick a better default server as soon as it becomes available

Comment 3

5 years ago
> 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.
Depends on: 880464

Comment 4

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
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

5 years ago
Yeah, I seem to be missing a lot of things today... :-\
Is this bug for "defaultaccount setting upon account creation" only.
No plan to cover "defaultaccount setting upon or after account deletion(and modification too)"?
(Assignee)

Comment 9

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

Updated

5 years ago
No longer blocks: 854098

Updated

5 years ago
Blocks: 881114
(Assignee)

Comment 10

4 years ago
Created attachment 8425792 [details] [diff] [review]
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.
Attachment #8425792 - Flags: review?(neil)
(Assignee)

Updated

4 years ago
Severity: normal → minor
Status: NEW → ASSIGNED
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.
Attachment #8425792 - Flags: review?(neil) → review-
(Assignee)

Comment 12

4 years ago
(In reply to neil@parkwaycc.co.uk 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?
Flags: needinfo?(neil)
(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?
Flags: needinfo?(neil)
(Assignee)

Comment 14

4 years ago
(In reply to neil@parkwaycc.co.uk 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?
(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.
(Assignee)

Comment 16

4 years ago
Created attachment 8429467 [details] [diff] [review]
patch v2

OK, let's try this.
Attachment #8425792 - Attachment is obsolete: true
Attachment #8429467 - Flags: review?(neil)
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?)
(Assignee)

Comment 18

4 years ago
(In reply to neil@parkwaycc.co.uk 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.
(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?
(Assignee)

Comment 20

4 years ago
Created attachment 8496470 [details] [diff] [review]
patch v3

Another try, now with a test.
Attachment #8429467 - Attachment is obsolete: true
Attachment #8429467 - Flags: review?(neil)
Attachment #8496470 - Flags: review?(neil)
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.
(Assignee)

Comment 22

4 years ago
Created attachment 8503649 [details] [diff] [review]
patch v4
Attachment #8496470 - Attachment is obsolete: true
Attachment #8496470 - Flags: review?(neil)
Attachment #8503649 - Flags: review?(neil)
(Assignee)

Comment 23

4 years ago
(In reply to neil@parkwaycc.co.uk 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 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?
(Assignee)

Comment 25

4 years ago
Created attachment 8530949 [details] [diff] [review]
patch v5

Maybe this is a bit better?
Attachment #8503649 - Attachment is obsolete: true
Attachment #8503649 - Flags: review?(neil)
Attachment #8530949 - Flags: review?(neil)
(Assignee)

Comment 26

4 years ago
Neil: ping :)
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 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

3 years ago
Is there any reason this patch died?
(Assignee)

Comment 30

3 years ago
It was just postponed as I did not want to push it into TB38 so late. I can resume finishing it now.
(Assignee)

Comment 31

3 years ago
Created attachment 8598210 [details] [diff] [review]
patch v5.1

(In reply to neil@parkwaycc.co.uk 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.
Attachment #8530949 - Attachment is obsolete: true
Attachment #8530949 - Flags: review?(neil)
Flags: needinfo?(archaeopteryx)
Attachment #8598210 - Flags: review?(neil)

Comment 34

3 years ago
Is this worth having someone test a try build?
(Assignee)

Comment 35

3 years ago
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.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 37

3 years ago
Created attachment 8658861 [details] [diff] [review]
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.
Attachment #8598210 - Attachment is obsolete: true
Attachment #8598210 - Flags: review?(neil)
Flags: needinfo?(aryx.bugmail)

Comment 39

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

next step ?
You need to log in before you can comment on or make changes to this bug.