Skip to content

fix(immutable-arraybuffer): unify shim to work on more platforms#2855

Merged
erights merged 9 commits into
masterfrom
markm-unify-immutable-arraybuffer-shims
Jun 18, 2025
Merged

fix(immutable-arraybuffer): unify shim to work on more platforms#2855
erights merged 9 commits into
masterfrom
markm-unify-immutable-arraybuffer-shims

Conversation

@erights

@erights erights commented Jun 12, 2025

Copy link
Copy Markdown
Contributor

Closes: #XXXX
Refs: #2785 #2399

Description

The Immutable ArrayBuffers shim can only emulate transferToImmutable on platforms with either structuredClone or ArrayBuffer.prototype.transfer. Otherwise, the shim would refuse to initialize. Without Immutable ArrayBuffers, we cannot initialize ses, and pass-style cannot implement the new byteArray Passable type, which marshal and patterns now depends on.

Unfortunately, it turns out that some platforms we want to support do not have structuredClone or ArrayBuffer.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 supporting sliceToImmutable instead, 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 structuredClone or ArrayBuffer.prototype.transfer. So this PR unifies the two shim strategies into one which uses the WeakMap technique, which works everywhere, and emulating sliceToImmutable, which we can do everywhere. It still sniffs for structuredClone or ArrayBuffer.prototype.transfer, and only emulates transferToImmutable if one is present. Thus, we sniff only for features not platforms, and emulate sliceToImmutable on all platforms.

Other PRs (TODO: which ones?) already ensure that pass-style, marshal, and patterns are fine with the absence of transferToImmutable.

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

  • redundancy, which needs to be removed
  • contains unconditional transferToImmutable tests that won't work on some platforms. These specific tests need to be made conditional, probably using the same test.skip trick some tests use for Array.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

  • Update NEWS.md for user-facing changes.

@erights erights self-assigned this Jun 12, 2025
@erights erights changed the base branch from master to markm-better-byteArrays June 12, 2025 02:14
@erights erights changed the title Markm unify immutable arraybuffer shims fix(immutable-arraybuffer): unify shim to work on more platforms Jun 12, 2025
@erights erights marked this pull request as ready for review June 12, 2025 02:49
@erights erights requested a review from kumavis June 12, 2025 02:50
Base automatically changed from markm-better-byteArrays to master June 12, 2025 04:52
@erights erights force-pushed the markm-unify-immutable-arraybuffer-shims branch from ae0eb5f to 82bc495 Compare June 12, 2025 04:54
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 leotm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested working (React Native runtime) on both legacy RN JSC (ses.cjs) and Hermes (ses-hermes.cjs) ^ better unified approach than my prev initial thoughts of introducing another flavour SES shim, less maintenance and 1 Hermes transform fewer

@gibson042 gibson042 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js Outdated

@boneskull boneskull left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am too ignorant to approve this. I will, however, test it with LavaMoat once it's shipped. 😄

Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js
@erights erights force-pushed the markm-unify-immutable-arraybuffer-shims branch from 3eb6caf to 7d72eb6 Compare June 13, 2025 02:15
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js
@erights erights force-pushed the markm-unify-immutable-arraybuffer-shims branch from 34c0fdf to eebf99c Compare June 14, 2025 20:53
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 kriskowal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With notes.

Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js
Comment thread packages/immutable-arraybuffer/package.json
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js Outdated
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-shim.js Outdated
@erights erights force-pushed the markm-unify-immutable-arraybuffer-shims branch from eebf99c to 48199e2 Compare June 17, 2025 22:36
@kriskowal

Copy link
Copy Markdown
Member

Needs NEWS

diff --git a/packages/immutable-arraybuffer/NEWS.md b/packages/immutable-arraybuffer/NEWS.md
index d12ca09d3f..dc1a584999 100644
--- a/packages/immutable-arraybuffer/NEWS.md
+++ b/packages/immutable-arraybuffer/NEWS.md
@@ -7,6 +7,11 @@ User visible changes in `@endo/immutable-arraybuffer`:
   `@endo/immutable-arraybuffer/shim.js` does not interfere with this module's
   designed behavior.
 
+- Removes `@endo/immutable-arraybufer/shim-hermes.js` and absorbs the necessary
+  features into `@endo/immutable-arraybuffer/shim.js`.
+  We are not qualifying this as a breaking change since the feature did not
+  exist long enough to become relied upon.
+
 # 1.0.0 (2025-05-07)
 
 First release.

@erights

erights commented Jun 17, 2025

Copy link
Copy Markdown
Contributor Author

Removes @endo/immutable-arraybufer/shim-hermes.js and absorbs the necessary features into @endo/immutable-arraybuffer/shim.js. We are not qualifying this as a breaking change since the feature did not exist long enough to become relied upon.

Done, thanks.

@erights erights requested review from leotm and naugtur June 17, 2025 23:28
Comment thread packages/immutable-arraybuffer/src/immutable-arraybuffer-pony.js Outdated
@erights erights force-pushed the markm-unify-immutable-arraybuffer-shims branch from e29e224 to 1b0bc2a Compare June 18, 2025 04:29
@erights erights requested a review from kriskowal June 18, 2025 04:49
@erights erights merged commit 25039f5 into master Jun 18, 2025
16 checks passed
@erights erights deleted the markm-unify-immutable-arraybuffer-shims branch June 18, 2025 21:19
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants