Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[browser] dispose more JS proxies #89559

Merged
merged 10 commits into from
Jul 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -233,12 +233,13 @@ 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 (Exception)
{
// this would also trigger abort
abortRegistration?.Dispose();
abortController?.Dispose();
throw;
}
}
Expand Down Expand Up @@ -313,15 +314,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);
pavelsavara marked this conversation as resolved.
Show resolved Hide resolved
ArgumentNullException.ThrowIfNull(abortController);

FetchResponse = fetchResponse;
_abortRegistration = abortRegistration;
_abortController = abortController;
}

public void ThrowIfDisposed()
Expand Down Expand Up @@ -350,6 +354,7 @@ public void Dispose()
return;
self._isDisposed = true;
self._abortRegistration.Dispose();
self._abortController.Dispose();
if (!self.FetchResponse!.IsDisposed)
{
BrowserHttpInterop.AbortResponse(self.FetchResponse);
Expand All @@ -362,6 +367,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