node does not abort at the right time when using --abort-on-uncaught-exception

[misterdjules]

nodejs/master's head does not abort at the right time when using --abort-on-uncaught-exception. Here's a reproduction of the problem on SmartOS:

$ git rev-parse --short HEAD
02448c6
[root@dev ~/node-1]# make
make -C out BUILDTYPE=Release V=1
make[1]: Entering directory '/root/node-1/out'
make[1]: Nothing to be done for 'all'.
make[1]: Leaving directory '/root/node-1/out'
ln -fs out/Release/node node
$ ln -sf `pwd`/out/Release/node /opt/local/bin/node
[root@dev ~/node-1]# node --version
v5.0.0-pre
$ node --abort-on-uncaught-exception -e 'setTimeout(function () { function boom() { throw new Error("foo") } boom(); }, 10);'
[eval]:1
setTimeout(function () { function boom() { throw new Error("foo") } boom(); }, 10);
                                           ^

Error: foo
    at boom ([eval]:1:50)
    at null._onTimeout ([eval]:1:69)
    at Timer.listOnTimeout (timers.js:89:15)
Abort (core dumped)
$ mdb /var/cores/core.node.8212 
Loading modules: [ libumem.so.1 libc.so.1 ld.so.1 ]
> ::load /root/mdb_v8/build/amd64/mdb_v8.so
mdb_v8 version: 1.0.0 (dev)
V8 version: 4.5.103.33
Autoconfigured V8 support from target
C++ symbol demangling enabled
> ::jsstack
native: libc.so.1`_lwp_kill+0xa
native: libc.so.1`raise+0x20
native: libc.so.1`abort+0x98
native: node::FatalException+0xd7
native: v8::internal::MessageHandler::ReportMessage+0x1ee
native: v8::internal::Isolate::ReportPendingMessages+0x234
native: v8::internal::Execution::Call+0x4a3
native: v8::Function::Call+0xff
native: v8::Function::Call+0x41
native: node::AsyncWrap::MakeCallback+0x23a
native: node::TimerWrap::OnTimeout+0x96
native: uv__run_timers+0x7d
native: uv_run+0x35a
native: node::Start+0x538
native: _start+0x6c
>

Note that the call stack doesn't contain any JavaScript frame, and indicates that node aborted in node::FatalException, not when the error was thrown.

This is a problem because users of post-mortem debuggers and --abort-on-uncaught-exception need to have core dumps that are generated when the exception is thrown, and the core dumps need to have the frame that throws the error in the call stack. Otherwise, it becomes much more difficult, if not impossible to determine the root cause of the problem.

In the example above, there's no way to know that the error was thrown by the function named foo, we just know that one timer's callback threw.

This regression was introduced by #922, which made V8 ignore --abort-on-uncaught-exception. An attempt at fixing that regression was made with #2776, but instead of letting V8 abort in Isolate::Throw (when it actually throws the error and when all the relevant frames are active on the stack), it throws in node::FatalException as shown above.

I will submit two different PRs that fix this issue in two different ways so that we can discuss the pros and cons of the two different approaches I came up with.

/cc @nodejs/post-mortem

[misterdjules]

I implemented two different approaches with #3036 and #3038 to solve this problem. They're both drafts of PRs, and I didn't pay too much attention to details. I'm mainly looking for feedback on what would be the preferred approach.

I personally have a preference for #3036, because I find it less intrusive and it's been tested with node v0.10. However I would welcome any other opinion/feedback, and there may be other valid approaches.

[yunong]

Just to weigh in here -- this is critical for us at Netflix and our production stack -- and will potentially prevent us from moving to 4.x until this is fixed. Thanks for all the hardwork @misterdjules and we'd love to see this integrated soon!

[evanlucas]

Thanks @misterdjules! I feel like #3036 is a much cleaner way of doing it and it doesn't require messing with timers and repl.

[Raynos]

👍

It's important that we have the right stack. --abort-on-uncaught-exception is critical for debugging "Maximum call stack size exceeded".

We need the stack in the core file to have the entire call stack that we blew up.

[misterdjules]

If the two approaches described in #3036 and #3038 are not viable, a third option is to revert 0af4c9e and 921f2de, and to display a warning when domains and --abort-on-uncaught-exception are used at the same time.

This would bring us back to the point where domains and --abort-on-uncaught-exception work independently, but not when used together in some use cases.