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

vm: behavior with --abort-on-uncaught-exception #13258

Closed
cjihrig opened this issue May 27, 2017 · 7 comments
Closed

vm: behavior with --abort-on-uncaught-exception #13258

cjihrig opened this issue May 27, 2017 · 7 comments
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@cjihrig
Copy link
Contributor

cjihrig commented May 27, 2017

  • Version: master
  • Platform: all
  • Subsystem: vm

In the following code, the vm.Script() constructor throws an exception. When node is run normally, the exception is caught and node exits cleanly. However, when run with --abort-on-uncaught-exception, the exception is treated as uncaught.

'use strict';
const vm = require('vm');

try {
  new vm.Script('[', {});
} catch (err) {}

Before attempting to fix this, I wanted to make sure there is agreement that this is a bug. This seems like something that could be handled in ShouldAbortOnUncaughtException().

Related: nodejs/node-report#60

@addaleax addaleax added the vm Issues and PRs related to the vm subsystem. label May 27, 2017
@addaleax
Copy link
Member

I agree, this looks like a bug.

This seems like something that could be handled in ShouldAbortOnUncaughtException().

I’m not sure – why doesn’t V8 see the try { … } catch {} block here? And how would we tell whether the exception would be caught or not?

@cjihrig
Copy link
Contributor Author

cjihrig commented May 27, 2017

My understanding of the code I linked (and I could definitely be wrong) is that the try...catch would be seen and catch the error, but it aborts before getting back to that point because it is an external handler. It might even be considered a V8 bug, but the comment in that code block seems deliberate.

@mscdex
Copy link
Contributor

mscdex commented May 28, 2017

/cc @nodejs/v8

@bnoordhuis
Copy link
Member

I’m not sure – why doesn’t V8 see the try { … } catch {} block here?

new vm.Script(...) calls ContextifyScript::New(). It has a v8::TryCatch (an EXTERNAL handler, per the comment Colin linked to) that V8 sees as the top handler.

I'd say this is a bug but I don't have a good suggestion off the top of my head on how to fix it.

  • The TryCatch is there in order to decorate SyntaxError exceptions. Simply removing it is going to break more than it fixes and moving it to after the call to v8::ScriptCompiler::CompileUnboundScript() won't work.

  • Naively removing the CAUGHT_BY_EXTERNAL check from V8 breaks --abort_on_uncaught_exception because there is always a TryCatch on the stack in node.

  • That bottom TryCatch can't be removed because that breaks process.on('uncaughtException') and a host of other things.

@cjihrig
Copy link
Contributor Author

cjihrig commented May 29, 2017

Even if we can't fix this in the general case, do you think it's worth adding an option to the vm functions to control this behavior?

@fhinkel
Copy link
Member

fhinkel commented Aug 3, 2017

As this is very brittle to changes, I'd prefer not to add the option. As Ben said, most solutions would break more than fix. Do you have a use case for it? Would it be enough to note the bug in the documentation?

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 4, 2017

I'm definitely in favor of at least documenting this.

I don't personally have a need for the option. However, running with --abort-on-uncaught-exception is a somewhat common practice in production, and this bug kind of makes the vm module incompatible with that. That said, I don't recall ever seeing this reported before.

addaleax added a commit to addaleax/node that referenced this issue Dec 1, 2017
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.

Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.

Fixes: nodejs#13258
MylesBorins pushed a commit that referenced this issue Dec 21, 2017
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.

Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.

PR-URL: #17394
Fixes: #13258
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants