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

How do you catch runtime errors in the realm? #212

Closed
mantou132 opened this issue Oct 14, 2019 · 18 comments
Closed

How do you catch runtime errors in the realm? #212

mantou132 opened this issue Oct 14, 2019 · 18 comments

Comments

@mantou132
Copy link

mantou132 commented Oct 14, 2019

let r = new Realm();
let f = r.evaluate(`
  onerror ???
  asf; // runtime error
`);
@Jamesernator
Copy link

Jamesernator commented Oct 29, 2019

Are you wanting something like window.onerror? The short answer is you'd have to provide something yourself e.g.:

class Environment {
  _realm = Realm.makeRootRealm();
  _onUncaughtError;

  constructor(onUncaughtError) {
    this._onUncaughtError = onUncaughtError;
  }

  runScript(sourceText) {
    try {
      return this._realm.eval(sourceText);
    } catch (err) {
      if (this._realm.global.onerror) {
        this._realm.global.onerror(err);
      } else {
        this._onUncaughtError(err);
      } 
    }
  }
}

const e = new Environment(uncaughtError => console.error(uncaughtError));

e.runScript(`
  globalThis.onerror = (err) => {
    globalThis.caughtAnError = true;
  }
`);

e.runScript(`
  throw new Error("Ooops!");
`);

e.global.caughtAnError === true;

@mantou132
Copy link
Author

@Jamesernator This does not catch asynchronous execution errors.

e.runScript(`
  setTimeout(() => {
      throw new Error("Ooops!");
    })
`);

@Jamesernator
Copy link

Jamesernator commented Oct 29, 2019

Note that setTimeout is also host defined†, it's not part of the language, you can do similar wrapping when adding it to the Realm:

class Environment {
  _realm = Realm.makeRootRealm();
  _onUncaughtError;

  constructor(onUncaughtError) {
    this._onUncaughtError = onUncaughtError;

    // Define our own setTimeout that creates a setTimeout within the Realm
    this._realm.evaluate(`
      globalThis.setTimeout = function setTimeout(func, ...otherArgs) {
        realSetTimeout(function(...args) {
          try {
            func(...args);
          } catch (err) {
            handleUncaughtError(err);
          }
        }, ...otherArgs);
      }
    `, {
      // inject real functions from our own host into this evaluation
      // this does not make them available to successive untrusted scripts
      realSetTimeout: setTimeout,
      handleUncaughtError: (err) => this._handleUncaughtError(err),
     });
  }

  _handleUncaughtError(err) {
    if (this._realm.onerror) {
      try {
        this._realm.onerror(err);
      } catch (err) {
        this._onUncaughtError(err);
      }
    } else {
      this._onUncaughtError(err);
    }
  }

  runScript(sourceText) {
    try {
      return this._realm.eval(sourceText);
    } catch (err) {
      this._handleUncaughtError(err);
    }
  }
}

const e = new Environment(() => console.error("An error was never caught"));

e.runScript(`
  globalThis.onerror = () => {
    // will be called at some point
  }

  setTimeout(() => {
    throw new Error("Oops!");
  });
`);

Now this is especially confusing because the window.onerror handling is a separate error handling mechanism to the only error handling mechanism that is actually part of the language unhandledrejection, I have an issue for that though here amongst other things.

† In browsers setTimeout is defined by the HTML spec. The window.onerror behavior falls out of a bunch of HTML architecture.

@caridy
Copy link
Collaborator

caridy commented Nov 27, 2019

Resolution:

The error control is done by the agent, not by the realm the realm doesn't know about next-turn. This level of control could be implemented in the future if we ever attempt to introduce the concept of the agent. It seems that there is nothing to be done here.

@caridy caridy closed this as completed Nov 27, 2019
@ghermeto
Copy link
Member

ghermeto commented Feb 5, 2020

I agree having error control happening on the agent is a good idea, but this quote:

could be implemented in the future if we ever attempt to introduce the concept of the agent

worries me... What will be the recommended way to do error handling until agents are introduced?

cc/ @erights

@erights
Copy link
Collaborator

erights commented Feb 6, 2020

We should work towards an Agent API, so we don't have to start by doing things the wrong way.

Do you have a particular error example that is too urgent to wait for Agent?

@ghermeto
Copy link
Member

ghermeto commented Feb 6, 2020

synchronous:

let r = new Realm();
let f = r.evaluate(`
  throw new Error('error in realm!');
`);

and async:

let r = new Realm();
let f = r.evaluate(`
  let p = new Promise((resolve, reject) => {
    reject('rejection in relam');
  });
`);

I'm specifically concerned with the observability and debuggability around uncaught exceptions and unhandled rejections. Once Realms land, it is likely that frameworks start incorporating it to achieve isolation and I would like to be sure we have proper error handling...

@erights
Copy link
Collaborator

erights commented Feb 6, 2020

@ghermeto and I talked about this and we decided to start an Agent proposal. However, we need a name other than Agent. It was always a terrible name for the concept. Anyone care to suggest a better name before we create the proposal repository?

@caridy
Copy link
Collaborator

caridy commented Feb 6, 2020

@ghermeto this is a great question, for "synchronous" error, it seems to be fine, because the "initiator" is always the outer realm, which can try/catch it and react to it. But for "async" error, this is a little bit more complicated. IIRC, in browsers today, the identity of the Promise intrinsic object matters, e.g.:

    iframe.contentWindow.eval(`
        new Promise((resolve, reject) => {
            reject('rejection of Promise Intrinsic Object from iframe');
        });
    `);
    iframe.contentWindow.eval(`
        // top.* here means outer realm
        new top.Promise((resolve, reject) => {
            reject('rejection of Promise Intrinsic Object from outer realm');
        });
    `);

I believe that only the second rejection will be observed from outer realm. We should discuss this in more details asap.

I also think that this touches on another set of open issues, what are the capabilities a newly created realm, maybe the host can provide extra sugar for folks trying to observe uncaught exceptions. I will reopen this issue. /cc @littledan

@caridy caridy reopened this Feb 6, 2020
@caridy
Copy link
Collaborator

caridy commented Feb 6, 2020

To clarify, I am talking about HostPromiseRejectionTracker here, specified here https://tc39.es/ecma262/#sec-host-promise-rejection-tracker, but it is host specific. It doesn't say much, it is something controlled by the host.

Update: another important point to notice here is that in nodejs, things are quite different, the unhandled rejection can be capture at the process level via process.on, in which case the HostPromiseRejectionTracker will be able to capture any rejection independently of the realm that you're on.

@ghermeto
Copy link
Member

ghermeto commented Feb 7, 2020

@caridy I think if it gets to the unhandledRejection event, then it is diminishes the value to use Realms for virtualization and isolation, because it means the process should abort and because the error happened inside the Realm, you probably don't want it terminating the host.

I think having an Agent or Zone what can isolate and observe the errors for the Realms and Compartments would be the right abstraction, but in the meantime that is a possible solution for node. However, we still needs an interim solution for browsers...

@caridy
Copy link
Collaborator

caridy commented Feb 7, 2020

Ok, after spending some time discussing this with few folks, here is my proposal:

Proposal

  • No normative changes to the spec (which is intentionally vague on the error control).
  • Add a non-normative section to suggest that unhandled rejections in a realm can be handled by the host via HostPromiseRejectionTracker on the window. Matching the semantics of the vm module in node (via process). This will allow framework authors to at least capture the failure, and implement some user-land artifact to identify the origin of the event.

Once we have the new proposal for the Agent, we can provide a lot more control. Honestly, I haven't seen folks complaining about this in node when using VM, it might be sufficient to capture them at the process level without ways to identify the realm.

@devsnek
Copy link
Member

devsnek commented Feb 7, 2020

Add a non-normative section to suggest that unhandled rejections in a realm can be handled by the host via HostPromiseRejectionTracker on the window.

This is already a normative requirement in the current spec.

@devsnek
Copy link
Member

devsnek commented Feb 7, 2020

@erights you should call it System :P

@littledan
Copy link
Member

I'm having trouble following this thread. I imagined the Realm proposal was all part of the same Agent, and therefore, there's no support for attribution of errors to a particular Realm. (I'm not even sure what it would mean to attribute errors to a Realm, since callstacks may be mixed, unlike between Agents.) Is there anywhere I can read more about what people are thinking about for Agents?

@littledan
Copy link
Member

For Stage 3, I think we should draw a conclusion on this issue. Here's a conclusion that I'd propose:

  • In this proposal alone, errors are tracked by the agent, not the Realm, so there's no changes.
  • In a future proposal, maybe for Zones or Agents, or maybe for Evaluators or Compartments, there may be hooks to control error tracking.

Does this match the champion group's thoughts?

@caridy
Copy link
Collaborator

caridy commented Feb 28, 2020

💯

@caridy caridy closed this as completed Feb 28, 2020
@littledan
Copy link
Member

As part of closing this issue, should the resolution be documented anywhere (e.g., the explainer)? I'm planning on integrating it into the HTML PR, but we should probably have some record of what the intention is cross-environment.

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

No branches or pull requests

7 participants