From 02bdacef42bbaeeaea1ef5a6c1588d5f934cbb5c Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Mon, 2 Sep 2019 13:18:39 +0200
Subject: [PATCH 1/2] Ensure that `Error`s are handled correctly when using
 `postMessage` with Streams in `MessageHandler`

Having recently worked with this code, it struck me that most of the `postMessage` calls where `Error`s are involved have never been correctly implemented (i.e. missing `wrapReason` calls).
---
 src/shared/message_handler.js     | 10 +++++-----
 test/unit/message_handler_spec.js | 10 ++++++----
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js
index 2a7a64d0b..ee4122649 100644
--- a/src/shared/message_handler.js
+++ b/src/shared/message_handler.js
@@ -231,7 +231,7 @@ MessageHandler.prototype = {
           targetName,
           stream: StreamKind.CANCEL,
           streamId,
-          reason,
+          reason: wrapReason(reason),
         });
         // Return Promise to signal success or failure.
         return cancelCapability.promise;
@@ -296,7 +296,7 @@ MessageHandler.prototype = {
           targetName,
           stream: StreamKind.ERROR,
           streamId,
-          reason,
+          reason: wrapReason(reason),
         });
       },
 
@@ -327,7 +327,7 @@ MessageHandler.prototype = {
         targetName,
         stream: StreamKind.START_COMPLETE,
         streamId,
-        reason,
+        reason: wrapReason(reason),
       });
     });
   },
@@ -397,7 +397,7 @@ MessageHandler.prototype = {
             targetName,
             stream: StreamKind.PULL_COMPLETE,
             streamId,
-            reason,
+            reason: wrapReason(reason),
           });
         });
         break;
@@ -450,7 +450,7 @@ MessageHandler.prototype = {
             targetName,
             stream: StreamKind.CANCEL_COMPLETE,
             streamId,
-            reason,
+            reason: wrapReason(reason),
           });
         });
         this.streamSinks[data.streamId].sinkCapability.
diff --git a/test/unit/message_handler_spec.js b/test/unit/message_handler_spec.js
index f9465791f..908391e10 100644
--- a/test/unit/message_handler_spec.js
+++ b/test/unit/message_handler_spec.js
@@ -142,11 +142,13 @@ describe('message_handler', function () {
         sink.onCancel = function (reason) {
           log += 'c';
         };
+        log += '0';
         sink.ready.then(() => {
+          log += '1';
           sink.enqueue([1, 2, 3, 4], 4);
           return sink.ready;
         }).then(() => {
-          log += 'error';
+          log += 'e';
           sink.error('error');
         });
       });
@@ -161,14 +163,14 @@ describe('message_handler', function () {
       let reader = readable.getReader();
 
       sleep(10).then(() => {
-        expect(log).toEqual('');
+        expect(log).toEqual('01');
         return reader.read();
       }).then((result) => {
         expect(result.value).toEqual([1, 2, 3, 4]);
         expect(result.done).toEqual(false);
         return reader.read();
-      }).then(() => {
-      }, (reason) => {
+      }).catch((reason) => {
+        expect(log).toEqual('01pe');
         expect(reason).toEqual('error');
         done();
       });

From 74f5a59f43e501d9603de64b7a9393e894d7cdce Mon Sep 17 00:00:00 2001
From: Jonas Jenwald <jonas.jenwald@gmail.com>
Date: Mon, 2 Sep 2019 13:28:19 +0200
Subject: [PATCH 2/2] Ensure that the `cancel`/`error` methods on Streams are
 always called with valid `reason` arguments

---
 src/shared/message_handler.js     | 2 ++
 test/unit/message_handler_spec.js | 9 ++++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/src/shared/message_handler.js b/src/shared/message_handler.js
index ee4122649..88c565775 100644
--- a/src/shared/message_handler.js
+++ b/src/shared/message_handler.js
@@ -223,6 +223,7 @@ MessageHandler.prototype = {
       },
 
       cancel: (reason) => {
+        assert(reason instanceof Error, 'cancel must have a valid reason');
         let cancelCapability = createPromiseCapability();
         this.streamControllers[streamId].cancelCall = cancelCapability;
         this.streamControllers[streamId].isClosed = true;
@@ -287,6 +288,7 @@ MessageHandler.prototype = {
       },
 
       error(reason) {
+        assert(reason instanceof Error, 'error must have a valid reason');
         if (this.isCancelled) {
           return;
         }
diff --git a/test/unit/message_handler_spec.js b/test/unit/message_handler_spec.js
index 908391e10..2eaee0030 100644
--- a/test/unit/message_handler_spec.js
+++ b/test/unit/message_handler_spec.js
@@ -13,7 +13,9 @@
  * limitations under the License.
  */
 
-import { AbortException, createPromiseCapability } from '../../src/shared/util';
+import {
+  AbortException, createPromiseCapability, UnknownErrorException
+} from '../../src/shared/util';
 import { LoopbackPort } from '../../src/display/api';
 import { MessageHandler } from '../../src/shared/message_handler';
 
@@ -149,7 +151,7 @@ describe('message_handler', function () {
           return sink.ready;
         }).then(() => {
           log += 'e';
-          sink.error('error');
+          sink.error(new Error('should not read when errored'));
         });
       });
       let messageHandler1 = new MessageHandler('main', 'worker', port);
@@ -171,7 +173,8 @@ describe('message_handler', function () {
         return reader.read();
       }).catch((reason) => {
         expect(log).toEqual('01pe');
-        expect(reason).toEqual('error');
+        expect(reason instanceof UnknownErrorException).toEqual(true);
+        expect(reason.message).toEqual('should not read when errored');
         done();
       });
     });