domains: port fix abort on uncaught to v0.12

[whitlockjc]

Port fbff705 and caeb677 from v0.10 to v0.12, original commit messages:

fbff705
Add v8::Isolate::SetAbortOnUncaughtException() so the user can be
notified when an uncaught exception has bubbled.

caeb677
Do not abort the process if an error is thrown from within a domain,
an error handler is setup for the domain and
--abort-on-uncaught-exception was passed on the command line.

However, if an error is thrown from within the top-level domain's
error handler and --abort-on-uncaught-exception was passed on the
command line, make the process abort.

Fixes: #8877

[misterdjules]

Thank you @whitlockjc! Tests running on UNIX and on Windows.

[misterdjules]

@orangemocha I've re-enabled the node-accept-pull-request job on jenkins.nodejs.org to test this PR because when I tried to use the node-test-pull-request job on jenkins-iojs.nodesource.com, it failed: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/63/.

[misterdjules]

/cc @joyent/node-collaborators

[misterdjules]

@whitlockjc test-domain-with-abort-on-uncaught-exception.js is failing on Linux and SmartOS.

Fixing them should be straightforward, could you please update this PR with a fix when you have some time? Thank you!

[whitlockjc]

You got it. I'm sure their failures are similar to the fix for OS X where we have to check for different exit codes. I'll update the PR.

[orangemocha]

@orangemocha I've re-enabled the node-accept-pull-request job on jenkins.nodejs.org to test this PR because when I tried to use the node-test-pull-request job on jenkins-iojs.nodesource.com, it failed: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/63/.

The issue should be fixed: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/65/

In any case, you can use node-accept-pull-request from the new Jenkins. I have re-disabled the old one now. Let me know if you still have problems. Thanks!

[misterdjules]

A fix for similar issue(s) was merged in io.js a while ago: nodejs/node#922. That fix has the advantage of not requiring to make any change to V8, so it would be great if we could use it instead of the changes in this PR.

However, it seems that that fix does not fix #8630:

➜  v0.12 git:(v0.12) ✗ cat ~/dev/repros/domain-top-level-error-handler-throw.js
var domainErrHandlerExMessage = 'exception from domain error handler';
var internalExMessage = 'You should NOT see me';

var domain = require('domain');
var d = domain.create();

d.on('error', function() {
    console.log('in domain error handler');
    throw new Error(domainErrHandlerExMessage);
});

d.run(function doStuff() {
    process.nextTick(function () {
      throw new Error(internalExMessage);
    });
});
➜  v0.12 git:(v0.12) ✗ ./node ~/dev/repros/domain-top-level-error-handler-throw.js
in domain error handler
/Users/JulienGilli/dev/repros/domain-top-level-error-handler-throw.js:14
      throw new Error(internalExMessage);
            ^
Error: You should NOT see me
    at /Users/JulienGilli/dev/repros/domain-top-level-error-handler-throw.js:14:13
    at process._tickDomainCallback (node.js:389:11)
    at Function.Module.runMain (module.js:503:11)
    at startup (node.js:129:16)
    at node.js:822:3
➜  v0.12 git:(v0.12) ✗

It seems to also make a node process not abort when it seems it should:

➜  v0.12 git:(v0.12) ✗ cat ~/dev/repros/domain-abort-on-uncaught.js
 var domain = require('domain');
var d = domain.create();
var triggeredProcessUncaughtException = false;

process.on('uncaughtException', function onUncaughtException() {
  console.log('In uncaughtException handler');
});

d.on('error', function() {
  console.log('Throwing from top level domain error handler');
  throw new Error('Error from domain error handler');
});

d.run(function doStuff() {
  // Throwing from within different types of callbacks as each of them
  // handles domains differently
  process.nextTick(function () {
    throw new Error("Error from nextTick callback");
  });

  var fs = require('fs');
  fs.exists('/non/existing/file', function onExists(exists) {
    throw new Error("Error from fs.exists callback");
  });

  setImmediate(function onSetImmediate() {
    throw new Error("Error from setImmediate callback");
  });

  throw new Error("Error from domain.run callback");
});
➜  v0.12 git:(v0.12) ✗ ./node --abort-on-uncaught-exception ~/dev/repros/domain-abort-on-uncaught.js
Throwing from top level domain error handler
In uncaughtException handler
Throwing from top level domain error handler
In uncaughtException handler
Throwing from top level domain error handler
Throwing from top level domain error handler
In uncaughtException handler
Throwing from top level domain error handler
Throwing from top level domain error handler
Throwing from top level domain error handler
In uncaughtException handler
➜  v0.12 git:(v0.12) ✗ 

In the test above, I would expect node to abort, not the uncaughtException handler to be called. But I may misinterpret the semantics of process.on('uncaughtException') and --abort-on-uncaught-exception.

@chrisdickinson Thoughts?

[whitlockjc]

I have updated the tests to pass on Linux and SmartOS. This involved always checking the exit signal, even though SmartOS is the only one right now that gets a signal other than null, and updating Linux to have variable exit codes. I also updated the comments explaining why we use arrays of possible values for exit codes/signals.

[misterdjules]

Thank you @whitlockjc!

Reran the whole tests suite here: https://jenkins-iojs.nodesource.com/job/node-accept-pull-request/. We have one failure on Windows. This is also related to changes in the implementation of OS::Abort in V8 from v0.10 to v0.12.

[whitlockjc]

I'll get on it.

[whitlockjc]

Fixed, @misterdjules was right...this was a result of OS::Abort changing its exit code so we now need to check for multiple values like we do on *nix. I also cleaned up a few places in the test where lines were greater than 80 characters.

[misterdjules]

@whitlockjc Thank you! All tests passed.

[whitlockjc]

WOOHOO!!!

[jasnell]

@misterdjules ... is this ready to land?

[whitlockjc]

I don't want this to drop through the cracks so can we update this with the next steps? Really hoping to either find out that this isn't necessary in the new node repository or be told what the next steps are. If it's still useful/important, I'll gladly port this to the new repository via a PR.

/cc @nodejs/collaborators and @nodejs/tsc

[misterdjules]

@whitlockjc I've ported this PR in nodejs/node's master with nodejs/node#3036 because of nodejs/node#3035.

The next step for this PR is for me to give it a final review and land it in v0.12.

[whitlockjc]

Thanks a lot. I was planning on porting this today but since you beat me to it... ;)

[misterdjules]

There should be no parentheses around FLAG_abort_on_uncaught_exception.

[whitlockjc]

Yup, that was an oversight...probably a relic from previous debugging session.

[misterdjules]

@whitlockjc Added more comments.

Also, it would be great to split this PR in two different commits, the first one for the changes in deps/v8, and the second one for the changes outside of deps/v8. This way we can isolate and document this new floating patch in https://github.com/nodejs/node-v0.x-archive/wiki/V8-upgrade-process#v012x.

When these are addressed we can land this PR in v0.12. Thank you for your work again! 👍

[whitlockjc]

You got it. I'll get on it.

[whitlockjc]

I've addressed the changes @misterdjules suggested. I will not break the singular port commit into two port commits. When this is complete, I will update you.

[whitlockjc]

I have broken the PR commit into two commits, one for v8 and one for domains. I retained the original commit messages in their respective port commits but the subject of each port commit has been shortened to fit.

[misterdjules]

LGTM. Thank you once again @whitlockjc!

[misterdjules]

Landing with https://ci.nodejs.org/job/node-accept-pull-request/129/.

[misterdjules]

Landed in 1982ed6 and f0453ca.

[whitlockjc]

Is there a next step for getting this into nodejs/node?

[misterdjules]

@whitlockjc These two commits are in nodejs/node's v0.12 branch. I'll follow-up on nodejs/node#3036 when I get some feedback about the V8 changes in https://codereview.chromium.org/1375933003/. If that takes too long, or if it turns out nodejs/node#3036 is not the solution we want to consider, we'll consider the other options mentioned in nodejs/node#3035.

[whitlockjc]

Gotcha. Thanks for the heads up and let me know if I can help further regardless of the decisions mentioned above.