From 082210101fcc7e7aee007b8f3f124b5342cbb9b7 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 15 Feb 2019 11:17:31 +0100
Subject: [PATCH 1/2] src: check HasCaught() in JSStream calls

`MakeCallback` can return an empty `MaybeLocal<>` even if no
exception has been generated, in particular, if
we were already terminating the current thread *before* the `TryCatch`
scope started, which meant it would not have an exception to report.
---
 src/js_stream.cc                              | 10 ++---
 ...-worker-http2-generic-streams-terminate.js | 39 +++++++++++++++++++
 2 files changed, 44 insertions(+), 5 deletions(-)
 create mode 100644 test/parallel/test-worker-http2-generic-streams-terminate.js

diff --git a/src/js_stream.cc b/src/js_stream.cc
index 32dea04db5aeec..ae1aa7cd30e949 100644
--- a/src/js_stream.cc
+++ b/src/js_stream.cc
@@ -46,7 +46,7 @@ bool JSStream::IsClosing() {
   TryCatchScope try_catch(env());
   Local<Value> value;
   if (!MakeCallback(env()->isclosing_string(), 0, nullptr).ToLocal(&value)) {
-    if (!try_catch.HasTerminated())
+    if (try_catch.HasCaught() && !try_catch.HasTerminated())
       FatalException(env()->isolate(), try_catch);
     return true;
   }
@@ -62,7 +62,7 @@ int JSStream::ReadStart() {
   int value_int = UV_EPROTO;
   if (!MakeCallback(env()->onreadstart_string(), 0, nullptr).ToLocal(&value) ||
       !value->Int32Value(env()->context()).To(&value_int)) {
-    if (!try_catch.HasTerminated())
+    if (try_catch.HasCaught() && !try_catch.HasTerminated())
       FatalException(env()->isolate(), try_catch);
   }
   return value_int;
@@ -77,7 +77,7 @@ int JSStream::ReadStop() {
   int value_int = UV_EPROTO;
   if (!MakeCallback(env()->onreadstop_string(), 0, nullptr).ToLocal(&value) ||
       !value->Int32Value(env()->context()).To(&value_int)) {
-    if (!try_catch.HasTerminated())
+    if (try_catch.HasCaught() && !try_catch.HasTerminated())
       FatalException(env()->isolate(), try_catch);
   }
   return value_int;
@@ -99,7 +99,7 @@ int JSStream::DoShutdown(ShutdownWrap* req_wrap) {
                     arraysize(argv),
                     argv).ToLocal(&value) ||
       !value->Int32Value(env()->context()).To(&value_int)) {
-    if (!try_catch.HasTerminated())
+    if (try_catch.HasCaught() && !try_catch.HasTerminated())
       FatalException(env()->isolate(), try_catch);
   }
   return value_int;
@@ -134,7 +134,7 @@ int JSStream::DoWrite(WriteWrap* w,
                     arraysize(argv),
                     argv).ToLocal(&value) ||
       !value->Int32Value(env()->context()).To(&value_int)) {
-    if (!try_catch.HasTerminated())
+    if (try_catch.HasCaught() && !try_catch.HasTerminated())
       FatalException(env()->isolate(), try_catch);
   }
   return value_int;
diff --git a/test/parallel/test-worker-http2-generic-streams-terminate.js b/test/parallel/test-worker-http2-generic-streams-terminate.js
new file mode 100644
index 00000000000000..17a3edcf1cf725
--- /dev/null
+++ b/test/parallel/test-worker-http2-generic-streams-terminate.js
@@ -0,0 +1,39 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const http2 = require('http2');
+const { Duplex } = require('stream');
+const { Worker, isMainThread, workerData } = require('worker_threads');
+
+// Tests the interaction between terminating a Worker thread and running
+// the native SetImmediate queue, which may attempt to perform multiple
+// calls into JS even though one already terminates the Worker.
+
+if (isMainThread) {
+  const counter = new Int32Array(new SharedArrayBuffer(4));
+  const worker = new Worker(__filename, { workerData: { counter } });
+  worker.on('exit', common.mustCall(() => {
+    assert.strictEqual(counter[0], 1);
+  }));
+} else {
+  const { counter } = workerData;
+
+  // Start two HTTP/2 connections. This will trigger write()s call from inside
+  // the SetImmediate queue.
+  for (let i = 0; i < 2; i++) {
+    http2.connect('http://localhost', {
+      createConnection() {
+        return new Duplex({
+          write(chunk, enc, cb) {
+            Atomics.add(counter, 0, 1);
+            process.exit();
+          },
+          read() { }
+        });
+      }
+    });
+  }
+}

From f82df5a3fd8c210fd2e921abfc340e9393946e47 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Fri, 15 Feb 2019 19:24:19 +0100
Subject: [PATCH 2/2] fixup! src: check HasCaught() in JSStream calls

---
 test/parallel/test-worker-http2-generic-streams-terminate.js | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/parallel/test-worker-http2-generic-streams-terminate.js b/test/parallel/test-worker-http2-generic-streams-terminate.js
index 17a3edcf1cf725..234697fb4d26dc 100644
--- a/test/parallel/test-worker-http2-generic-streams-terminate.js
+++ b/test/parallel/test-worker-http2-generic-streams-terminate.js
@@ -6,13 +6,13 @@ if (!common.hasCrypto)
 const assert = require('assert');
 const http2 = require('http2');
 const { Duplex } = require('stream');
-const { Worker, isMainThread, workerData } = require('worker_threads');
+const { Worker, workerData } = require('worker_threads');
 
 // Tests the interaction between terminating a Worker thread and running
 // the native SetImmediate queue, which may attempt to perform multiple
 // calls into JS even though one already terminates the Worker.
 
-if (isMainThread) {
+if (!workerData) {
   const counter = new Int32Array(new SharedArrayBuffer(4));
   const worker = new Worker(__filename, { workerData: { counter } });
   worker.on('exit', common.mustCall(() => {