From 6bdacfb973257acface9c7ebddc74ac213280391 Mon Sep 17 00:00:00 2001
From: Maksim Ivanov <emaxx@google.com>
Date: Thu, 14 Sep 2023 17:57:25 +0200
Subject: [PATCH] JS Requester handles disposal-and-exception-while-sending

Improve the JS Requester class to gracefully handle this edge case:
the send() call triggers not only an exception, but also the disposal
of the message channel. The bug was that we were trying to reject the
same request twice (first during disposal, second while handling the
exception).

This is follow-up to the crash handling issue fixed in #823.
---
 .../js/src/requesting/requester-unittest.js   | 49 ++++++++++++++++---
 common/js/src/requesting/requester.js         |  7 ++-
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/common/js/src/requesting/requester-unittest.js b/common/js/src/requesting/requester-unittest.js
index 8d1fa9f91..c0cb3f881 100644
--- a/common/js/src/requesting/requester-unittest.js
+++ b/common/js/src/requesting/requester-unittest.js
@@ -62,6 +62,18 @@ class PromiseTracker {
   }
 }
 
+/**
+ * @param {!goog.testing.messaging.MockMessageChannel} mockMessageChannel
+ */
+function disposeOfMockMessageChannel(mockMessageChannel) {
+  mockMessageChannel.dispose();
+  // Hack: call the protected disposeInternal() method in order to trigger the
+  // disposal notifications; the MockMessageChannel.dispose() doesn't call
+  // them.
+  mockMessageChannel[goog.reflect.objectProperty(
+      'disposeInternal', mockMessageChannel)]();
+}
+
 goog.exportSymbol('testRequester', function() {
   const REQUESTER_NAME = 'test-requester';
   const REQUEST_MESSAGE_TYPE = getRequestMessageType(REQUESTER_NAME);
@@ -133,12 +145,7 @@ goog.exportSymbol('testRequester', function() {
   // After that, dispose of the message channel. The promise for request #0 gets
   // rejected.
   const request0CompletionPromise = request1CompletionPromise.then(function() {
-    mockMessageChannel.dispose();
-    // Hack: call the protected disposeInternal() method in order to trigger the
-    // disposal notifications; the MockMessageChannel.dispose() doesn't call
-    // them.
-    mockMessageChannel[goog.reflect.objectProperty(
-        'disposeInternal', mockMessageChannel)]();
+    disposeOfMockMessageChannel(mockMessageChannel);
     return request0Promise.then(null, function(rejectionError) {
       assert(request0PromiseTracker.isResolved);
       assert(request1PromiseTracker.isResolved);
@@ -169,6 +176,8 @@ goog.exportSymbol('testRequester_disposed', function() {
   }, () => {});
 });
 
+// Test that an exception happening while sending the request message results in
+// a rejected response promise.
 goog.exportSymbol('testRequester_exceptionWhileSending', async function() {
   const REQUESTER_NAME = 'test-requester';
   const SEND_ERROR_MESSAGE = 'fake send error';
@@ -192,4 +201,32 @@ goog.exportSymbol('testRequester_exceptionWhileSending', async function() {
   }
   fail('Message posting unexpectedly succeeded');
 });
+
+// Test that if sending the request message results in immediate disposal and an
+// exception, the response promise is rejected correctly.
+goog.exportSymbol(
+    'testRequester_exceptionAndDisposalWhileSending', async function() {
+      const REQUESTER_NAME = 'test-requester';
+      const SEND_ERROR_MESSAGE = 'fake send error';
+
+      const mockControl = new goog.testing.MockControl();
+      const mockMessageChannel =
+          new goog.testing.messaging.MockMessageChannel(mockControl);
+      /** @type {?} */ mockMessageChannel.send;
+      // Set up mock that throws an exception when a message is sent.
+      mockMessageChannel.send(ignoreArgument, ignoreArgument).$does(() => {
+        disposeOfMockMessageChannel(mockMessageChannel);
+        throw new Error(SEND_ERROR_MESSAGE);
+      });
+      mockMessageChannel.send.$replay();
+      const requester = new Requester(REQUESTER_NAME, mockMessageChannel);
+
+      try {
+        await requester.postRequest({});
+      } catch (e) {
+        // This is the expected branch.
+        return;
+      }
+      fail('Message posting unexpectedly succeeded');
+    });
 });  // goog.scope
diff --git a/common/js/src/requesting/requester.js b/common/js/src/requesting/requester.js
index 35bba3ff6..aa28ea6db 100644
--- a/common/js/src/requesting/requester.js
+++ b/common/js/src/requesting/requester.js
@@ -136,8 +136,11 @@ GSC.Requester = class extends goog.Disposable {
       this.messageChannel_.send(serviceName, messageData);
     } catch (e) {
       // If sending the message triggered an exception, use it to populate the
-      // response.
-      this.rejectRequest_(requestId, e);
+      // response. However, this is already done if the channel got immediately
+      // disposed of, since `disposeInternal()` cancels all ongoing requests.
+      if (!this.isDisposed()) {
+        this.rejectRequest_(requestId, e);
+      }
     }
 
     return promiseResolver.promise;