From 93b3ff017078b2c1e993457ef43af6b52e715ba6 Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Thu, 29 Jun 2023 07:24:01 -0600 Subject: [PATCH] fix(ext/websocket): Ensure that errors are available after async response returns (#19642) Fixes the WPT tests that test w/invalid codes. Also explicitly ignoring some h2 tests to hopefully prevent flakes. The previous changes to WebSocketStream introduced a bug where the close errors were not made available if the `pull` method was re-entrant. --- ext/websocket/01_websocket.js | 5 +++-- ext/websocket/02_websocketstream.js | 12 +++++----- ext/websocket/lib.rs | 4 +--- tools/wpt.ts | 8 +++++-- tools/wpt/expectation.json | 34 ++++++----------------------- tools/wpt/runner.ts | 1 - 6 files changed, 24 insertions(+), 40 deletions(-) diff --git a/ext/websocket/01_websocket.js b/ext/websocket/01_websocket.js index e6b0053c63..50e110444b 100644 --- a/ext/websocket/01_websocket.js +++ b/ext/websocket/01_websocket.js @@ -467,6 +467,7 @@ class WebSocket extends EventTarget { default: { /* close */ const code = kind; + const reason = code == 1005 ? "" : op_ws_get_error(rid); const prevState = this[_readyState]; this[_readyState] = CLOSED; clearTimeout(this[_idleTimeoutTimeout]); @@ -476,7 +477,7 @@ class WebSocket extends EventTarget { await op_ws_close( rid, code, - op_ws_get_error(rid), + reason, ); } catch { // ignore failures @@ -486,7 +487,7 @@ class WebSocket extends EventTarget { const event = new CloseEvent("close", { wasClean: true, code: code, - reason: op_ws_get_error(rid), + reason, }); this.dispatchEvent(event); core.tryClose(rid); diff --git a/ext/websocket/02_websocketstream.js b/ext/websocket/02_websocketstream.js index 068fa3e1b9..2810252896 100644 --- a/ext/websocket/02_websocketstream.js +++ b/ext/websocket/02_websocketstream.js @@ -242,8 +242,8 @@ class WebSocketStream { }, }); const pull = async (controller) => { + // Remember that this pull method may be re-entered before it has completed const kind = await op_ws_next_event(this[_rid]); - switch (kind) { case 0: /* string */ @@ -266,17 +266,18 @@ class WebSocketStream { core.tryClose(this[_rid]); break; } - case 4: { + case 1005: { /* closed */ - this[_closed].resolve(undefined); + this[_closed].resolve({ code: 1005, reason: "" }); core.tryClose(this[_rid]); break; } default: { /* close */ + const reason = op_ws_get_error(this[_rid]); this[_closed].resolve({ code: kind, - reason: op_ws_get_error(this[_rid]), + reason, }); core.tryClose(this[_rid]); break; @@ -294,7 +295,8 @@ class WebSocketStream { return pull(controller); } - this[_closed].resolve(op_ws_get_error(this[_rid])); + const error = op_ws_get_error(this[_rid]); + this[_closed].reject(new Error(error)); core.tryClose(this[_rid]); } }; diff --git a/ext/websocket/lib.rs b/ext/websocket/lib.rs index dd975589c3..a8d18b4ad6 100644 --- a/ext/websocket/lib.rs +++ b/ext/websocket/lib.rs @@ -591,10 +591,8 @@ pub async fn op_ws_next_event( Ok(val) => val, Err(err) => { // No message was received, socket closed while we waited. - // Try close the stream, ignoring any errors, and report closed status to JavaScript. + // Report closed status to JavaScript. if resource.closed.get() { - let _ = state.borrow_mut().resource_table.close(rid); - resource.set_error(None); return MessageKind::ClosedDefault as u16; } diff --git a/tools/wpt.ts b/tools/wpt.ts index fe2a350b27..29d3e3517e 100755 --- a/tools/wpt.ts +++ b/tools/wpt.ts @@ -567,8 +567,9 @@ function analyzeTestResult( function reportVariation(result: TestResult, expectation: boolean | string[]) { if (result.status !== 0 || result.harnessStatus === null) { - console.log(`test stderr:`); - writeAllSync(Deno.stdout, new TextEncoder().encode(result.stderr)); + if (result.stderr) { + console.log(`test stderr:\n${result.stderr}\n`); + } const expectFail = expectation === false; const failReason = result.status !== 0 @@ -611,6 +612,9 @@ function reportVariation(result: TestResult, expectation: boolean | string[]) { for (const result of expectedFailedButPassed) { console.log(` ${JSON.stringify(result.name)}`); } + if (result.stderr) { + console.log("\ntest stderr:\n" + result.stderr); + } console.log( `\nfile result: ${ failedCount > 0 ? red("failed") : green("ok") diff --git a/tools/wpt/expectation.json b/tools/wpt/expectation.json index 9cf0a7af16..5353721793 100644 --- a/tools/wpt/expectation.json +++ b/tools/wpt/expectation.json @@ -6945,18 +6945,6 @@ "Create-extensions-empty.any.worker.html": true, "Create-extensions-empty.any.worker.html?wpt_flags=h2": false, "Create-extensions-empty.any.worker.html?wss": true, - "Create-invalid-urls.any.html": true, - "Create-invalid-urls.any.html?wpt_flags=h2": true, - "Create-invalid-urls.any.html?wss": true, - "Create-invalid-urls.any.worker.html": true, - "Create-invalid-urls.any.worker.html?wpt_flags=h2": true, - "Create-invalid-urls.any.worker.html?wss": true, - "Create-non-absolute-url.any.html": false, - "Create-non-absolute-url.any.html?wpt_flags=h2": true, - "Create-non-absolute-url.any.html?wss": true, - "Create-non-absolute-url.any.worker.html": false, - "Create-non-absolute-url.any.worker.html?wpt_flags=h2": true, - "Create-non-absolute-url.any.worker.html?wss": true, "Create-nonAscii-protocol-string.any.html": true, "Create-nonAscii-protocol-string.any.html?wpt_flags=h2": true, "Create-nonAscii-protocol-string.any.html?wss": true, @@ -7030,12 +7018,6 @@ "Create-valid-url.any.worker.html": true, "Create-valid-url.any.worker.html?wpt_flags=h2": false, "Create-valid-url.any.worker.html?wss": true, - "Create-wrong-scheme.any.html": true, - "Create-wrong-scheme.any.html?wpt_flags=h2": true, - "Create-wrong-scheme.any.html?wss": true, - "Create-wrong-scheme.any.worker.html": true, - "Create-wrong-scheme.any.worker.html?wpt_flags=h2": true, - "Create-wrong-scheme.any.worker.html?wss": true, "Send-0byte-data.any.html": true, "Send-0byte-data.any.html?wpt_flags=h2": false, "Send-0byte-data.any.html?wss": true, @@ -7212,19 +7194,17 @@ "close.any.html?wpt_flags=h2": false, "close.any.html?wss": true, "close.any.worker.html?wpt_flags=h2": false, - "close.any.worker.html?wss": { - "ignore": true - }, + "close.any.worker.html?wss": true, "constructor.any.html?wpt_flags=h2": false, "constructor.any.html?wss": true, "constructor.any.worker.html?wpt_flags=h2": false, "constructor.any.worker.html?wss": true, - "abort.any.html?wpt_flags=h2": [ - "abort after connect should do nothing" - ], - "abort.any.worker.html?wpt_flags=h2": [ - "abort after connect should do nothing" - ], + "abort.any.html?wpt_flags=h2": { + "ignore": true + }, + "abort.any.worker.html?wpt_flags=h2": { + "ignore": true + }, "backpressure-receive.any.worker.html?wpt_flags=h2": false } }, diff --git a/tools/wpt/runner.ts b/tools/wpt/runner.ts index dffeacae1e..90898ae6ff 100644 --- a/tools/wpt/runner.ts +++ b/tools/wpt/runner.ts @@ -154,7 +154,6 @@ export async function runSingleTest( harnessStatus = JSON.parse(line.slice(5)); } else { stderr += line + "\n"; - console.error(line); } }