1961106 - remove EnigmailFuncs.sync() usage where we easily can

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, task)

Description

[Magnus Melin [:mkmelin]]

EnigmailFuncs.sync() is an old and evil way of faking sync functionality where async functionality happens. Basically busy-wait...
Not sure we can easily remove all usage but many of the cases are fairly trivial to rework as much of the other code now uses real async.

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: Bug 1961106 - use proper async for decryptMessage() and its callers. r=kaie

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: Bug 1961106 - Get rid of sync version of EnigmailKeyRing.importKey() in favor of the async version. r=kaie

Also adding some tags to allow easier running of all the openpgp tests like

./mach xpcshell-test --tag=openpgp comm/
./mach mochitest --headless --tag=openpgp comm/

Comment 3

[Magnus Melin [:mkmelin]]

Good try for these: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9cd7590c9675c6e73fd18566c54838f62f53ccdc&selectedTaskRun=Qk9mqTQHT5Gdcl06-t-ocQ.0

Unfortunately the other cases are proving more difficult...

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0de231cb826b
use proper async for decryptMessage() and its callers. r=kaie
https://hg.mozilla.org/comm-central/rev/381e2ce8c4ab
Get rid of sync version of EnigmailKeyRing.importKey() in favor of the async version. r=kaie

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: Bug 1961106 - remove EnigmailFuncs.sync() from message sending. r=edicharry

Comment 6

[Magnus Melin [:mkmelin]]

A few of calls will be left after the above patch lands. Unfortunately it turned out those are not easy to replace. There's some libmime interaction with sync call assumptions, so leaving it for a future bug.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0832b219daf1
remove EnigmailFuncs.sync() from message sending. r=edicharry

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). TB 140 patch. r=edicharry

TB 140 version of patch.

Comment 9

[Kai Engert [:KaiE:]]

Reopening until we have backed out the regressor patch.

The backout patch for c-c needs manual merging, because of context changes.
I'll work on that.

Comment 10

[Kai Engert [:KaiE:]]

Attachment: Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). r=edicharry

Comment 11

[Wayne Mery (:wsmwk)]

The regressions issues lack automated test?

Comment 12

[Kai Engert [:KaiE:]]

In phab attachment 9490051 [details]
I've found at least one bug, see my comment there.

Comment 13

[Kai Engert [:KaiE:]]

Comment on attachment 9496523 [details]
Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). TB 140 patch. r=edicharry

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: Fixes functional regression
• User impact if declined: Unable to send signed/encrypted messages, in some legitimate configurations.
  (While the related code is generally covered by automated tests, we don't cover the specific configuration that failed.)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Daily?: No
• Has the fix been verified in Beta?: No
• Needs manual test from QA?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Restoring code that worked previously.
• String changes made/needed: Patch deliberately doesn't restore old strings, but the string was very unlikely to be seen/experienced by users.

Comment 14

[Kai Engert [:KaiE:]]

Comment on attachment 9496529 [details]
Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). r=edicharry

EDIT:

I've removed the empty approval comment details, see above instead.

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9496523 [details]
Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). TB 140 patch. r=edicharry

[Triage Comment]
Approved for esr140

Comment 16

[Wayne Mery (:wsmwk)]

Comment on attachment 9496529 [details]
Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). r=edicharry

[Triage Comment]
Approved for release, if this is landed after the merges. Or approved for beta if you want to land this before the merge.

Take your pick, as long as we meet the goal of getting this to be in 140-everything.

Comment 17

[Corey Bryant]

Thunderbird 141.0b1:
https://hg.mozilla.org/releases/comm-beta/rev/43ae27603617

Comment 18

[Corey Bryant]

Comment on attachment 9496523 [details]
Bug 1961106 - Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). TB 140 patch. r=edicharry

[Triage Comment]
Approved for release

Comment 19

[Corey Bryant]

Thunderbird 140.0:
https://hg.mozilla.org/releases/comm-release/rev/a99d8c33d1d4

Comment 20

[Corey Bryant]

Thunderbird 140.0esr:
https://hg-edge.mozilla.org/releases/comm-esr140/rev/346a82877e76942dd6bc78cff06fb8d8fbd31bf7

Comment 21

[Kai Engert [:KaiE:]]

I had assumed we'd back the regressor patch from comm-central, too.

I realize this hasn't happened yet. I guess it wasn't done immediately, because we had to fix while trees were closed using 140 branching/release preparations.

Now, with the patch, still present in c-c, Magnus has requested a remedy patch on top of the regressor patch in bug 1971472.

I had originally thought we'd back out everywhere, and then do a full patch, as that would allow me to cleanly review/comment on all aspects of the patch.

Comment 22

[Kai Engert [:KaiE:]]

Please land revision D254842 into comm-central to get us into the same state as on the 140/141 branches, and then we can work from there.

Comment 23

[Kai Engert [:KaiE:]]

Please also backout from the new beta 142 branch as soon as it's possible.

Comment 24

[Kai Engert [:KaiE:]]

(In reply to Kai Engert [:KaiE:] from comment #23)

Please also backout from the new beta 142 branch as soon as it's possible.

Comment was wrong.
141 beta already has the backout.

142 is comm-central.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/360eac600146
Partially backed out changeset 0832b219daf1 (not restoring previously removed strings). r=edicharry

Comment 26

[Magnus Melin [:mkmelin]]

This bug is quite a mess now. It should never have been reopened, and the backouts should have happened in the relevant bug found.
I'm going to clone this bug to start anew, as all the statuses are now wrong.