From 7e12dc78f2ea7c1b64dcd23804ae7b1d64a62fb4 Mon Sep 17 00:00:00 2001 From: Pavel Savara Date: Fri, 28 Jul 2023 22:56:05 +0200 Subject: [PATCH] [browser] dispose more JS proxies (#89559) --- .../Http/BrowserHttpHandler/BrowserHttpHandler.cs | 10 ++++- .../tests/FunctionalTests/MetricsTest.cs | 1 + .../InteropServices/JavaScript/JSException.cs | 30 +++++++++++---- .../JavaScript/JSHostImplementation.cs | 43 ++++++++++++++-------- src/mono/wasm/runtime/globals.ts | 9 +---- src/mono/wasm/runtime/loader/globals.ts | 29 +++++++-------- src/mono/wasm/runtime/marshal-to-js.ts | 7 +++- src/mono/wasm/runtime/startup.ts | 7 +++- src/mono/wasm/runtime/types/internal.ts | 2 +- src/mono/wasm/runtime/web-socket.ts | 42 +++++++++++++++------ 10 files changed, 117 insertions(+), 63 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs b/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs index 73cb9f7..2cf419e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs @@ -233,7 +233,7 @@ namespace System.Net.Http cancellationToken.ThrowIfCancellationRequested(); JSObject fetchResponse = await BrowserHttpInterop.CancelationHelper(promise, cancellationToken, abortController, null).ConfigureAwait(true); - return new WasmFetchResponse(fetchResponse, abortRegistration.Value); + return new WasmFetchResponse(fetchResponse, abortController, abortRegistration.Value); } catch (JSException jse) { @@ -243,6 +243,7 @@ namespace System.Net.Http { // this would also trigger abort abortRegistration?.Dispose(); + abortController?.Dispose(); throw; } } @@ -317,15 +318,18 @@ namespace System.Net.Http public readonly object ThisLock = new object(); #endif public JSObject? FetchResponse; + private readonly JSObject _abortController; private readonly CancellationTokenRegistration _abortRegistration; private bool _isDisposed; - public WasmFetchResponse(JSObject fetchResponse, CancellationTokenRegistration abortRegistration) + public WasmFetchResponse(JSObject fetchResponse, JSObject abortController, CancellationTokenRegistration abortRegistration) { ArgumentNullException.ThrowIfNull(fetchResponse); + ArgumentNullException.ThrowIfNull(abortController); FetchResponse = fetchResponse; _abortRegistration = abortRegistration; + _abortController = abortController; } public void ThrowIfDisposed() @@ -354,6 +358,7 @@ namespace System.Net.Http return; self._isDisposed = true; self._abortRegistration.Dispose(); + self._abortController.Dispose(); if (!self.FetchResponse!.IsDisposed) { BrowserHttpInterop.AbortResponse(self.FetchResponse); @@ -366,6 +371,7 @@ namespace System.Net.Http #else _isDisposed = true; _abortRegistration.Dispose(); + _abortController.Dispose(); if (FetchResponse != null) { if (!FetchResponse.IsDisposed) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs index 746feb4..111957c 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/MetricsTest.cs @@ -615,6 +615,7 @@ namespace System.Net.Http.Functional.Tests } [Fact] + [SkipOnPlatform(TestPlatforms.Browser, "Browser is relaxed about validating HTTP headers")] public async Task FailedRequests_ConnectionClosedWhileReceivingHeaders_Recorded() { using CancellationTokenSource cancelServerCts = new CancellationTokenSource(); diff --git a/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSException.cs b/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSException.cs index c2cf295..f06834b 100644 --- a/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSException.cs +++ b/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSException.cs @@ -13,6 +13,7 @@ namespace System.Runtime.InteropServices.JavaScript public sealed class JSException : Exception { internal JSObject? jsException; + internal string? combinedStackTrace; /// /// Initializes a new instance of the JSException class with a specified error message. @@ -21,11 +22,13 @@ namespace System.Runtime.InteropServices.JavaScript public JSException(string msg) : base(msg) { jsException = null; + combinedStackTrace = null; } internal JSException(string msg, JSObject? jsException) : base(msg) { this.jsException = jsException; + this.combinedStackTrace = null; } /// @@ -33,6 +36,10 @@ namespace System.Runtime.InteropServices.JavaScript { get { + if (combinedStackTrace != null) + { + return combinedStackTrace; + } var bs = base.StackTrace; if (jsException == null) { @@ -42,16 +49,19 @@ namespace System.Runtime.InteropServices.JavaScript #if FEATURE_WASM_THREADS if (jsException.OwnerThreadId != Thread.CurrentThread.ManagedThreadId) { - return null; + return bs; } #endif string? jsStackTrace = jsException.GetPropertyAsString("stack"); + + // after this, we don't need jsException proxy anymore + jsException.Dispose(); + jsException = null; + if (jsStackTrace == null) { - if (bs == null) - { - return null; - } + combinedStackTrace = bs; + return combinedStackTrace; } else if (jsStackTrace.StartsWith(Message + "\n")) { @@ -62,11 +72,15 @@ namespace System.Runtime.InteropServices.JavaScript if (bs == null) { - return jsStackTrace; + combinedStackTrace = jsStackTrace; } - return base.StackTrace + "\r\n" + jsStackTrace; - } + combinedStackTrace = bs != null + ? bs + Environment.NewLine + jsStackTrace + : jsStackTrace; + + return combinedStackTrace; + } } /// diff --git a/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs b/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs index 94707e6..248740b 100644 --- a/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs +++ b/src/libraries/System.Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs @@ -275,20 +275,6 @@ namespace System.Runtime.InteropServices.JavaScript jso.Dispose(); } } - foreach (var gch in ThreadJsOwnedObjects.Values) - { - GCHandle gcHandle = (GCHandle)gch; - - // if this is pending promise we reject it - if (gcHandle.Target is TaskCallback holder) - { - unsafe - { - holder.Callback!.Invoke(null); - } - } - gcHandle.Free(); - } SynchronizationContext.SetSynchronizationContext(ctx!.previousSynchronizationContext); JSSynchronizationContext.CurrentJSSynchronizationContext = null; ctx.isDisposed = true; @@ -309,9 +295,36 @@ namespace System.Runtime.InteropServices.JavaScript Environment.FailFast($"There should be no JS proxies of managed objects on this thread, ManagedThreadId: {Thread.CurrentThread.ManagedThreadId}"); } } + + Interop.Runtime.UninstallWebWorkerInterop(uninstallJSSynchronizationContext); + + if (uninstallJSSynchronizationContext) + { + try + { + foreach (var gch in ThreadJsOwnedObjects.Values) + { + GCHandle gcHandle = (GCHandle)gch; + + // if this is pending promise we reject it + if (gcHandle.Target is TaskCallback holder) + { + unsafe + { + holder.Callback!.Invoke(null); + } + } + gcHandle.Free(); + } + } + catch(Exception ex) + { + Environment.FailFast($"Unexpected error in UninstallWebWorkerInterop, ManagedThreadId: {Thread.CurrentThread.ManagedThreadId}. " + ex); + } + } + ThreadCsOwnedObjects.Clear(); ThreadJsOwnedObjects.Clear(); - Interop.Runtime.UninstallWebWorkerInterop(uninstallJSSynchronizationContext); } private static FieldInfo? thread_id_Field; diff --git a/src/mono/wasm/runtime/globals.ts b/src/mono/wasm/runtime/globals.ts index 9ca3d5d..2443e20 100644 --- a/src/mono/wasm/runtime/globals.ts +++ b/src/mono/wasm/runtime/globals.ts @@ -5,7 +5,6 @@ /// /// -import { mono_log_error } from "./logging"; import { RuntimeAPI } from "./types/index"; import type { GlobalObjects, EmscriptenInternals, RuntimeHelpers, LoaderHelpers, DotnetModuleInternal, PromiseAndController } from "./types/internal"; @@ -87,10 +86,6 @@ export function mono_assert(condition: unknown, messageFactory: string | (() => const message = "Assert failed: " + (typeof messageFactory === "function" ? messageFactory() : messageFactory); - const abort = runtimeHelpers.mono_wasm_abort; - if (abort) { - mono_log_error(message); - abort(); - } - throw new Error(message); + const error = new Error(message); + runtimeHelpers.abort(error); } \ No newline at end of file diff --git a/src/mono/wasm/runtime/loader/globals.ts b/src/mono/wasm/runtime/loader/globals.ts index 5f4e833..8905bc00 100644 --- a/src/mono/wasm/runtime/loader/globals.ts +++ b/src/mono/wasm/runtime/loader/globals.ts @@ -8,7 +8,7 @@ import type { MonoConfig, RuntimeAPI } from "../types"; import { assert_runtime_running, is_exited, is_runtime_running, mono_exit } from "./exit"; import { assertIsControllablePromise, createPromiseController, getPromiseController } from "./promise-controller"; import { mono_download_assets, resolve_asset_path } from "./assets"; -import { mono_log_error, setup_proxy_console } from "./logging"; +import { setup_proxy_console } from "./logging"; import { hasDebuggingEnabled } from "./blazor/_Polyfill"; import { invokeLibraryInitializers } from "./libraryInitializers"; @@ -17,20 +17,20 @@ export const ENVIRONMENT_IS_WEB = typeof window == "object"; export const ENVIRONMENT_IS_WORKER = typeof importScripts == "function"; export const ENVIRONMENT_IS_SHELL = !ENVIRONMENT_IS_WEB && !ENVIRONMENT_IS_NODE && !ENVIRONMENT_IS_WORKER; -export let runtimeHelpers: RuntimeHelpers = null as any; -export let loaderHelpers: LoaderHelpers = null as any; -export let exportedRuntimeAPI: RuntimeAPI = null as any; -export let INTERNAL: any; +export let runtimeHelpers: RuntimeHelpers = {} as any; +export let loaderHelpers: LoaderHelpers = {} as any; +export let exportedRuntimeAPI: RuntimeAPI = {} as any; +export let INTERNAL: any = {}; export let _loaderModuleLoaded = false; // please keep it in place also as rollup guard export const globalObjectsRoot: GlobalObjects = { mono: {}, binding: {}, - internal: {}, + internal: INTERNAL, module: {}, - loaderHelpers: {}, - runtimeHelpers: {}, - api: {} + loaderHelpers, + runtimeHelpers, + api: exportedRuntimeAPI, } as any; setLoaderGlobals(globalObjectsRoot); @@ -59,7 +59,8 @@ export function setLoaderGlobals( mono_wasm_bindings_is_ready: false, javaScriptExports: {} as any, config: globalObjects.module.config, - diagnosticTracing: false + diagnosticTracing: false, + abort: (reason: any) => { throw reason; }, }); Object.assign(loaderHelpers, { config: globalObjects.module.config, @@ -111,10 +112,6 @@ export function mono_assert(condition: unknown, messageFactory: string | (() => const message = "Assert failed: " + (typeof messageFactory === "function" ? messageFactory() : messageFactory); - const abort = globalObjectsRoot.runtimeHelpers.mono_wasm_abort; - if (abort) { - mono_log_error(message); - abort(); - } - throw new Error(message); + const error = new Error(message); + runtimeHelpers.abort(error); } \ No newline at end of file diff --git a/src/mono/wasm/runtime/marshal-to-js.ts b/src/mono/wasm/runtime/marshal-to-js.ts index d861cd4..9b5a931 100644 --- a/src/mono/wasm/runtime/marshal-to-js.ts +++ b/src/mono/wasm/runtime/marshal-to-js.ts @@ -5,7 +5,7 @@ import MonoWasmThreads from "consts:monoWasmThreads"; import BuildConfiguration from "consts:configuration"; import cwraps from "./cwraps"; -import { _lookup_js_owned_object, mono_wasm_get_jsobj_from_js_handle, mono_wasm_get_js_handle, setup_managed_proxy } from "./gc-handles"; +import { _lookup_js_owned_object, mono_wasm_get_jsobj_from_js_handle, mono_wasm_get_js_handle, setup_managed_proxy, teardown_managed_proxy } from "./gc-handles"; import { Module, createPromiseController, loaderHelpers, mono_assert, runtimeHelpers } from "./globals"; import { ManagedObject, ManagedError, @@ -197,7 +197,10 @@ function _marshal_delegate_to_js(arg: JSMarshalerArgument, _?: MarshalerType, re return runtimeHelpers.javaScriptExports.call_delegate(gc_handle, arg1_js, arg2_js, arg3_js, res_converter, arg1_converter, arg2_converter, arg3_converter); }; result.dispose = () => { - result.isDisposed = true; + if (!result.isDisposed) { + result.isDisposed = true; + teardown_managed_proxy(result, gc_handle); + } }; result.isDisposed = false; if (BuildConfiguration === "Debug") { diff --git a/src/mono/wasm/runtime/startup.ts b/src/mono/wasm/runtime/startup.ts index 12e8341..be65c39 100644 --- a/src/mono/wasm/runtime/startup.ts +++ b/src/mono/wasm/runtime/startup.ts @@ -344,7 +344,12 @@ function mono_wasm_pre_init_essential(isWorker: boolean): void { init_c_exports(); runtimeHelpers.mono_wasm_exit = cwraps.mono_wasm_exit; - runtimeHelpers.mono_wasm_abort = cwraps.mono_wasm_abort; + runtimeHelpers.abort = (reason: any) => { + if (!loaderHelpers.is_exited()) { + cwraps.mono_wasm_abort(); + } + throw reason; + }; cwraps_internal(INTERNAL); if (WasmEnableLegacyJsInterop && !linkerDisableLegacyJsInterop) { cwraps_mono_api(MONO); diff --git a/src/mono/wasm/runtime/types/internal.ts b/src/mono/wasm/runtime/types/internal.ts index 3b54d6b1..66e7fb4 100644 --- a/src/mono/wasm/runtime/types/internal.ts +++ b/src/mono/wasm/runtime/types/internal.ts @@ -174,7 +174,7 @@ export type RuntimeHelpers = { ExitStatus: ExitStatusError; quit: Function, mono_wasm_exit?: (code: number) => void, - mono_wasm_abort?: () => void, + abort: (reason: any) => void, javaScriptExports: JavaScriptExports, storeMemorySnapshotPending: boolean, memorySnapshotCacheKey: string, diff --git a/src/mono/wasm/runtime/web-socket.ts b/src/mono/wasm/runtime/web-socket.ts index b085cfe..b5de8be 100644 --- a/src/mono/wasm/runtime/web-socket.ts +++ b/src/mono/wasm/runtime/web-socket.ts @@ -11,6 +11,7 @@ import { VoidPtr } from "./types/emscripten"; import { PromiseController } from "./types/internal"; import { mono_log_warn } from "./logging"; import { viewOrCopy, utf8ToStringRelaxed, stringToUTF8 } from "./strings"; +import { IDisposable } from "./marshal"; const wasm_ws_pending_send_buffer = Symbol.for("wasm ws_pending_send_buffer"); const wasm_ws_pending_send_buffer_offset = Symbol.for("wasm ws_pending_send_buffer_offset"); @@ -21,6 +22,7 @@ const wasm_ws_pending_open_promise = Symbol.for("wasm ws_pending_open_promise"); const wasm_ws_pending_close_promises = Symbol.for("wasm ws_pending_close_promises"); const wasm_ws_pending_send_promises = Symbol.for("wasm ws_pending_send_promises"); const wasm_ws_is_aborted = Symbol.for("wasm ws_is_aborted"); +const wasm_ws_on_closed = Symbol.for("wasm ws_on_closed"); const wasm_ws_receive_status_ptr = Symbol.for("wasm ws_receive_status_ptr"); let mono_wasm_web_socket_close_warning = false; const ws_send_buffer_blocking_threshold = 65536; @@ -41,6 +43,7 @@ function verifyEnvironment() { export function ws_wasm_create(uri: string, sub_protocols: string[] | null, receive_status_ptr: VoidPtr, onClosed: (code: number, reason: string) => void): WebSocketExtension { verifyEnvironment(); mono_assert(uri && typeof uri === "string", () => `ERR12: Invalid uri ${typeof uri}`); + mono_assert(typeof onClosed === "function", () => `ERR12: Invalid onClosed ${typeof onClosed}`); const ws = new globalThis.WebSocket(uri, sub_protocols || undefined) as WebSocketExtension; const { promise_control: open_promise_control } = createPromiseController(); @@ -51,6 +54,7 @@ export function ws_wasm_create(uri: string, sub_protocols: string[] | null, rece ws[wasm_ws_pending_send_promises] = []; ws[wasm_ws_pending_close_promises] = []; ws[wasm_ws_receive_status_ptr] = receive_status_ptr; + ws[wasm_ws_on_closed] = onClosed as any; ws.binaryType = "arraybuffer"; const local_on_open = () => { if (ws[wasm_ws_is_aborted]) return; @@ -65,10 +69,11 @@ export function ws_wasm_create(uri: string, sub_protocols: string[] | null, rece const local_on_close = (ev: CloseEvent) => { ws.removeEventListener("message", local_on_message); if (ws[wasm_ws_is_aborted]) return; - if (onClosed) onClosed(ev.code, ev.reason); + + onClosed(ev.code, ev.reason); // this reject would not do anything if there was already "open" before it. - open_promise_control.reject(ev.reason); + open_promise_control.reject(new Error(ev.reason)); for (const close_promise_control of ws[wasm_ws_pending_close_promises]) { close_promise_control.resolve(); @@ -82,9 +87,16 @@ export function ws_wasm_create(uri: string, sub_protocols: string[] | null, rece setI32(receive_status_ptr + 8, 1);// end_of_message: true receive_promise_control.resolve(); }); + + // cleanup the delegate proxy + ws[wasm_ws_on_closed].dispose(); }; const local_on_error = (ev: any) => { - open_promise_control.reject(ev.message || "WebSocket error"); + if (ws[wasm_ws_is_aborted]) return; + ws.removeEventListener("message", local_on_message); + const error = new Error(ev.message || "WebSocket error"); + mono_log_warn("WebSocket error", error); + reject_promises(ws, error); }; ws.addEventListener("message", local_on_message); ws.addEventListener("open", local_on_open, { once: true }); @@ -177,23 +189,30 @@ export function ws_wasm_abort(ws: WebSocketExtension): void { mono_assert(!!ws, "ERR18: expected ws instance"); ws[wasm_ws_is_aborted] = true; + reject_promises(ws, new Error("OperationCanceledException")); + + // cleanup the delegate proxy + ws[wasm_ws_on_closed]?.dispose(); + + // this is different from Managed implementation + ws.close(1000, "Connection was aborted."); +} + +function reject_promises(ws: WebSocketExtension, error: Error) { const open_promise_control = ws[wasm_ws_pending_open_promise]; if (open_promise_control) { - open_promise_control.reject(new Error("OperationCanceledException")); + open_promise_control.reject(error); } for (const close_promise_control of ws[wasm_ws_pending_close_promises]) { - close_promise_control.reject(new Error("OperationCanceledException")); + close_promise_control.reject(error); } for (const send_promise_control of ws[wasm_ws_pending_send_promises]) { - send_promise_control.reject(new Error("OperationCanceledException")); + send_promise_control.reject(error); } ws[wasm_ws_pending_receive_promise_queue].drain(receive_promise_control => { - receive_promise_control.reject(new Error("OperationCanceledException")); + receive_promise_control.reject(error); }); - - // this is different from Managed implementation - ws.close(1000, "Connection was aborted."); } // send and return promise @@ -224,7 +243,7 @@ function _mono_wasm_web_socket_send_and_wait(ws: WebSocketExtension, buffer_view if (readyState != WebSocket.OPEN && readyState != WebSocket.CLOSING) { // only reject if the data were not sent // bufferedAmount does not reset to zero once the connection closes - promise_control.reject(`InvalidState: ${readyState} The WebSocket is not connected.`); + promise_control.reject(new Error(`InvalidState: ${readyState} The WebSocket is not connected.`)); } else if (!promise_control.isDone) { globalThis.setTimeout(polling_check, nextDelay); @@ -372,6 +391,7 @@ type WebSocketExtension = WebSocket & { [wasm_ws_pending_send_promises]: PromiseController[] [wasm_ws_pending_close_promises]: PromiseController[] [wasm_ws_is_aborted]: boolean + [wasm_ws_on_closed]: IDisposable [wasm_ws_receive_status_ptr]: VoidPtr [wasm_ws_pending_send_buffer_offset]: number [wasm_ws_pending_send_buffer_type]: number -- 2.7.4