src,http: fix uncaughtException miss in http

In AsyncWrap::MakeCallback always return empty handle if there is an
error. In the future this should change to return a v8::MaybeLocal, but
that major change will have to wait for v6.x, and these changes are
meant to be backported to v4.x.

The HTTParser call to AsyncWrap::MakeCallback failed because it expected
a thrown call to return an empty handle.

In node::MakeCallback return an empty handle if the call is
in_makecallback(), otherwise return v8::Undefined() as usual to preserve
backwards compatibility.

Fixes: #5555
PR-URL: #5591
Reviewed-By: Julien Gilli <[email protected]>

Author: trevnorris

src/async-wrap.cc

@@ -193,7 +193,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
     if (has_domain) {
       domain = domain_v.As<Object>();
       if (domain->Get(env()->disposed_string())->IsTrue())
-        return Undefined(env()->isolate());
+        return Local<Value>();
     }
   }
 
@@ -220,7 +220,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }
 
   if (ret.IsEmpty()) {
-    return Undefined(env()->isolate());
+    return ret;
   }
 
   if (has_domain) {
@@ -249,7 +249,7 @@ Local<Value> AsyncWrap::MakeCallback(const Local<Function> cb,
   }
 
   if (env()->tick_callback_function()->Call(process, 0, nullptr).IsEmpty()) {
-    return Undefined(env()->isolate());
+    return Local<Value>();
   }
 
   return ret;

src/node.cc

@@ -1185,7 +1185,11 @@ Local<Value> MakeCallback(Environment* env,
   }
 
   if (ret.IsEmpty()) {
-    return Undefined(env->isolate());
+    if (callback_scope.in_makecallback())
+      return ret;
+    // NOTE: Undefined() is returned here for backwards compatibility.
+    else
+      return Undefined(env->isolate());
   }
 
   if (has_domain) {

test/parallel/test-http-catch-uncaughtexception.js

@@ -0,0 +1,23 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const http = require('http');
+
+const uncaughtCallback = common.mustCall(function(er) {
+  assert.equal(er.message, 'get did fail');
+});
+
+process.on('uncaughtException', uncaughtCallback);
+
+const server = http.createServer(function(req, res) {
+  res.writeHead(200, { 'Content-Type': 'text/plain' });
+  res.end('bye');
+}).listen(common.PORT, function() {
+  http.get({ port: common.PORT }, function(res) {
+    res.resume();
+    throw new Error('get did fail');
+  }).on('close', function() {
+    server.close();
+  });
+});