Closed Bug 1559608 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

3.04 KB, patch
jorgk-bmo
: review+
iannbugzilla
: review+
BenB
: feedback+
Details | Diff | Splinter Review

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.

Summary: failure composing message from Local accounts if first identity is from a disabled account type → failure composing message from Local folders if first identity is from a disabled account type
Attached patch 1559608.patchSplinter Review

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

Attachment #9072375 - Flags: review?(jorgk)
Attachment #9072375 - Flags: review?(iann_bugzilla)
Attachment #9072375 - Flags: feedback?(ben.bucksch)
Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

Looks OK to me. I'll land the TB part now.
Attachment #9072375 - Flags: review?(jorgk) → review+
Keywords: leave-open
Target Milestone: --- → Thunderbird 69.0
Comment on attachment 9072375 [details] [diff] [review]
1559608.patch

That more defensive goodness can go to beta as well.
Attachment #9072375 - Flags: approval-comm-beta+

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

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.

Blocks: 880602
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.
Attachment #9072375 - Flags: review?(iann_bugzilla) → review+
Pushed by mozilla@jorgk.com:
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

Landed with indentation fixed.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
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 :)
Attachment #9072375 - Flags: feedback?(ben.bucksch) → feedback+

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

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

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