Repository navigation
vm: behavior with --abort-on-uncaught-exception #13258
Description
- 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
Activity
addaleax commentedon May 27, 2017
I agree, this looks like a bug.
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 commentedon May 27, 2017
My understanding of the code I linked (and I could definitely be wrong) is that the
try...catchwould 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 commentedon May 28, 2017
/cc @nodejs/v8
bnoordhuis commentedon May 28, 2017
new vm.Script(...)callsContextifyScript::New(). It has av8::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_exceptionbecause 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 commentedon 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
vmfunctions to control this behavior?fhinkel commentedon 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 commentedon 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-exceptionis a somewhat common practice in production, and this bug kind of makes thevmmodule incompatible with that. That said, I don't recall ever seeing this reported before.vm: never abort on caught syntax error
vm: never abort on caught syntax error