Closed Bug 1774991 Opened 2 years ago Closed 2 years ago

"Edit draft" / "Edit as new message" doesn't populate address fields any more (existing profiles with encryption set up) when OpenPGP is configured for the identity

Categories

(Thunderbird :: Message Compose Window, defect, P2)

Thunderbird 102

Tracking

(thunderbird_esr102 fixed, thunderbird102? wontfix, thunderbird103 fixed)

RESOLVED FIXED
104 Branch
Tracking Status
thunderbird_esr102 --- fixed
thunderbird102 ? wontfix
thunderbird103 --- fixed

People

(Reporter: rachel, Assigned: KaiE)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

Seen in TB 102 beta 7:
"edit as new message" on any message in the Sent folder.

Actual results:

Addressing fields not populated.

Expected results:

Addressing fields should be populated.

Should there be a test for this?

Alice, can you please check where this went broken? "Edit as new message" has the Ctrl+E shortcut in the message list.

Flags: needinfo?(alice0775)

Edit draft loses the addressing details as well. This is a recent regression, still working in beta 5 (didn't try beta 6).

Summary: "edit as new message" doesn't populate address fields any more → "edit draft" / "edit as new message" doesn't populate address fields any more

Works for me in TB-beta102.0b7 as well as Daily103.0a1(20220619005013) Windows10.

Flags: needinfo?(alice0775)

Thanks for testing. It works on a new profile, but we see the failure on multiple existing profiles. Mozregression points to
Bug 1771122 - Remind user if encryption is possible but not enabled. r=mkmelin
Differential Revision: https://phabricator.services.mozilla.com/D147275
https://hg.mozilla.org/comm-central/rev/1a62e2afc99432552fd022db030584a988a4411b
https://hg.mozilla.org/comm-central/pushloghtml?fromchange=c17365dc7e4e34f901c18b54c115e3f788051db3&tochange=1a62e2afc99432552fd022db030584a988a4411b

This also got uplifted to 102.0beta6.

Flags: needinfo?(kaie)
Keywords: regression
Regressed by: 1771122

Regression confirmed via backout.

Blocks: tb102found

I don't doubt that this happens, but I cannot reproduce and I use edit as new frequently.
Did you ever rename the account or server for this account?

Probably related to whether you're set up for OpenPGP or S/MIME, or not.

(In reply to Wayne Mery (:wsmwk) from comment #6)

Did you ever rename the account or server for this account?

Looks like you were asking whether we were affected by bug 1773511. Yes, we were, but we don't recall renaming anything in years. BTW, we have both PGP and S/MIME configured (not sure about the validity of the S/MIME certificate though).

Confirming based on Rachel's analysis/confirmation in comment 4 and comment 5.
Not seen on new profile (per comment 4), i.e. requires existing profiles, plus encryption.
For affected users, if it happens as described, this will be a serious regression as saved drafts will lose all recipients every time you edit them again.

Rachel, does this bug happen for Reply/Reply-to-all too?

  • if yes, that makes this bug worse as it also breaks those scenarios
  • if not, imo there may be something wrong with the implementation logics of Bug 1771122: For replies (as opposed to edit draft/edit as new), is there a reason to not remind user if encryption is possible but not enabled?

Similarly, does this bug happen for Forward (and then manually adding recipients, which imo should recheck if we need to show the notification)? [Edited]

Severity: -- → S2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(rachel)
Priority: -- → P2
Summary: "edit draft" / "edit as new message" doesn't populate address fields any more → "Edit draft" / "Edit as new message" doesn't populate address fields any more (existing profiles with encryption set up)

Reply works. Not sure what you mean by Forward. Forward doesn't add recipients.

Flags: needinfo?(rachel)

(In reply to Rachel Martin from comment #10)

Reply works. Not sure what you mean by Forward. Forward doesn't add recipients.

Thanks for checking! I was thinking of Forward and then manually adding recipients, assuming that should recheck recipients and show notification to remind user if encryption is possible but not enabled if applicable.

Beta 7 works when we remove PGP for the identity, that is select "Do not use OpenPGP for this identity". Have you set up PGP and does "edit draft" / "edit as new message" work?

I can reproduce. It doesn't happen if I temporarily skip all actions oif checkRecipientKeys();

I added a console.trace() there, and I see it's called three times. Once very early during load, then again while loading flags from the draft, and then again after composer loading is ready.

I think we could skip that function while composer isn't fully ready, which fixes the bug for me.

Assignee: nobody → kaie
Flags: needinfo?(kaie)

Rachel, does the patch fix the bug for you?

Flags: needinfo?(rachel)

Yes (applied to MsgComposeCommands.js on an unpacked omni.ja). We didn't test that the notification comes up.

Flags: needinfo?(rachel)
Summary: "Edit draft" / "Edit as new message" doesn't populate address fields any more (existing profiles with encryption set up) → "Edit draft" / "Edit as new message" doesn't populate address fields any more (existing profiles with encryption set up) when OpenPGP is configured for the identity

@KaiE: From a cursory look at https://phabricator.services.mozilla.com/D147275 (from regressing Bug 1771122), I was also wondering about the following (if I read this right):

checkRecipientKeys does what it says and then shows the notification if encryption is possible but not enabled. The notification has an Encrypt button. The button has a callback which then calls checkRecipientKeys again even when recipients haven't changed.
Is this needed? I would probably expect a recheck whenever recipients change.

(In reply to Thomas D. (:thomas8) from comment #17)

The button has a callback which then calls checkRecipientKeys again even when recipients haven't changed.
Is this needed?

Yes, the function does different things, based on the encryption setting.

If the user enables encryption, the function checks the recipients, and adds a highlighting color for recipients that we cannot encrypt to.

You might say, but we know we can encrypt, because the button to enable encryption was shown. That's true, but we are an application with multiple windows, and an action in another window could have had the effect that encryption is no longer (e.g. the user changed acceptance for one of the keys).

Also, a key could still have been valid at the time we suggested to enable encryption, but a few seconds later, the key expired, and we can no longer encrypt.

I think it's safer to simply check again when we enable.

Geoff, could you please have a look at the patch? We need a hotfix for 102.

Flags: needinfo?(geoff)

Thanks Geoff. Let's land this is as a temporary hotfix, and see if we can find a different cause later. Keeping bug open.

Keywords: leave-open
Attachment #9282132 - Attachment description: Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=mkmelin → Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan

Depends on D149833

Attachment #9283606 - Attachment is obsolete: true
Attachment #9282132 - Attachment description: Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan → Bug 1774991 - While still loading composer, don't broadcast encryption change events. r=darktrojan
Status: NEW → ASSIGNED
Target Milestone: --- → 104 Branch

Pushed by nicolai@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/c4b6a72d280b
While still loading composer, don't broadcast encryption change events. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED

Comment on attachment 9282132 [details]
Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan

[Approval Request Comment]
Regression caused by (bug #): 1771122
User impact if declined: composer recipients fields will not be prefilled in multiple contexts
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low

Attachment #9282132 - Attachment description: Bug 1774991 - While still loading composer, don't broadcast encryption change events. r=darktrojan → Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan
Attachment #9282132 - Flags: approval-comm-esr102?
Attachment #9282132 - Flags: approval-comm-beta?

Comment on attachment 9282132 [details]
Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan

[Triage Comment]
Approved for beta

Attachment #9282132 - Flags: approval-comm-beta? → approval-comm-beta+
Flags: needinfo?(geoff)

Comment on attachment 9282132 [details]
Bug 1774991 - Don't call checkRecipientKeys() early during composer window loading. r=darktrojan

[Triage Comment]
Approved for esr102

Attachment #9282132 - Flags: approval-comm-esr102? → approval-comm-esr102+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: