bug(errors): Stack can't be set after importing `@ocap/shims/endoify`.

[grypez]

Repro: #168

Expected Behavior

The following test passes.

import '@ocap/shims/endoify';
import {
  marshalError,
  unmarshalError,
  VatAlreadyExistsError,
} from '@ocap/errors';
import { describe, it, expect } from 'vitest';

describe('marshal', () => {
  it('should round trip a thrown error', async () => {
    const thrown = new VatAlreadyExistsError('v123');
    const marshaled = marshalError(thrown);
    const received = unmarshalError(marshaled);

    expect(received).toStrictEqual(thrown);
  });
});

Observed Behavior

The test fails with a TypeError during the call to unmarshalError.

Root Cause

Unknown. The roundtrip test passes if the endoify shim is replaced with the harden mock from @ocap/test-utils/src/env/mock-endo.ts. The endoify shim has at least hundreds of lines dedicated to transforming the error prototype.

References

Endoify shim

From @ocap/shims/dist/endoify.js:2964:

/**
 * Make reasonable best efforts to make a `Passable` error.
 *   - `sanitizeError` will remove any "extraneous" own properties already added
 *     by the host,
 *     such as `fileName`,`lineNumber` on FireFox or `line` on Safari.
 *   - If any such "extraneous" properties were removed, `sanitizeError` will
 *     annotate
 *     the error with them, so they still appear on the causal console
 *     log output for diagnostic purposes, but not be otherwise visible.
 *   - `sanitizeError` will ensure that any expected properties already
 *     added by the host are data
 *     properties, converting accessor properties to data properties as needed,
 *     such as `stack` on v8 (Chrome, Brave, Edge?)
 *   - `sanitizeError` will freeze the error, preventing any correct engine from
 *     adding or
 *     altering any of the error's own properties `sanitizeError` is done.
 *
 * However, `sanitizeError` will not, for example, `harden`
 * (i.e., deeply freeze)
 * or ensure that the `cause` or `errors` property satisfy the `Passable`
 * constraints. The purpose of `sanitizeError` is only to protect against
 * mischief the host may have already added to the error as created,
 * not to ensure that the error is actually Passable. For that,
 * see `toPassableError` in `@endo/pass-style`.
 *
 * @param {Error} error
 */

The linked @endo/pass-style code mentions we may need to resolve this in @endo.

/**
 * ...
 * TODO Adopt a more flexible notion of passable error, in which
 * a passable error can contain other own data properties with
 * throwable values.
 * ...
 */

More References

• Node.js generates the stack lazily at access. This presents an obstruction to passing errors over the wire.
• MDN mentions error.prototype.stack is a nonstandard feature which Firefox implements as an accessor, but Chrome and Safari implement as a writable, nonenumerable, configurable property.
• Mentioned in agoric membranes talk that disallowing stack trace forging may be intended behavior.
• See tc39 proposal for markm's assessment.

Comments

I don't have all the dots connected, but from my initial examination it seems that setting error.stack might work in Chrome or Safari without endoify, but fail in Firefox; and that endoify will prevent us from doing this in any environment. The fix does not seem to be as simple as replacing return harden(output) with return toPassableError(output) in marshalError.ts, but it may be only slightly more complicated.

[sirtimid]

Perhaps we can bypass this error by passing the cause when instantiating the errors, before hardening. For simple errors it would be like new Error(marshaledError.message, { cause }) and for custom error classes we can pass it already

Update: My mind was stuck with cause when the issue is stack. We can implement a custom stack with the same logic for our custom classes.

[rekmarks]

This is certainly due to the fact that we harden custom error instances in their constructors. Like @sirtimid is pointing out, we have to pass the stack we want to the constructor, or create a new error object. This should not require any changes in endo.

One minor point: In your example @sirtimid, we should only need to pass { cause } to super(), without also assigning it to this.cause?

[sirtimid]

This is certainly due to the fact that we harden custom error instances in their constructors. Like @sirtimid is pointing out, we have to pass the stack we want to the constructor, or create a new error object. This should not require any changes in endo.

One minor point: In your example @sirtimid, we should only need to pass { cause } to super(), without also assigning it to this.cause?

@rekmarks Yes, you are right, I've made a mistake there, it should be something like this