From 4cc9e2e3255594fc9ded57a48ac4abcbac8e2732 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= <kamil.ogorek@gmail.com>
Date: Sat, 14 Jan 2023 15:06:28 +0100
Subject: [PATCH] fix(ext/flash): Correctly handle errors for chunked responses
 (#17303)

The leading cause of the problem was that `handleResponse` has
`tryRespondChunked` passed as an argument, which in turn is implemented
as a call to `core.ops.op_try_flash_respond_chuncked`, that throws in
the repro code.

`handleResponse` was not handled correctly, as it not returned any
value, and had no `catch` attached to it.
It also effectively was never correctly handled inside two other blocks
with `resp.then` and `PromisePrototypeCatch(PromisePrototypeThen(resp,
"..."))` as well, as it just short-circuited the promise with an empty
resolve, instead of relying on the last `(async () => {})` block.

This change makes `handleResponse` return a correct value and attach
`onError` handler to the "non-thenable" variant of response handling
code.
---
 cli/tests/unit/flash_test.ts | 46 ++++++++++++++++++++++++++++++++++++
 ext/flash/01_http.js         | 30 +++++++++++------------
 2 files changed, 60 insertions(+), 16 deletions(-)

diff --git a/cli/tests/unit/flash_test.ts b/cli/tests/unit/flash_test.ts
index 9491e4534c..5604f6b5fc 100644
--- a/cli/tests/unit/flash_test.ts
+++ b/cli/tests/unit/flash_test.ts
@@ -12,6 +12,7 @@ import {
   assert,
   assertEquals,
   assertRejects,
+  assertStringIncludes,
   assertThrows,
   Deferred,
   deferred,
@@ -495,6 +496,51 @@ Deno.test(
   },
 );
 
+// https://github.com/denoland/deno/issues/17291
+Deno.test(
+  { permissions: { net: true } },
+  async function httpServerIncorrectChunkedResponse() {
+    const ac = new AbortController();
+    const listeningPromise = deferred();
+    const errorPromise = deferred();
+    const server = Deno.serve({
+      handler: () => {
+        const body = new ReadableStream({
+          start(controller) {
+            // Non-encoded string is not a valid readable chunk.
+            controller.enqueue("wat");
+          },
+        });
+        return new Response(body);
+      },
+      port: 4501,
+      signal: ac.signal,
+      onListen: onListen(listeningPromise),
+      onError: (err) => {
+        const errResp = new Response(
+          `Internal server error: ${(err as Error).message}`,
+          { status: 500 },
+        );
+        ac.abort();
+        errorPromise.resolve(errResp);
+        return errResp;
+      },
+    });
+
+    await listeningPromise;
+
+    const resp = await fetch("http://127.0.0.1:4501/");
+    // Incorrectly implemented reader ReadableStream should reject.
+    await assertRejects(() => resp.body!.getReader().read());
+
+    const err = await errorPromise as Response;
+    assertStringIncludes(await err.text(), "Expected ArrayBufferView");
+
+    ac.abort();
+    await server;
+  },
+);
+
 Deno.test(
   { permissions: { net: true } },
   async function httpServerCorrectLengthForUnicodeString() {
diff --git a/ext/flash/01_http.js b/ext/flash/01_http.js
index e76914e728..e76f4de528 100644
--- a/ext/flash/01_http.js
+++ b/ext/flash/01_http.js
@@ -327,7 +327,7 @@
       );
     }
 
-    (async () => {
+    return (async () => {
       if (!ws) {
         if (hasBody && body[_state] !== "closed") {
           // TODO(@littledivy): Optimize by draining in a single op.
@@ -590,7 +590,6 @@
                     ),
                     onError,
                   );
-                  continue;
                 } else if (typeof resp?.then === "function") {
                   resp.then((resp) =>
                     handleResponse(
@@ -606,24 +605,23 @@
                       tryRespondChunked,
                     )
                   ).catch(onError);
-                  continue;
+                } else {
+                  handleResponse(
+                    req,
+                    resp,
+                    body,
+                    hasBody,
+                    method,
+                    serverId,
+                    i,
+                    respondFast,
+                    respondChunked,
+                    tryRespondChunked,
+                  ).catch(onError);
                 }
               } catch (e) {
                 resp = await onError(e);
               }
-
-              handleResponse(
-                req,
-                resp,
-                body,
-                hasBody,
-                method,
-                serverId,
-                i,
-                respondFast,
-                respondChunked,
-                tryRespondChunked,
-              );
             }
 
             offset += tokens;