fix(immutable-arraybuffer): unify shim to work on more platforms#2855
Merged
Conversation
ae0eb5f to
82bc495
Compare
leotm
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 12, 2025
- endojs/endo#2855 - iOS on RN JSC up and running again with SES - Android on Hermes working as expecting with SES
leotm
approved these changes
Jun 12, 2025
15 tasks
gibson042
reviewed
Jun 12, 2025
gibson042
left a comment
Member
There was a problem hiding this comment.
IIUC, this shim sits underneath SES and should therefore defend itself against post-hoc manipulation of primordials. Many of my suggestions relate to that, while others propose minor refactorings and comment text rewording.
19 tasks
boneskull
reviewed
Jun 12, 2025
boneskull
left a comment
Member
There was a problem hiding this comment.
I am too ignorant to approve this. I will, however, test it with LavaMoat once it's shipped. 😄
3eb6caf to
7d72eb6
Compare
kriskowal
reviewed
Jun 13, 2025
34c0fdf to
eebf99c
Compare
leotm
added a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jun 17, 2025
- endojs/endo#2855 - iOS on RN JSC up and running again with SES - Android on Hermes working as expecting with SES
kriskowal
approved these changes
Jun 17, 2025
eebf99c to
48199e2
Compare
Member
|
Needs NEWS |
Contributor
Author
Done, thanks. |
kriskowal
reviewed
Jun 18, 2025
Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
e29e224 to
1b0bc2a
Compare
kriskowal
approved these changes
Jun 18, 2025
github-merge-queue Bot
pushed a commit
to MetaMask/metamask-mobile
that referenced
this pull request
Jul 9, 2025
## **Description** Introduce Hardened JavaScript now on both iOS (RN JSC) and Android (Hermes) via Metro (@lavamoat/react-native-lockdown beta) instead of RN patch and remove [old iOS UI](https://github.com/user-attachments/assets/b53be562-bc36-4ff2-a177-ef5c24c44de4) from: Settings > Experimental > Security TODO - [x] Remove stale root SES shim (now via @lavamoat/react-native-lockdown) - [x] Remove stale RN iOS patch (now via @lavamoat/react-native-lockdown) - [x] Add temp @lavamoat/react-native-lockdown tgz - [x] Add temp SES patch endojs/endo#2855 - [x] Replace both with official @lavamoat/react-native-lockdown - [x] once LavaMoat/LavaMoat#1716 merged - [x] released LavaMoat/LavaMoat#1643 - [x] Remove experimental feature toggle UI - Ref: #8373 - preserve react-native-mmkv (we still use it) - update UI component snapshot - remove stale EN txt (and others?) - [x] <s>Fix smoke/regression e2e test timeouts</s> passing - [x] check failing regression e2e tests - same ones also failing on `main` - [x] cursor[bot] feedback ## **Related issues** Fixes: ## **Manual testing steps** ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: sethkfman <10342624+sethkfman@users.noreply.github.com>
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #XXXX
Refs: #2785 #2399
Description
The Immutable ArrayBuffers shim can only emulate
transferToImmutableon platforms with eitherstructuredCloneorArrayBuffer.prototype.transfer. Otherwise, the shim would refuse to initialize. Without Immutable ArrayBuffers, we cannot initializeses, andpass-stylecannot implement the newbyteArrayPassable type, whichmarshalandpatternsnow depends on.Unfortunately, it turns out that some platforms we want to support do not have
structuredCloneorArrayBuffer.prototype.transfer. The first we encountered was Hermes, that also does not have classes with private fields. #2785 makes all these work on Hermes, by supportingsliceToImmutableinstead, and using WeakMaps rather than private fields. On the theory that Hermes was a one-off, #2785 sniffed to see if it was on Hermes, only only then used the alternate shim.In turns out Hermes was not a one-off. Some version of JSC of concern (TODO which ones?) also have neither
structuredCloneorArrayBuffer.prototype.transfer. So this PR unifies the two shim strategies into one which uses the WeakMap technique, which works everywhere, and emulatingsliceToImmutable, which we can do everywhere. It still sniffs forstructuredCloneorArrayBuffer.prototype.transfer, and only emulatestransferToImmutableif one is present. Thus, we sniff only for features not platforms, and emulatesliceToImmutableon all platforms.Other PRs (TODO: which ones?) already ensure that
pass-style,marshal, andpatternsare fine with the absence oftransferToImmutable.Security Considerations
A singe piece of code with small feature detects is simpler, and thus more secure, than maintaining two alternate pieces of code, each for a different set of platforms.
Scaling Considerations
On most platforms, a class with private fields is probably more efficient than a WeakMap. This PR sacrifices that optimization in exchange for unification.
Documentation Considerations
Less to document.
Testing Considerations
The "unified" tests are just the union of the old tests, and so have two problems
transferToImmutabletests that won't work on some platforms. These specific tests need to be made conditional, probably using the sametest.skiptrick some tests use forArray.prototype.transfer.Compatibility Considerations
The goal is to make the shim compat with more platforms. On platforms that worked before this PR, this PR should be a pure refactor, and so without any compat hazards.
Upgrade Considerations
NEWS.mdfor user-facing changes.