From 3521b052b175110aab08fe6296e7d5cffbb72925 Mon Sep 17 00:00:00 2001
From: Trevor Norris <trev.norris@gmail.com>
Date: Fri, 4 Mar 2016 15:14:27 -0700
Subject: [PATCH] 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: https://github.com/nodejs/node/issues/5555
PR-URL: https://github.com/nodejs/node/pull/5591
Reviewed-By: Julien Gilli <jgilli@nodejs.org>
---
 src/async-wrap.cc                             |  6 ++---
 src/node.cc                                   |  6 ++++-
 .../test-http-catch-uncaughtexception.js      | 23 +++++++++++++++++++
 3 files changed, 31 insertions(+), 4 deletions(-)
 create mode 100644 test/parallel/test-http-catch-uncaughtexception.js

diff --git a/src/async-wrap.cc b/src/async-wrap.cc
index 11a696cc23..e205403101 100644
--- a/src/async-wrap.cc
+++ b/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;
diff --git a/src/node.cc b/src/node.cc
index c71e35a45d..976237efe2 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -1198,7 +1198,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) {
diff --git a/test/parallel/test-http-catch-uncaughtexception.js b/test/parallel/test-http-catch-uncaughtexception.js
new file mode 100644
index 0000000000..c4054aafbf
--- /dev/null
+++ b/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();
+  });
+});