1659443 - Use JS API readTypedArray() to fix slow OpenPGP decryption

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, defect)

Description

[Kai Engert (:KaiE:)]

Processing a 7 MB encrypted message takes 40 seconds on a fast computer.

(Timing is for a debug build, updated timing info in comment 3.)

Comment 1

[Kai Engert (:KaiE:)]

The slow part is this loop, which converts from a js-ctypes byte array to a JS string:
https://searchfox.org/comm-central/rev/38c93f8c99cfe10cff5cd1b0ce399ff2334c0003/mail/extensions/openpgp/content/modules/RNP.jsm#973

Is there a more efficient way to perform this conversion?

This is fast:

        let char_array = ctypes.cast(
          result_buf,
          ctypes.char.array(result_len.value).ptr
        ).contents;

        result.decryptedData = char_array.readString();

... but we cannot use it. If the decryptedData contains a message with 8-bit characters in an encoding other than UTF-8, then readString() fails. And using readStringReplaceMalformed() would result in dataloss.

Comment 2

[Kai Engert (:KaiE:)]

Help wanted from a js-ctypes expert.
Whom could we ask?

Comment 3

[Kai Engert (:KaiE:)]

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

Processing a 7 MB encrypted message takes 40 seconds on a fast computer.

Well, this was for a debug build.
With a fully optimized build, it's about 5 seconds for this loop.

Comment 4

[Magnus Melin [:mkmelin]]

Sorry, I don't know who would know about js-ctypes. Could ask in #developers?

Some thoughts:
Is that an Uint8Array, or what is the real type?
Could we use something like new TextDecoder("utf-8").decode(Uint8Array.from(uint8_array));

But then again, if it's not UTF-8, how will you know what to decode it to?

Comment 5

[Patrick Brunschwig]

The source of the problem is that it's not a UTF-8 string. That conversion is not allowed. You have to interpret it as binary 8-bit data stored in a string.

Comment 6

[Kai Engert (:KaiE:)]

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

Is that an Uint8Array, or what is the real type?

No, this is the type:

      let result_buf = new ctypes.uint8_t.ptr();

Comment 7

[Kai Engert (:KaiE:)]

Patrick, for MIME messages, if they are in non-UTF-8 encodings, do you think we might ever see NULL bytes?

Steve has thankfully worked on a new JS String API that might help us to obtain the data in an efficient way, but it would terminate at NULL.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: 1659443-v1.patch

This patch uses the suggested new API from bug 1659508.

It works for me and is fast.

Once bug 1659508 is reviewed, we could consider to start using a THUNDERBIRD_78_VERBRANCH of mozilla-esr78 and apply it locally. (It seems unlikely that Mozilla would accept the patch for mozilla-esr78, but we could ask, once ready.)

Comment 9

[Kai Engert (:KaiE:)]

Magnus, Wayne, any thoughts on starting a THUNDERBIRD_78_VERBRANCH for mozilla-esr78 ?

Comment 10

[Wayne Mery (:wsmwk)]

I have no objection. I defer to Magnus and Rob.

Comment 11

[Rob Lemley [:rjl]]

I'd like to discuss this further in a Zoom if possible and bring in Ryan V if since it's likely him that would be maintaining this branch.

Comment 12

[Patrick Brunschwig]

I think that NULL bytes are not allowed in MIME strings.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Rob Lemley [:rjl] from comment #11)

I'd like to discuss this further in a Zoom if possible and bring in Ryan V if since it's likely him that would be maintaining this branch.

What do you need to push this forward?

Comment 14

[Kai Engert (:KaiE:)]

I have tested the final code from bug 1659801, locally applied to mozilla-esr78.
It works fine.
I'll attach a phabricator patch that makes use of the feature.

The open question is, how can we take this patch for Thunderbird 78.x ?

I've asked for uplift of the new feature to the official mozilla-esr78 branch.
Should this request get rejected, then we cannot avoid our own THUNDERBIRD_78_VERBRANCH.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Bug 1659443 - Use js-ctypes readTypedArray() from bug 1659801 for faster decryption. r=mkmelin

Comment 16

[Kai Engert (:KaiE:)]

Thankfully the required JS API has been applied to mozilla-esr78.

Comment 17

[Kai Engert (:KaiE:)]

Attachment: 1659443.patch

same patch as latest rev in phab

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2de738db85be
Use js-ctypes readTypedArray() from bug 1659801 for faster decryption. r=mkmelin

Comment 19

[Kai Engert (:KaiE:)]

Magnus had requested to change another place, I had missed that request, sorry.
Will create a follow-up patch.

Comment 20

[Kai Engert (:KaiE:)]

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

Magnus had requested to change another place, I had missed that request, sorry.
Will create a follow-up patch.

Unfortunately it isn't working in the other place!
This isn't a blocker, I've filed it as bug 1663339.

Comment 21

[Kai Engert (:KaiE:)]

I see that 78.3.0 has been pinned to comm-esr78, this means we're ready to uplift this fix.

Comment 22

[Kai Engert (:KaiE:)]

Comment on attachment 9172493 [details]
Bug 1659443 - Use js-ctypes readTypedArray() from bug 1659801 for faster decryption. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): none
User impact if declined: much slowness decrypting big messages
Testing completed (on c-c, etc.): yes
Risk to taking this patch (and alternatives if risky): low, any problem would be immediately obvious

Comment 23

[Wayne Mery (:wsmwk)]

Comment on attachment 9172493 [details]
Bug 1659443 - Use js-ctypes readTypedArray() from bug 1659801 for faster decryption. r=mkmelin

[Triage Comment]
Approved for esr78 - if Rob can take it

Comment 24

[Rob Lemley [:rjl]]

Thunderbird 78.3.0:
https://hg.mozilla.org/releases/comm-esr78/rev/baa5e14319ff