Skip to content

feat(immutable-arraybuffer): sliceToImmutable Hermes ponyfill and shim#2785

Merged
leotm merged 2 commits into
masterfrom
markm-to-immutable-hermes-only
May 15, 2025
Merged

feat(immutable-arraybuffer): sliceToImmutable Hermes ponyfill and shim#2785
leotm merged 2 commits into
masterfrom
markm-to-immutable-hermes-only

Conversation

@erights

@erights erights commented Apr 30, 2025

Copy link
Copy Markdown
Contributor

Refs: (#2399) #2400 #2798

Description

Rather than excluding the '@endo/immutable-arraybuffer/shim.js' import in the Hermes SES shim
(since Hermes lacks structuredClone, transfer, private fields and class syntax),
here we introduce '@endo/immutable-arraybuffer/shim-hermes.js' - a limited version achieved without those.

TODO

  • add limited hermes pony
  • add limited hermes pony shim
  • update get anonymous intrinsics
  • add/update types
  • add/update tests
  • improve types
  • add transform

Security Considerations

Does this change introduce new assumptions or dependencies that, if violated, could introduce security vulnerabilities? How does this PR change the boundaries between mutually-suspicious components? What new authorities are introduced by this change, perhaps by new API calls?

Documented in source code

Scaling Considerations

Does this change require or encourage significant increase in consumption of CPU cycles, RAM, on-chain storage, message exchanges, or other scarce resources? If so, can that be prevented or mitigated?

Documentation Considerations

Give our docs folks some hints about what needs to be described to downstream users. Backwards compatibility: what happens to existing data or deployments when this code is shipped? Do we need to instruct users to do something to upgrade their saved data? If there is no upgrade path possible, how bad will that be for users?

Documented in source code

Testing Considerations

Every PR should of course come with tests of its own functionality. What additional tests are still needed beyond those unit tests? How does this affect CI, other test automation, or the testnet?

Separate tests added for Hermes

  • pony
  • shim

Compatibility Considerations

Does this change break any prior usage patterns? Does this change allow usage patterns to evolve?

See #2785 (comment) RE expanding the transform to support more than side effect imports if one day required

Upgrade Considerations

What aspects of this PR are relevant to upgrading live production systems, and how should they be addressed?

Include *BREAKING*: in the commit message with migration instructions for any breaking change.

Update NEWS.md for user-facing changes.

Delete guidance from pull request description before merge (including this!)

SES NEWS.md bullet already added for next release

@erights erights self-assigned this Apr 30, 2025
@erights erights changed the title Markm to immutable hermes only experiment-only(immutable-arraybuffer)! experimental hermes-only Apr 30, 2025
@leotm leotm force-pushed the markm-to-immutable-hermes-only branch 3 times, most recently from 438d05e to a69fb24 Compare May 6, 2025 19:20
Comment thread packages/immutable-arraybuffer/src/limited-pony-for-hermes.js
Comment thread packages/immutable-arraybuffer/src/limited-pony-for-hermes.js Outdated
...asyncArrowEliminator,
...asyncGeneratorDestroyer,
...removeImmutableArrayBufferShim,
...immutableArrayBufferPonyfier,

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.

we're not able to replace import '@endo/immutable-arraybuffer/shim.js'; in ses/lockdown.js with e.g.

const ab = new ArrayBuffer(0);
if (typeof ab.transferToImmutable === 'function') {
  import('@endo/immutable-arraybuffer/shim.js');
} else {
  import('@endo/immutable-arraybuffer/shim-hermes.js');
}

so this transform is needed

@erights erights May 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kriskowal , I'm fine with the ponyfier. But do you now have some module magic that lets us conditionally synchronously import one of the other depending on platforms? Are we doing something similar to adapt to XS?

@kriskowal you're also the right person to ask: should it be "ponyfier" or "ponifier". FWIW, I prefer it as is, with the "y".

@leotm leotm May 14, 2025

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.

module magic that lets us conditionally synchronously import one of the other depending on platforms? Are we doing something similar to adapt to XS?

happy to follow-up and replace this transform with it if we do ^ or if we're doing something similar with XS

@leotm leotm marked this pull request as ready for review May 8, 2025 21:11
@leotm leotm self-requested a review May 8, 2025 21:12
@leotm

leotm commented May 8, 2025

Copy link
Copy Markdown
Contributor

@erights ready on my side, if you have any further feedback

edit: all discussions addressed

erights added a commit that referenced this pull request May 9, 2025

@erights erights left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See #2798 as an additional set of review suggestions on this PR expressed as a differential PR. Please review these suggestions in deciding what to do with them.

Comment thread packages/immutable-arraybuffer/package.json
Comment thread packages/ses/NEWS.md
Comment thread packages/ses/scripts/hermes-transforms.js
...asyncArrowEliminator,
...asyncGeneratorDestroyer,
...removeImmutableArrayBufferShim,
...immutableArrayBufferPonyfier,

@erights erights May 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@kriskowal , I'm fine with the ponyfier. But do you now have some module magic that lets us conditionally synchronously import one of the other depending on platforms? Are we doing something similar to adapt to XS?

@kriskowal you're also the right person to ask: should it be "ponyfier" or "ponifier". FWIW, I prefer it as is, with the "y".

Comment thread packages/immutable-arraybuffer/src/limited-pony-for-hermes.js Outdated
@erights

erights commented May 9, 2025

Copy link
Copy Markdown
Contributor Author

Please also fill out the PR form at the top before merging.

@leotm leotm marked this pull request as draft May 14, 2025 14:42
@leotm leotm marked this pull request as ready for review May 15, 2025 15:44
@leotm leotm force-pushed the markm-to-immutable-hermes-only branch from f525105 to 316ea46 Compare May 15, 2025 15:44
@leotm leotm changed the title experiment-only(immutable-arraybuffer)! experimental hermes-only feat(immutable-arraybuffer): sliceToImmutable Hermes ponyfill and shim May 15, 2025
leotm and others added 2 commits May 15, 2025 17:18
Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>
Co-authored-by: Mark S. Miller <erights@users.noreply.github.com>
@leotm leotm force-pushed the markm-to-immutable-hermes-only branch from 316ea46 to d81dca0 Compare May 15, 2025 16:18

@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.

LGTM, all comments addressed

rebased into 2 commits in style of #2399 and #2400

#2798 differential changes reviewed separately and included here, thank you again

@leotm leotm merged commit e432b14 into master May 15, 2025
16 checks passed
@leotm leotm deleted the markm-to-immutable-hermes-only branch May 15, 2025 16:38
kriskowal added a commit that referenced this pull request Jun 2, 2025
# `ses` v1.13.0

- Two new `stackFiltering:` options are added
- `'omit-frames'` -- Only omit likely uninteresting frames. Keep
original paths.
- `'shorten-paths'` -- Only shorten paths to text likely clickable in an
IDE

This fills out the matrix of what should have been orthogonal options.
The existing `'concise'` setting both omits likely uninteresting frames
and shortens their paths. The existing `'verbose'` setting does neither.

- Uses the `@endo/immutable-arraybuffer` shim to add
`ArrayBuffer.p.immutable`, `ArrayBuffer.p.transferToImmutable`, and
`ArrayBuffer.p.sliceToImmutable` to ses, in order to emulate the
[Immutable ArrayBuffer
proposal](https://github.com/tc39/proposal-immutable-arraybuffer). These
make an ArrayBuffer-like object whose contents cannot be mutated.
However, due to limitations of the shim
- Unlike `ArrayBuffer` and `SharedArrayBuffer` this shim's
ArrayBuffer-like object cannot be transfered or cloned between JS
threads.
- Unlike `ArrayBuffer` and `SharedArrayBuffer`, this shim's
ArrayBuffer-like object cannot be used as the backing store of
TypeArrays or DataViews.
- The shim depends on the platform providing either `structuredClone` or
`Array.prototype.transfer`. Node <= 16 and provides neither, causing the
shim to fail to initialize, and therefore SES to fail to initialize on
such platforms.
- Current Hermes has even stronger constraints, lacking
`structuredClone`, `transfer`, private fields, and even `class` syntax.
This requires other coping strategies. See
#2785
- Even after the upcoming `transferToImmutable` proposal is implemented
by the platform, the current code will still replace it with the shim
implementation, in accord with shim best practices. See
#2311 (comment) . It
will require a later manual step to delete the shim or have it avoid
overriting a platform implementation, after manual analysis of the
compat implications.

- The
[evalTaming](https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md#evaltaming-options)
option `'safe-eval'` now can only throw error `SES_DIRECT_EVAL`. This
allows SES to initialize with `'unsafe-eval'` or `'no-eval'` on hosts
with no direct eval available such as Hermes for a successful lockdown
that tolerates it's language
[features](https://github.com/facebook/hermes/blob/main/doc/Features.md).

The module name `ses/hermes` can now be required to call `lockdown` and
`repairIntrinsics` only, `Compartment` is not yet available.

It is currently compatible with Hermes
[v0.12.0](https://www.npmjs.com/package/hermes-engine-cli/v/0.12.0), we
plan to support
[v0.13.0](https://github.com/facebook/hermes/releases/tag/v0.13.0) then
subsequent Hermes [tags](https://github.com/facebook/hermes/tags) or
side-by-side versions built for React Native depending on ecosystem
usage and official
[support](https://github.com/reactwg/react-native-releases/blob/main/docs/support.md),
then Static Hermes when released.

Also `ses/hermes` can now be hooked into bundlers such as Metro to run
Hardened JS.

# `@endo/compartment-mapper` v1.6.1

- The `dev` flag for `mapNodeModules()` is no longer deprecated. The
concept of a "condition" ([conditional
exports](https://nodejs.org/api/packages.html#conditional-exports)) is
disinct from the flag's original meaning (instructs `mapNodeModules()`
to consider `devDependencies` when graphing packages). Users who have
switched to using a `development` condition for `dev`'s purpose are
encouraged to _switch back_ to using the `dev` flag instead. **In a
future release, the presence of a `development` condition will no longer
mimic an enabled `dev` flag** and will only be considered when
evaluating conditional exports.

# `@endo/evasive-transform` v2.0.0

- The `sourceType` option is now restricted to `script` and `module`
only. Function signature types have changed to be more precise.

# `@endo/bundle-source` v4.1.0

- The `'endoZipBase64'` moduleFormat now utilizes the `importHook`
option to exit dependencies whose specifiers return a truthy value.

# `@endo/import-bundle` v1.5.0

- The `'endoZipBase64'` moduleFormat now utilizes the `importHook`
option.
erights added a commit that referenced this pull request Jun 11, 2025
Closes: #XXXX
Refs: #2785

## Description

The Immutable ArrayBuffer shim, on non-Hermes, relies on the existence
of *either* a global `structuredClone` or `Array.prototype.transfer`.
However, I forgot to capture early the global `structuredClone` into a
module-local one, and to then use that repeatedly. MetaMask scuttling
deleted the global `structuredClone`, running headlong into my mistake
on platforms (Node 18 and Node 20) that do not also have
`Array.prototype.transfer`.

This PR fixes that mistake, capturing `structuredClone` early, and then
using the resulting module-local one.

### Security Considerations

The bug this PR fixes was a security hazard.

### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

In theory, we should add scuttling as an orthogonal dimension of our
platform testing matrix. However that is rather expensive for minor
benefit.

### Compatibility Considerations

The bug created compat problems that this PR fixes.
### Upgrade Considerations

none
erights added a commit that referenced this pull request Jun 18, 2025
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

- [x] Update `NEWS.md` for user-facing changes.

---------

Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants