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

feat(ses): Carry compartment names in error messages #431

Merged
merged 3 commits into from
Aug 26, 2020

Conversation

kriskowal
Copy link
Member

This change adds the name option to the Compartment constructor, a name accessor to the Compartment prototype, and annotates certain errors that pass through the Compartment with the name of the compartment they pass through.

@kriskowal kriskowal requested a review from erights August 25, 2020 00:41
@kriskowal kriskowal mentioned this pull request Aug 25, 2020
36 tasks
* No changes yet.
* Adds the `name` option to the `Compartment` constructor and `name` accessor
to the `Compartment` prototype.
Errors that pass through a compartment may be annotated with the compartment
Copy link
Contributor

Choose a reason for hiding this comment

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

From reading this description I was confused about what to expect.

  • These are only errors that happen during load. What about errors raised by user code during module initialization?
  • By "annotated" you do not mean that you mutate the error object in place. (Whew!) Rather you rethrow a new error object that is like the old one except for added text in the message string.

@@ -204,6 +209,7 @@ export const makeCompartmentConstructor = (intrinsics, nativeBrander) => {
function Compartment(endowments = {}, moduleMap = {}, options = {}) {
// Extract options, and shallow-clone transforms.
const {
name = '<unknown>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe <unnamed>?

moduleSpecifier,
).catch(error => {
const { name } = compartmentPrivateFields.get(compartment);
throw new Error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Given

https://github.com/Agoric/SES-shim/blob/cbe689e018b2786076d18c1883d75b00eb051fb4/packages/ses/src/tame-error-constructor.js#L9-L17

we should reproduce the error class as well

Suggested change
throw new Error(
const ErrorConstructor = NativeErrors[error.name] || Error;
throw new ErrorConstructor(

Rather than importing NativeErrors from tame-error-constructor.js, it should go in whitelist.js. But that does not need to happen in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given the recursion, is the intent that each level of recursion adds its own location info?

Copy link
Contributor

@erights erights Aug 25, 2020

Choose a reason for hiding this comment

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

The problem with reconstructing the error object is that you silently lose the call stack associated with the original error object. What assumptions are you making about ambient availability of a console object for this code? Nothing I've done for keeping track of error stack correspondence is close to ready. One thing this code can do in the meantime is to do something like

    const newError = NewErrorContructor(...);
    if (someAppropriateOption) {
      console.log(error, 'rethrown as', newError);
    }
    throw newError;

The normal console behavior is to emit the stacks associated with error objects it logs. This would at least give us the correspondences to grovel through to find the original error stack.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on your goals and your console assumptions, we could take the opposite approach:

    if (someAppropriateOption) {
      console.log(error, `, loading  ${q(moduleSpecifier)} in compartment ${q(name,)}`);
    }
    throw error;

This completely avoids recreating new error objects as well as modifying the thrown one in place. It rethrows the one we've got, which retains the stack trace it was constructed with. All the new compartment-name diagnostic info in only logged to the console.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it suffice to reüse the error.constructor?

@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from 3a12559 to 68f2852 Compare August 25, 2020 18:09
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch 2 times, most recently from 677a949 to e94fbe4 Compare August 25, 2020 18:54
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch 2 times, most recently from e140f2c to ec31d51 Compare August 25, 2020 19:03
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch from 128b630 to 5fb3910 Compare August 25, 2020 21:50
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch 2 times, most recently from 4f9b18a to cf22857 Compare August 25, 2020 21:54
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch 2 times, most recently from 9e9c09b to fd6c1b6 Compare August 25, 2020 21:55
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from cf22857 to abc3697 Compare August 25, 2020 21:55
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch from fd6c1b6 to f03e404 Compare August 25, 2020 23:52
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from abc3697 to ef542c7 Compare August 25, 2020 23:52
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch from f03e404 to ad8d6a2 Compare August 26, 2020 18:19
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from ef542c7 to 2506747 Compare August 26, 2020 18:19
@kriskowal kriskowal force-pushed the kris/endo-json-errors branch from ad8d6a2 to 89883e1 Compare August 26, 2020 18:25
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from 2506747 to 2bdad74 Compare August 26, 2020 18:25
Base automatically changed from kris/endo-json-errors to kris/endo-mitm August 26, 2020 19:02
@kriskowal kriskowal merged commit aa7871e into kris/endo-mitm Aug 26, 2020
@kriskowal kriskowal force-pushed the kris/ses-compartment-names branch from 2bdad74 to aa7871e Compare August 26, 2020 19:02
@kriskowal kriskowal deleted the kris/ses-compartment-names branch August 26, 2020 19:02
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.

None yet

2 participants