1661510 - Encrypted Reply Drafts loose "Re:" prefix

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect, P2)

Description

[Patrick Brunschwig]

Reply to an encrypted message. Subject has Re: prefix. Save draft. Close message.

Go to drafts folder, edit draft. Result: Re: prefix is lost.

Note: Maybe the encrypted message already needs to be a reply.

Comment 1

[p≡p Project]

This seems to be a regression from bug 1633251. The p≡p team have fixed it in the p≡p add-on adding this line of code to the spot where the subject is prepared: if (msgHdr.flags & Ci.nsMsgMessageFlags.HasRe) hdrSubject = "Re: " + hdrSubject;

The equivalent spot in the Thunderbird code base would most likely at a call site of fixMessageSubject
https://searchfox.org/comm-central/rev/1bcbe36ec43f68f5fa52028adf43cd11e0389cd0/mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js#3159
at a location where the message header of the draft is available.

Comment 2

[Lasana Murray]

Attachment: 1661510.patch

Changes setOriginalSubject to detect when gMsgCompose.type is a draft with the HasRe flag.

Comment 3

[Lasana Murray]

Attachment: 1661510.patch

The test I wrote errors on windows, not quite sure why.

Comment 4

[Lasana Murray]

Try run in progress:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5faed4a9af6a0361a5d93fe2153b09cef4a05a6a

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9182495 [details] [diff] [review]
1661510.patch

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

Looks pretty ok, but need the test to pass of course.

For try runs, what you sent doesn't run the mac tests (so pointless). It did use to work, but haven't for some time...
Now you can use "-all" to get all when you need it, like, or use the task_config way

hg qnew -m "try: -b o -p all -u all --artifact" try && hg push -f -r tip cc-try && hg qpop && hg qdelete try

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +536,5 @@
> +   *                                 otherwise it is only updated for the
> +   *                                 following nsIMsgCompTypes: Draft,Template,
> +   *                                 EditTemplate,ForwardInline,
> +   *                                 ForwardAttachement or EditAsNew.
> +   */

would prefer for long lines, just wrap and indent by two more

::: mail/test/browser/openpgp/browser_composeWindow.js
@@ +64,5 @@
> +      getTestFilePath(
> +        "data/keys/[e-mail]"
> +      )
> +    )
> +  );

maybe here assert that you do get an id.

This task doesn't seeem to use any Assert at all. Mochitest usually doesn't allow one to have a test like that (which "doesn't do anything"), so I'm surprised.

@@ +96,5 @@
> +  // Delete the messages we saved to drafts.
> +  registerCleanupFunction(
> +    async () =>
> +      new Promise(resolve => {
> +        let msgs = [];

just use 

let msgs = [...draftsFolder.msgDatabase.EnumerateMessages()];

@@ +104,5 @@
> +          msgs.push(enumerator.getNext().QueryInterface(Ci.nsIMsgDBHdr));
> +        }
> +
> +        draftsFolder.deleteMessages(
> +          window.toXPCOMArray(msgs, Ci.nsIMutableArray),

please don't prefix it with window. 
But do put an 
var { toXPCOMArray } = ChromeUtils.import(
  "resource:///modules/iteratorUtils.jsm"
);

at the top of the file

@@ +144,5 @@
> +    select_click_row(0);
> +    open_selected_message();
> +
> +    let draftWindow = await draftWindowPromise;
> +    wait_for_window_focused(draftWindow);

might want to use let focus = BrowserTestUtils.waitForEvent(draftWindow, "focus", true);  and then await focus.

Comment 6

[Lasana Murray]

Attachment: 1661510v2.patch

The failure on OSX seems to be related to librnp not initializing properly. Not sure why but that's probably beyond the scope of this patch.

Comment 7

[Lasana Murray]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=193b450a10358da6fbf4e7dffc92e4b42c10232e

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 9183299 [details] [diff] [review]
1661510v2.patch

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

Looks good! r=mkmelin

::: mail/test/browser/openpgp/browser.ini
@@ +8,5 @@
>    mailnews.start_page.url=about:blank
>  subsuite = thunderbird
>  support-files = data/**
>  
> +[browser_composeWindow.js]

I'm sure we'll have many tests for the compose window, so please rename it to something more distinct. Like browser_openpgpDrafts.js

::: mail/test/browser/openpgp/browser_composeWindow.js
@@ +66,5 @@
> +      )
> +    )
> +  );
> +
> +  Assert.ok(id != null, "private key id received");

Nit: prefer falsy/truthy checks, so just Assert.ok(id, "private key id received");

Comment 9

[Magnus Melin [:mkmelin]]

It's probably best do do a try run without artifact to see there's no real problem for the test

Comment 10

[Lasana Murray]

Attachment: 1661510v3.patch

Renamed the file etc.

Comment 11

[Lasana Murray]

A try run without --artifact did not raise the failures mentioned before.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a58926d7023963fd421820c52d894b2fa902f194&selectedTaskRun=LDCFQVHyT9uhBZFP6-eJZQ.0

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9183743 [details] [diff] [review]
1661510v3.patch

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

LGTM, with one nit. r=mkmelin

::: mail/extensions/openpgp/content/ui/enigmailMsgComposeOverlay.js
@@ +540,4 @@
>      const CT = Ci.nsIMsgCompType;
>      let subjElem = document.getElementById("msgSubject");
>      let prefix = "";
> +    let prefixIsRe = false;

Can we change this variable name to "isReply"

Comment 13

[Lasana Murray]

Attachment: 1661510v4.patch

Variable renamed.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d55a97e68cc7
Keep "Re:" prefix when editing a draft reply to encrypted message. r=mkmelin

Comment 15

[Magnus Melin [:mkmelin]]

Comment on attachment 9183772 [details] [diff] [review]
1661510v4.patch

[Approval Request Comment]
User impact if declined: drafts lose Re: if they had one
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): not risky, has test

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9183772 [details] [diff] [review]
1661510v4.patch

[Triage Comment]
Approved for beta

Comment 17

[Rob Lemley [:rjl]]

Thunderbird 83.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/1271c48c0e88

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9183772 [details] [diff] [review]
1661510v4.patch

[Triage Comment]
Approved for esr78

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 78.4.1:
https://hg.mozilla.org/releases/comm-esr78/rev/139811c07cb7