1659801 - Implement a CTypes readTypedArray() for CData and CDataFinalizer

Status: RESOLVED FIXED

(Core :: js-ctypes, enhancement, P3)

Description

[Steve Fink [:sfink] [:s:]]

Ok, I guess I got nerd-sniped with this one. It didn't seem too hard to also add a .readTypedArray() that is more semantically clean.

I think the .readStringLatin1() is probably still needed for the actual use case here, but this would probably be better for other uses (and for the original use to switch to eventually).

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Bug 1659801 - Implement a CTypes readTypedArray() for CData and CDataFinalizer

Comment 2

[Steve Fink [:sfink] [:s:]]

I need to fix up a few things before landing. The test added made me realize that most of our shell tests jobs in CI don't enable ctypes, so I need to make the tests conditional (as are all other tests in the jit-test/tests/ctypes/ directory.) But the one that does (SM(r) aka rootanalysis) is also failing due to some stupid timezone thing.

Comment 4

[Steve Fink [:sfink] [:s:]]

This patch subsumes readStringLatin1 from bug 1659508 because of the existence of TextDecoder:

  var decoder = new TextDecoder("latin1");
  var binaryString = decoder.decode(u8array.readTypedArray());

and note that this is NUL-safe, unlike readStringLatin1: if the array of uint8_t contains NUL bytes, then (of course) the resulting Uint8Array will contain them, but even better, TextDecoder("latin1") will preserve them rather than terminating the "string" early.

Note that binaryString here is still a strange beast: it is not a string at all, and doesn't necessarily represent valid string data in any encoding. But it is what you'd want and expect from binary data stored in a string.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3c5a6690fae
Implement a CTypes readTypedArray() for CData and CDataFinalizer r=jorendorff

Comment 6

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d3c5a6690fae

Comment 7

[Kai Engert (:KaiE:)]

This is great. Patch applies to comm-esr78 branch cleanly. I've tested it, it works and is fast. Thank you very much!!!

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 9170753 [details]
Bug 1659801 - Implement a CTypes readTypedArray() for CData and CDataFinalizer

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Needed to avoid slowness in Thunderbird. Shouldn't affect Firefox.

If you accept this patch, we could avoid maintaining a separate mozilla-esr78 branch for Thunderbird.

• User impact if declined: none to Firefox.
• Fix Landed on Version: 82
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): new separate code
• String or UUID changes made by this patch: none

Comment 9

[Kai Engert (:KaiE:)]

What are your opinions for uplifting this to the stable ESR mozilla-esr78 branch?

Comment 10

[Ted Campbell [:tcampbell]]

To add to Comment 8:

This patch adds a new javascript method for privileged JS that already had access to ctypes. The Firefox build is unaffected by the new method. Changes to code that could affect Firefox are a routine factoring-out of a code into a helper method. Overall this is extremely low risk to Firefox itself and solves a performance issue for Thunderbird.

I think uplift makes sense.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9170753 [details]
Bug 1659801 - Implement a CTypes readTypedArray() for CData and CDataFinalizer

NPOTB for Firefox but makes life easier for TB. Sounds pretty safe indeed :-). Approved for 78.3esr.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/d928031b7f17