feat(non-trapping-shim): opt-in shim of the non-trapping integrity trait

[erights]

Staged on #2704

Closes: #XXXX
Refs: https://github.com/tc39/proposal-stabilize #1756 #1912 #2675

Description

An opt-in shim for the non-trapping integrity level of https://github.com/tc39/proposal-stabilize

This does the non-trapping trait by itself only implying frozen, since we cannot yet ponyfill or shim "fix" or "permit-overrides". However, non-trapping would actually need to imply all other integrity levels, since the methods for querying or enabling these integrity levels would no longer trap once an object is non-trapping.

The names isNonTrapping and suppressTrapping are of course placeholders for whatever real names we decide on. But since non-trapping must imply all others, the name chosen should be appropriate for this maximal explicit integrity level. Currently, https://github.com/tc39/proposal-stabilize suggests that should be isStable and stabilize. If we revise this PR in that direction, we should also rename the package to stabilize-shim. A reason not to yet, is our inability to shim "fix" and "permit-overrides", which would be implied.

Updates

OTOH, as explained at tc39/proposal-stabilize#5, "fix" may be tied instead to instead to the existing non-extensible integrity trait, in which case it would not need its own trait. As explained at tc39/proposal-stabilize#4 "permit-overrides" might instead be fixed globally for the whole language, rather than being based on an integrity level. This would leave nonTrapping as the only case currently calling for a new integrity trait.

The shim is not opt-in via the env-option SES_NON_TRAPPING_SHIM as found on process.env, usually reflecting an environment variable of the same name. The two settings are disabled and enabled, with disabled being the default if absent. If this env-option is disabled, then importing the shim does nothing. Any tests that care should set this env-option explicitly, so that the test behavior does not depend on the environment variables in the testing environment.

Security Considerations

The point. The non-trapping integrity level enables practical programming patterns to protect themselves from reentrancy hazards, without introducing a check for whether something is a proxy. Thus, it does not damage practical membrane transparency.

Scaling Considerations

There is a hidden WeakMap and WeakSet involved. As with passStyleOf, use by agoric-sdk will need to arrange to build this package once with the real WeakMap and WeakSet, and to deeply endow bindings of Reflect, Object, and Proxy together. Otherwise, we recapitulate the scaling problems that passStyleOf had suffered from.

Update This PR now exports only the shim, not the ponyfill, because the eval-twin problems with use of the ponyfill are not practically solvable. Thus, there is no need for the deep endowment arrangement we need for passStyleOf. However, the core scaling consider remains: This shim implementation depends on hidden weak collections, whose expense may be surprising.

Documentation Considerations

The differences introduced by this PR should largely be covered by documentation for https://github.com/tc39/proposal-stabilize , which will normally only be of concern to the expert low-level programmers of our platforms -- both endo and agoric-sdk. For other programmers, this should only relieve us of the need to explain how to defend against reentrancy hazards that this PR makes go away.

Update

Now that this shim is opt-in via the env-option currently spelled SES_NON_TRAPPING_SHIM, we normally would update https://github.com/Agoric/agoric-sdk/blob/master/docs/env.md to include it. But to prevent churn, we should probably wait until the name bikeshed settles down.

Testing Considerations

Since this is a shim for a proposal, it should eventually be covered by test262 tests. The testing of the new integrity level should be at least as exhaustive as the test262 testing of other integrity levels.

Update

Now that this shim is opt-in via the env-option currently spelled SES_NON_TRAPPING_SHIM, we should arrange for many dependent tests to be run with this set each way, to make sure that they work whether opted into or out of.

Compatibility Considerations

To make this SES compatible, we need to extend permits.js to permit the four new methods. To use this to make SES safer, we will need to modify harden so that it makes all hardened objects also be non-trapping.

passStyleOf would need to check that copy-data (copyList, copyRecord, tagged) are non-trapping rather than just frozen.

At the agoric-sdk level, all virtual and durable object representatives are already hardened, so the previous changes will also make all these representatives non-trapping. Thus, we don't need any virtual or durable state to keep track of whether a virtual or durable object is non-trapping. Nor do we lose the bookkeeping when a new representative is created, even though this shim should be made by liveslots only with the real WeakMap and WeakSet, which is not virtual/durable-aware.

Separately,
The original Proxy is both constructible (i.e., responsive to new) and
lacks a prototype property. The closest we can come to this is to set ProxyPlus.prototype to undefined.
SES should then explicitly permit this extra prototype property with an undefined value, so we don't get unnecessary warning spam for a case we will now expect.

Update

We made the shim opt-in because of compat concerns when code using or assuming this shim is linked with code that does not. By defaulting to 'disabled', we ensure that no compat problems are caused be default.

Upgrade Considerations

In theory, old code could use Proxy in ways that this PR would break. But this is unlikely in general, and highly unlikely for any code in or depending on endo. Thus, practically, none.

Update

Upgrading from code prior to this shim to code using or assuming this shim should not cause any upgrade issues, since the shim is default 'disabled'. However, this postpones the upgrade question to that time when a vat opts in. As long as that opt-in is sound at that time because none of the code it is linked with depends on the absence of disabling of this shim, then the upgrade should work. Whether this shim is absent, disabled, or enabled should make no difference to the representation of durable state. Representatives are always hardened. In a vat that has opted in, these representatives will also be non-trapping.

[mhofman]

I've only skimmed through quickly, but from what I can tell this is following closely (but not fully) the letter of the proposed spec: that no state is held on the proxy and checks are instead made on the shadow target. However I have a feeling that we can prove an equivalent approach would be to store state on proxy objects only, since by definition ordinary objects can't be trapping. This should reduce the cost of side tables necessary for the implementation.

[erights]

@kriskowal , at https://github.com/endojs/endo/actions/runs/12563852500/job/35026252842?pr=2673 I am getting lint errors that seem clearly unrelated to this PR. Any idea what happened?

Update: See #2676 (comment) , which establishes that these lint errors are somehow dependent on this PR.

Update: The only thing I could think of was the devDepencies from this PR to ses-level stuff. So I removed those, and checked that the resulting yarn.lock entry for no-trapping-shim is like that for immutable-arraybuffer. However, I'm still getting those lint errors, and so am completely flummoxed. Help!

[info] Converting project at ./packages/lockdown
packages/marshal/src/encodeToCapData.js:190:9 - error TS2322: Type '{ [k: string]: string | number | boolean | EncodingClass<"NaN"> | EncodingClass<"undefined"> | EncodingClass<"Infinity"> | EncodingClass<"-Infinity"> | ... 9 more ... | null; }' is not assignable to type 'Encoding'.
  Type '{ [k: string]: string | number | boolean | EncodingClass<"NaN"> | EncodingClass<"undefined"> | EncodingClass<"Infinity"> | EncodingClass<"-Infinity"> | ... 9 more ... | null; }' is not assignable to type 'EncodingClass<"error"> & { name: string; message: string; errorId?: string | undefined; cause?: Encoding | undefined; errors?: Encoding[] | undefined; }'.
    Property ''@qclass'' is missing in type '{ [k: string]: string | number | boolean | EncodingClass<"NaN"> | EncodingClass<"undefined"> | EncodingClass<"Infinity"> | EncodingClass<"-Infinity"> | ... 9 more ... | null; }' but required in type 'EncodingClass<"error">'.

190         return fromEntries(
            ~~~~~~

  packages/marshal/src/types.js:25:16
    25  * @typedef {{ '@qclass': T }} EncodingClass
                      ~~~~~~~~~
    ''@qclass'' is declared here.

packages/pass-style/src/symbol.js:14:7 - error TS2578: Unused '@ts-expect-error' directive.

14       // @ts-expect-error It doesn't know name cannot be a symbol
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

packages/ses/src/error/assert.js:308:5 - error TS25[78](https://github.com/endojs/endo/actions/runs/12564819013/job/35028359869?pr=2673#step:8:79): Unused '@ts-expect-error' directive.

308     // @ts-expect-error TS still confused by symbols as property names
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

packages/ses/src/make-hardener.js:241:21 - error TS2578: Unused '@ts-expect-error' directive.

241                     // @ts-expect-error TS should know FERAL_STACK_GETTER
                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

---

Update workedaround in
7ab2643

---

Update

now filed as #2698

[erights]

@mhofman , all your suggestions addressed. PTAL. Thanks.

[mhofman]

I have a feeling that we can prove an equivalent approach would be to store state on proxy objects only, since by definition ordinary objects can't be trapping.

It was obviously too late for me to form a proper intuition there. The non-trapping trait is not proxy specific, but actually a regular object's trait about any proxy that may target them. Otherwise we would have a predicate that can ask "is this a proxy object behaving exotically". By defining the trait on objects in general, we maintain the transparency of proxies.

As such, I was mistaken, and I don't think it's possible to only store the trait exclusively on proxy object.

[mhofman]

I've only reviewed the implementation so far. Looks good overall, except I don't understand this shouldThrow condition.

Also a potential optimization: when adding a proxy to the noTrappingSet, we could eagerly redefine all own properties of handlerPlus to undefined (including the originalHandler). To make that easier we just need to save handlerPlus in the proxyHandlerMap instead of the original handler. If we do this, the original handler would become unreferenced when the proxy is found to be non-trapping, giving the nice property that it can be collected (this would be true with a native spec implementation).

[erights]

In a verbal discussion, @gibson042 and I concluded that the eval twin problems with the ponyfill are fatal. So I just added a commit that stops exporting the ponyfill. Currently, we still have the ponyfill vs shim internally for modularity purposes. But the ponyfill should only be used for

• separate unit testing
• building the shim.

[erights]

In theory, old code could use Proxy in ways that this PR would break. But this is unlikely in general, and highly unlikely for any code in or depending on endo. Thus, practically, none.

As discussed in #2675 (review), this turns out to not be so simple.

Quite right. In discussion with @michaelfig , we arrived at the opt-in mechanism this PR now implements. I added updates to the PR comment to reflect this.