Skip to content

Commit

Permalink
[browser] dispose more JS proxies (#89559)
Browse files Browse the repository at this point in the history
  • Loading branch information
pavelsavara committed Jul 28, 2023
1 parent 7bfe6ee commit 7e12dc7
Show file tree
Hide file tree
Showing 10 changed files with 117 additions and 63 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ private static async Task<WasmFetchResponse> CallFetch(HttpRequestMessage reques

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)
{
Expand All @@ -243,6 +243,7 @@ private static async Task<WasmFetchResponse> CallFetch(HttpRequestMessage reques
{
// this would also trigger abort
abortRegistration?.Dispose();
abortController?.Dispose();
throw;
}
}
Expand Down Expand Up @@ -317,15 +318,18 @@ internal sealed class WasmFetchResponse : IDisposable
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()
Expand Down Expand Up @@ -354,6 +358,7 @@ public void Dispose()
return;
self._isDisposed = true;
self._abortRegistration.Dispose();
self._abortController.Dispose();
if (!self.FetchResponse!.IsDisposed)
{
BrowserHttpInterop.AbortResponse(self.FetchResponse);
Expand All @@ -366,6 +371,7 @@ public void Dispose()
#else
_isDisposed = true;
_abortRegistration.Dispose();
_abortController.Dispose();
if (FetchResponse != null)
{
if (!FetchResponse.IsDisposed)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,7 @@ await Assert.ThrowsAsync<HttpRequestException>(async () =>
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "Browser is relaxed about validating HTTP headers")]
public async Task FailedRequests_ConnectionClosedWhileReceivingHeaders_Recorded()
{
using CancellationTokenSource cancelServerCts = new CancellationTokenSource();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ namespace System.Runtime.InteropServices.JavaScript
public sealed class JSException : Exception
{
internal JSObject? jsException;
internal string? combinedStackTrace;

/// <summary>
/// Initializes a new instance of the JSException class with a specified error message.
Expand All @@ -21,18 +22,24 @@ public sealed class JSException : Exception
public JSException(string msg) : base(msg)
{
jsException = null;
combinedStackTrace = null;
}

internal JSException(string msg, JSObject? jsException) : base(msg)
{
this.jsException = jsException;
this.combinedStackTrace = null;
}

/// <inheritdoc />
public override string? StackTrace
{
get
{
if (combinedStackTrace != null)
{
return combinedStackTrace;
}
var bs = base.StackTrace;
if (jsException == null)
{
Expand All @@ -42,16 +49,19 @@ public override string? StackTrace
#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"))
{
Expand All @@ -62,11 +72,15 @@ public override string? StackTrace

if (bs == null)
{
return jsStackTrace;
combinedStackTrace = jsStackTrace;
}
return base.StackTrace + "\r\n" + jsStackTrace;
}

combinedStackTrace = bs != null
? bs + Environment.NewLine + jsStackTrace
: jsStackTrace;

return combinedStackTrace;
}
}

/// <inheritdoc />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,20 +275,6 @@ public static void UninstallWebWorkerInterop()
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;
Expand All @@ -309,9 +295,36 @@ public static void UninstallWebWorkerInterop()
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;
Expand Down
9 changes: 2 additions & 7 deletions src/mono/wasm/runtime/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
/// <reference path="./types/v8.d.ts" />
/// <reference path="./types/node.d.ts" />

import { mono_log_error } from "./logging";
import { RuntimeAPI } from "./types/index";
import type { GlobalObjects, EmscriptenInternals, RuntimeHelpers, LoaderHelpers, DotnetModuleInternal, PromiseAndController } from "./types/internal";

Expand Down Expand Up @@ -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);
}
29 changes: 13 additions & 16 deletions src/mono/wasm/runtime/loader/globals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand All @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
}
7 changes: 5 additions & 2 deletions src/mono/wasm/runtime/marshal-to-js.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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") {
Expand Down
7 changes: 6 additions & 1 deletion src/mono/wasm/runtime/startup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/mono/wasm/runtime/types/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 7e12dc7

Please sign in to comment.