Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: tame Error constructor #359

Merged
merged 1 commit into from
Jul 1, 2020
Merged

fix: tame Error constructor #359

merged 1 commit into from
Jul 1, 2020

Conversation

erights
Copy link
Contributor

@erights erights commented Jun 27, 2020

An initial taming of the Error constructor that should both be safe enough and compat enough for cases we've seen.

Build on the partial-intrinsic-reform branch, which set the stage.

Current a Draft PR because none of this is tested yet.

@warner
Copy link
Contributor

warner commented Jun 29, 2020

I don't know if this is supposed to work or not, but this branch (at commit a04148a) did not allow a prepareStackTrace/stackTraceLimit -using library to work. My test program is:

import 'ses';
import callsites from 'callsites';
lockdown();
console.log(callsites());

callsites uses Error.prepareStackTrace. I don't know that we need that particular library to work, but it exercises a pattern similar to what we've identified in libraries that we do need, and it's currently present in the dependency graph of both agoric-sdk and SES-shim. You should be able to run that test program from any directory of the SES-shim tree.

If I comment out the lockdown(), the program prints a bunch of empty Callsite objects. With lockdown(), it throws a TypeError because Error is not extensible.

@erights erights changed the base branch from partial-intrinsic-reform to master June 30, 2020 21:30
@erights erights marked this pull request as ready for review June 30, 2020 21:38
@erights erights requested a review from warner June 30, 2020 21:40
Copy link
Contributor

@warner warner left a comment

Choose a reason for hiding this comment

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

I think the base use case would work, so this would be a step forward for compatibility. The "safe" mode seems fine to me, and the "unsafe" mode can't get any more unsafe :). So I'd say go ahead and land this, but let's make sure we figure out the non-v8 and nesting cases before we call this done.

packages/ses/test/tame-error.test.js Show resolved Hide resolved
packages/ses/test/error-manipulation.test.js Show resolved Hide resolved
packages/ses/src/tame-global-error-object.js Outdated Show resolved Hide resolved
@erights erights requested a review from warner July 1, 2020 05:04
@erights
Copy link
Contributor Author

erights commented Jul 1, 2020

@warner to address the infinite recursion, enough changed that I re-requested a review. PTAL

Copy link
Contributor

@warner warner left a comment

Choose a reason for hiding this comment

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

Looks great. Maybe add one comment, and take a brief look at my captureStackTrace comment, but otherwise it looks ready for landing to me.

packages/ses/src/tame-global-error-object.js Show resolved Hide resolved
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.

2 participants