1559608 - failure composing message from Local folders if first identity is from a disabled account type

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[:aceman]

STR:
1.Have the first (non-Local folders) identity created for an account/server whose type can become unavailable (e.g. created by an addon via JS Accounts).
2.Disable/remove the addon for that server.
3.Restart TB, the account won't be in the folder pane.
4.Have NO default account specified, but actually have some other news or mail account defined.
5.Select Local folders in the folder pane.
6.Click Write.

Result:
1.params.identity is null in MsgComposeCommands.js:2821
2.The From menulist has nothing selected.
3.When closing the compose window:
NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsICommandManager.removeCommandObserver] MsgComposeCommands.js:3060

Reason:
When there is no default account, we pick the first identity in our internal list of identities:
https://searchfox.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2807

If that identity is from the disabled account type, it won't be in the list of identities in the From menupopup (we enumerate those via the accounts, so this one will be skipped).

Selecting an item not in the list throws errors and causes problems further down.

Comment 1

[:aceman]

Attachment: 1559608.patch

Let's use the first identity we actually have in the identity list (menupopup).

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

Looks OK to me. I'll land the TB part now.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

That more defensive goodness can go to beta as well.

Comment 4

[Jorg K (CEST = GMT+2)]

Pulsebot on strike again :-(
https://hg.mozilla.org/comm-central/rev/a949ea7ada533248cdf0f93b3777f7e6597706b5
When default account is disabled, pick first identity from menu popup instead of all identities. r=jorgk

Comment 5

[:aceman]

And how I could have no default account defined while still having an IMAP account around?
Because the default account pref points to an account that I removed recently and no new default was picked up.
Picking a new one (fixing the pref) was removed in bug 342632 because it was supposed to be added again in bug 880602, but that has unexpectedly stalled.
So now we can expect to see more problems like this, as TB is expecting to have a default account set whenever slightly possible (it even used non-mail accounts as default in the past).
We must resurrect bug 880602 fast.

Comment 6

[Ian Neal]

Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

>+++ b/suite/mailnews/components/compose/content/MsgComposeCommands.js

>+    if (identities && identities.length > 0) {
>+      params.identity = identities.queryElementAt(0, Ci.nsIMsgIdentity);
>+    } else {
>+      // Get the first identity we have in the list.
>+     let identitykey = identityList.getItemAtIndex(0).getAttribute("identitykey");
>+     params.identity = MailServices.accounts.getIdentity(identitykey);
Nit: indentation out by one in the two lines above.
>+    }
>   }
r=me with that fixed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/12611e8ca448
When default account is disabled, pick first identity from menu popup instead of all identities. r=IanN DONTBUILD

Comment 8

[Jorg K (CEST = GMT+2)]

Landed with indentation fixed.

Comment 9

[Jorg K (CEST = GMT+2)]

TB 68 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/650a204ba555ac20446ba48d8a7ee5c0f20bcbda (merged changesets)

Comment 10

[Ben Bucksch (:BenB)]

Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

Question: What builds the list of identities? I'd probably have used the same algo, to avoid picking stuff from the UI.

But if the list is guaranteed to be there and filled, and with correct values, I don't see a reason why not to do it this way.

NITs:
* HG header has wrong commit message
* Use {} even for 1-line |if| blocks. (Of course only applies to those that you add, don't refactor existing code in this patch.)
* Comment indented 1 space too much for seamonkey
(OK, these are really minor nits :)

Comment 11

[Ben Bucksch (:BenB)]

Ooops, seems like my inbox was a time machine. Sorry for being late to the party and dancing after everybody went home!

Comment 12

[Jorg K (CEST = GMT+2)]

Indeed, other people took care of the nits, apart from the braces.