Closed Bug 1675325 Opened 3 months ago Closed 2 months ago

Wait for checkDatabaseStructure() to finish before proceeding with initialization of OpenPGP

Categories

(MailNews Core :: Security: OpenPGP, defect)

defect

Tracking

(thunderbird_esr78+ affected, thunderbird83 affected)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 + affected
thunderbird83 --- affected

People

(Reporter: lasana, Assigned: lasana)

References

Details

(Whiteboard: [TM 78.7])

Attachments

(1 file)

The checkDatabaseStructure() functions called on the lines below are async, currently we are don't wait for them to complete before proceeding with intialization.

https://searchfox.org/comm-central/source/mail/extensions/openpgp/content/modules/core.jsm#121

This is getting in the way of some tests I'm working on, the operation is not finished before I add private keys and I get errors like:

(new Error("Error(s) encountered during statement execution: no such table: acceptance_decision", "resource://gre/modules/Sqlite.jsm", 894))

Attached patch bug1675325.patchSplinter Review

Implements the needed changes.

Attachment #9185768 - Flags: review?(kaie)
Status: NEW → ASSIGNED
Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

>     // trigger service init
>-    getEnigmailCore().getService();
>+    await getEnigmailCore().getService();

Could this evaluate to "await null" ?
Is that allowed?


>+   * @returns {Promise<Enigmail|null>}

I wonder if this isn't rather:
 @returns {Promise<Enigmail>|null}

If I'm right, and this returns null, and we must avoid "await null" in the caller:
Should we avoid returning a promise, and use 

    try {
      this.createInstance();
-      return gEnigmailService.getService(win, startingPreferences);
+      let svc = await gEnigmailService.getService(win, startingPreferences);
+      return svc;

    } catch (ex) {
      return null;
    }

(In reply to Kai Engert (:KaiE:) from comment #2)

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

// trigger service init
  • getEnigmailCore().getService();
  • await getEnigmailCore().getService();

Declaring the function async means any non-Promise return value will be wrapped in a promise.

More info here:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function#Description

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

Thanks for the clarification!

Attachment #9185768 - Flags: review?(kaie) → review+
Target Milestone: --- → 84 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/7f0cb454b777
Wait for checkDatabaseStructure() to complete before continuing OpenPGP initialization. r=kaie

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

[Approval Request Comment]
Correctness fix. Likely to fix test failure, now and going further.

Attachment #9185768 - Flags: approval-comm-esr78?

I find it difficult to say if this patch is safe for esr78 without having done more testing, because OpenPGP init logic changed on comm-central, but not on esr78.

(In reply to Kai Engert (:KaiE:) from comment #7)

I find it difficult to say if this patch is safe for esr78 without having done more testing, because OpenPGP init logic changed on comm-central, but not on esr78.

But it has gone through a full beta, no?

Do we have any telemetry yet of how many are using encryption on beta and on esr?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(kaie)

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

But it has gone through a full beta, no?

That doesn't matter. It's code that may depend on, or indirectly interact with, other code which is on comm-central and beta, only.

Flags: needinfo?(kaie)

(In reply to Kai Engert (:KaiE:) from comment #9)

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

But it has gone through a full beta, no?

That doesn't matter. It's code that may depend on, or indirectly interact with, other code which is on comm-central and beta, only.

See bug 1677508 comment 25 for the explanation.

We need explicit (manual) testing that this patch works on comm-esr78.
Knowing it works on comm-central doesn't help.

There were several related commits, but the following one is the one that worries me most:
https://hg.mozilla.org/comm-central/rev/f97d0a2f0cc65c58b844c85046f149d82197724f

When bug 1677508 was landed on comm-esr78, it didn't even build. That's because the comm-central patches to add these tests obviously depend on changes made to comm-central, at least the above commit from bug 1676887 - and there were several other changes that Mark Banner made around the same time.

So, before you consider to land the tests again, please, give it explicit interactive testing that Thunderbird works fine.

Flags: needinfo?(lasana)

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

[Triage Comment]
Sounds like doing this for 78.6.0 would be rushing, and we should wait until 78.6.1 or 78.7.

Attachment #9185768 - Flags: approval-comm-esr78? → approval-comm-esr78-

The next point release would be fine. I do suspect this fixes subtle bugs in addition to tests. Kai, if you're working on the 78 branch, please include it in your tree.

Flags: needinfo?(mkmelin+mozilla)
Whiteboard: [TM 78.7]

Actually, this should have been uplifted before the others. The same bug exists in 78, we are using async functions but not awaiting them.

https://searchfox.org/comm-esr78/source/mail/extensions/openpgp/content/modules/sqliteDb.jsm#35

https://searchfox.org/comm-esr78/source/mail/extensions/openpgp/content/modules/core.jsm#118

Flags: needinfo?(lasana)
Blocks: 1677508

(In reply to Magnus Melin [:mkmelin] from comment #12)

if you're working on the 78 branch, please include it in your tree.

I did, and I haven't noticed any problems yet.

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: none
Testing completed (on c-c, etc.): by running a local build with this patch
Risk to taking this patch (and alternatives if risky): low risk

Attachment #9185768 - Flags: approval-comm-esr78- → approval-comm-esr78?

Comment on attachment 9185768 [details] [diff] [review]
bug1675325.patch

[Triage Comment]
Approved for esr78

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