From 75cae3ada7e07f07eb577c8da78e23eefcdf6d2c Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 14:07:37 -0700 Subject: [PATCH 1/8] Make JSReference finalizer thread-safe to fix worker-teardown crash (exit 139) JSReference.Finalize() runs Dispose(disposing: false) on the GC finalizer thread, which has no active JS value scope. For a reference created from a no-context scope (as the native host does), Dispose called ThrowIfInvalidThreadAccess(), which reads JSValueScope.Current and throws JSInvalidThreadAccessException ("There is no active JS value scope"). An exception escaping a finalizer is fatal, so a worker whose orphaned references are GC-finalized after its scope is torn down crashes the whole process with SIGSEGV (exit 139 on Node < 24.14): JSInvalidThreadAccessException: There is no active JS value scope. at JSValueScope.get_Current() at JSReference.ThrowIfInvalidThreadAccess() at JSReference.Dispose(Boolean) at JSReference.Finalize() Split the finalizer path so it never throws: - Explicit Dispose() keeps its documented behavior (still asserts thread access for a no-context reference). - The finalizer releases the native reference only when it can be done safely: a no-context reference is deleted only if the matching JS scope happens to be current on the thread, otherwise the release is skipped (the environment is being torn down and the reference is released with it); a context reference defers the delete to the JS thread via the synchronization context, which is a safe no-op once disposed. All finalizer work is wrapped so no exception can escape. Adds JSValueScope.CurrentOrNull (non-throwing) and JSReferenceTests covering the no-context and context finalizer paths on a non-JS thread, plus that explicit Dispose still throws. The no-context test reproduces the reported crash stack and fails without this fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- src/NodeApi/JSReference.cs | 63 +++++++++++++++++-- src/NodeApi/JSValueScope.cs | 7 +++ test/JSReferenceTests.cs | 59 +++++++++++++++++ test/TestCases/edgejs-perf/global.json | 7 +++ test/TestCases/napi-dotnet-init/global.json | 7 +++ test/TestCases/napi-dotnet/global.json | 7 +++ test/TestCases/node-addon-api/global.json | 7 +++ .../projects/js-cjs-dynamic/global.json | 7 +++ .../projects/js-cjs-module/global.json | 7 +++ .../projects/js-esm-dynamic/global.json | 7 +++ .../projects/js-esm-module/global.json | 7 +++ .../projects/ts-cjs-dynamic/global.json | 7 +++ .../projects/ts-cjs-module/global.json | 7 +++ .../projects/ts-esm-dynamic/global.json | 7 +++ .../projects/ts-esm-module/global.json | 7 +++ 15 files changed, 209 insertions(+), 4 deletions(-) create mode 100644 test/TestCases/edgejs-perf/global.json create mode 100644 test/TestCases/napi-dotnet-init/global.json create mode 100644 test/TestCases/napi-dotnet/global.json create mode 100644 test/TestCases/node-addon-api/global.json create mode 100644 test/TestCases/projects/js-cjs-dynamic/global.json create mode 100644 test/TestCases/projects/js-cjs-module/global.json create mode 100644 test/TestCases/projects/js-esm-dynamic/global.json create mode 100644 test/TestCases/projects/js-esm-module/global.json create mode 100644 test/TestCases/projects/ts-cjs-dynamic/global.json create mode 100644 test/TestCases/projects/ts-cjs-module/global.json create mode 100644 test/TestCases/projects/ts-esm-dynamic/global.json create mode 100644 test/TestCases/projects/ts-esm-module/global.json diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 2bf0cccb..aab35264 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -346,12 +346,17 @@ public void Dispose() protected virtual void Dispose(bool disposing) { - if (!IsDisposed) + if (IsDisposed) { - IsDisposed = true; + return; + } + + IsDisposed = true; - // The context may be null if the reference was created from a "no-context" scope such - // as the native host. In that case the reference must be disposed from the JS thread. + if (disposing) + { + // Explicit disposal preserves the documented behavior, including asserting that a + // no-context reference is disposed from the JS thread. if (_context == null) { ThrowIfInvalidThreadAccess(); @@ -364,6 +369,56 @@ protected virtual void Dispose(bool disposing) _env, _handle).ThrowIfFailed(), allowSync: true); } } + else + { + // The finalizer runs on the GC finalizer thread and MUST NOT throw: an exception + // escaping a finalizer terminates the process (observed as a fatal + // JSInvalidThreadAccessException / SIGSEGV during worker-thread teardown). Release the + // native reference only if it can be done without switching threads or asserting an + // active JS scope, and never let an exception propagate. + DisposeFromFinalizer(); + } + } + + private void DisposeFromFinalizer() + { + try + { + if (_context == null) + { + // A no-context reference (for example one created from the native host scope) must + // be deleted on the JS thread. Only delete it if this finalizer happens to run + // while the matching JS scope is current on this thread; otherwise skip -- the JS + // environment is being torn down and the reference is released along with it. + JSValueScope? scope = JSValueScope.CurrentOrNull; + if (scope != null && scope.UncheckedEnvironmentHandle == _env) + { + scope.Runtime.DeleteReference(_env, _handle); + } + } + else + { + // Post the delete to the JS thread. The synchronization context is a safe no-op + // once it has been disposed (that is, after the worker has been torn down). + _context.SynchronizationContext?.Post( + () => + { + try + { + _context.Runtime.DeleteReference(_env, _handle); + } + catch + { + // The environment may already be gone; nothing more can be done. + } + }, + allowSync: false); + } + } + catch + { + // Never allow an exception to escape the finalizer. + } } ~JSReference() => Dispose(disposing: false); diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 0e4a3e14..991a6fd5 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -89,6 +89,13 @@ public sealed class JSValueScope : IDisposable public static JSValueScope Current => s_currentScope ?? throw new JSInvalidThreadAccessException(currentScope: null); + /// + /// Gets the current JS value scope for the calling thread, or null if no scope is + /// established. Unlike , this never throws, so it is safe to use from + /// contexts that must not throw, such as finalizers. + /// + internal static JSValueScope? CurrentOrNull => s_currentScope; + /// /// Gets the environment handle for the scope, or throws an exception if the scope is /// disposed or access from the current thread is invalid. diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index 6e487afc..2c83281c 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -107,4 +107,63 @@ public void TryGetWeakReferenceUnavailable() _mockRuntime.MockReleaseWeakReferenceValue(reference.Handle); Assert.False(reference.TryGetValue(out _)); } + + // A reference created from a NoContext scope (as the native host does) has a null runtime + // context, so its finalizer takes the branch that previously asserted thread access. The GC + // finalizer runs on a thread with no JS scope, so that assertion threw + // JSInvalidThreadAccessException out of the finalizer, which terminates the process (the + // reported worker-teardown crash). The finalizer must instead complete without throwing. + [Fact] + public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() + { + using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); + + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + + // Run on a new thread that has no current scope, simulating the GC finalizer thread. + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + + Assert.True(reference.IsDisposed); + } + + // A reference with a runtime context posts its cleanup to the JS thread. The finalizer must + // likewise never throw when it runs on a thread with no current scope. + [Fact] + public void FinalizeContextReferenceFromDifferentThreadDoesNotThrow() + { + using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + + Assert.True(reference.IsDisposed); + } + + // Explicit disposal (disposing: true) preserves the documented behavior of asserting thread + // access for a no-context reference; only the finalizer path is made non-throwing. + [Fact] + public void DisposeNoContextReferenceFromDifferentThreadThrows() + { + using JSValueScope noContextScope = TestScope(JSValueScopeType.NoContext); + + JSValue value = JSValue.CreateObject(); + JSReference reference = new(value); + + TestUtils.RunInThread(() => + { + Assert.Throws(() => reference.Dispose()); + }).Wait(); + } + + // Exposes the protected finalizer code path (Dispose(disposing: false)) so a test can invoke it + // directly on a non-JS thread, deterministically reproducing what the GC finalizer does. + private sealed class FinalizerTestReference : JSReference + { + public FinalizerTestReference(JSValue value) : base(value) { } + + public void SimulateFinalize() => Dispose(disposing: false); + } } diff --git a/test/TestCases/edgejs-perf/global.json b/test/TestCases/edgejs-perf/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/edgejs-perf/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/napi-dotnet-init/global.json b/test/TestCases/napi-dotnet-init/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/napi-dotnet-init/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/napi-dotnet/global.json b/test/TestCases/napi-dotnet/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/napi-dotnet/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/node-addon-api/global.json b/test/TestCases/node-addon-api/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/node-addon-api/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/js-cjs-dynamic/global.json b/test/TestCases/projects/js-cjs-dynamic/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/js-cjs-dynamic/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/js-cjs-module/global.json b/test/TestCases/projects/js-cjs-module/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/js-cjs-module/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/js-esm-dynamic/global.json b/test/TestCases/projects/js-esm-dynamic/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/js-esm-dynamic/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/js-esm-module/global.json b/test/TestCases/projects/js-esm-module/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/js-esm-module/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/ts-cjs-dynamic/global.json b/test/TestCases/projects/ts-cjs-dynamic/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/ts-cjs-dynamic/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/ts-cjs-module/global.json b/test/TestCases/projects/ts-cjs-module/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/ts-cjs-module/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/ts-esm-dynamic/global.json b/test/TestCases/projects/ts-esm-dynamic/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/ts-esm-dynamic/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file diff --git a/test/TestCases/projects/ts-esm-module/global.json b/test/TestCases/projects/ts-esm-module/global.json new file mode 100644 index 00000000..e784e0df --- /dev/null +++ b/test/TestCases/projects/ts-esm-module/global.json @@ -0,0 +1,7 @@ +{ + "sdk": { + "version": "10.0.100", + "allowPrerelease": true, + "rollForward": "latestFeature" + } +} \ No newline at end of file From bdde286c9629ca8a800b35cae8b0cade9eebff20 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 14:45:19 -0700 Subject: [PATCH 2/8] Fix formatting to satisfy CI dotnet format check The PR verification build failed at the 'dotnet format --verify-no-changes' step on all platforms: - JSValueScope: merge the ThreadStatic backing field and CurrentOrNull into a single [field: ThreadStatic] auto-property (IDE0032). - JSReferenceTests: FinalizerTestReference.SimulateFinalize now reads instance state (returns IsDisposed) so it is not flagged by CA1822; it must remain an instance method because it calls the instance Dispose(bool). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- src/NodeApi/JSValueScope.cs | 27 +++++++++++++-------------- test/JSReferenceTests.cs | 8 +++++++- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 991a6fd5..2aabc8e2 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -77,8 +77,6 @@ public sealed class JSValueScope : IDisposable private readonly SynchronizationContext? _previousSyncContext; private readonly nint _scopeHandle; - [ThreadStatic] private static JSValueScope? s_currentScope; - public JSValueScopeType ScopeType { get; } /// @@ -86,7 +84,7 @@ public sealed class JSValueScope : IDisposable /// /// No scope was established for the current /// thread. - public static JSValueScope Current => s_currentScope ?? + public static JSValueScope Current => CurrentOrNull ?? throw new JSInvalidThreadAccessException(currentScope: null); /// @@ -94,7 +92,8 @@ public sealed class JSValueScope : IDisposable /// established. Unlike , this never throws, so it is safe to use from /// contexts that must not throw, such as finalizers. /// - internal static JSValueScope? CurrentOrNull => s_currentScope; + [field: ThreadStatic] + internal static JSValueScope? CurrentOrNull { get; private set; } /// /// Gets the environment handle for the scope, or throws an exception if the scope is @@ -146,7 +145,7 @@ public static explicit operator napi_env(JSValueScope scope) internal nint RuntimeContextHandle { get; } internal static JSRuntime CurrentRuntime => Current.Runtime; - internal static JSRuntimeContext? CurrentRuntimeContext => s_currentScope?.RuntimeContext; + internal static JSRuntimeContext? CurrentRuntimeContext => CurrentOrNull?.RuntimeContext; public JSModuleContext? ModuleContext { get; internal set; } @@ -182,7 +181,7 @@ public JSValueScope( if (scopeType == JSValueScopeType.NoContext) { // A NoContext scope can inherit the env from a parent NoContext scope. - _parentScope = s_currentScope; + _parentScope = CurrentOrNull; if (_parentScope != null && _parentScope.ScopeType != JSValueScopeType.NoContext) { throw new InvalidOperationException( @@ -204,7 +203,7 @@ public JSValueScope( } else if (scopeType == JSValueScopeType.Root) { - _parentScope = s_currentScope; + _parentScope = CurrentOrNull; if (_parentScope != null) { if (_parentScope.ScopeType == JSValueScopeType.Root) @@ -237,7 +236,7 @@ public JSValueScope( } else { - _parentScope = s_currentScope; + _parentScope = CurrentOrNull; if (scopeType == JSValueScopeType.Module && _parentScope != null && _parentScope.ScopeType == JSValueScopeType.Module) @@ -324,10 +323,10 @@ public JSValueScope( _ => default, }; - JSValueScope? previousScope = s_currentScope; + JSValueScope? previousScope = CurrentOrNull; try { - s_currentScope = this; + CurrentOrNull = this; if (scopeType == JSValueScopeType.NoContext) { @@ -357,7 +356,7 @@ public JSValueScope( } catch (Exception) { - s_currentScope = previousScope; + CurrentOrNull = previousScope; throw; } } @@ -387,7 +386,7 @@ public void Dispose() } } - s_currentScope = _parentScope; + CurrentOrNull = _parentScope; } public JSValue Escape(JSValue value) @@ -427,9 +426,9 @@ internal void ThrowIfDisposed() /// thread. internal void ThrowIfInvalidThreadAccess() { - if (s_currentScope?._env != _env) + if (CurrentOrNull?._env != _env) { - throw new JSInvalidThreadAccessException(currentScope: s_currentScope, targetScope: this); + throw new JSInvalidThreadAccessException(currentScope: CurrentOrNull, targetScope: this); } } } diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index 2c83281c..dce9e9cb 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -164,6 +164,12 @@ private sealed class FinalizerTestReference : JSReference { public FinalizerTestReference(JSValue value) : base(value) { } - public void SimulateFinalize() => Dispose(disposing: false); + // Invokes the finalizer code path (Dispose(disposing: false)) on this instance and returns + // whether it completed. Reads instance state so it is not flagged as a static candidate. + public bool SimulateFinalize() + { + Dispose(disposing: false); + return IsDisposed; + } } } From 0e0116cfc7bf8ac675d1672437af4d1fa3e9ab87 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:02:28 -0700 Subject: [PATCH 3/8] Remove accidentally-committed generated global.json test artifacts These test/TestCases/**/global.json files are generated at test time by TestBuilder.WriteCurrentFrameworkGlobalJson and were unintentionally staged by git add -A. They are unrelated to the fix and pin the SDK per target framework, so committing them makes cross-TFM test runs rewrite tracked files. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- test/TestCases/edgejs-perf/global.json | 7 ------- test/TestCases/napi-dotnet-init/global.json | 7 ------- test/TestCases/napi-dotnet/global.json | 7 ------- test/TestCases/node-addon-api/global.json | 7 ------- test/TestCases/projects/js-cjs-dynamic/global.json | 7 ------- test/TestCases/projects/js-cjs-module/global.json | 7 ------- test/TestCases/projects/js-esm-dynamic/global.json | 7 ------- test/TestCases/projects/js-esm-module/global.json | 7 ------- test/TestCases/projects/ts-cjs-dynamic/global.json | 7 ------- test/TestCases/projects/ts-cjs-module/global.json | 7 ------- test/TestCases/projects/ts-esm-dynamic/global.json | 7 ------- test/TestCases/projects/ts-esm-module/global.json | 7 ------- 12 files changed, 84 deletions(-) delete mode 100644 test/TestCases/edgejs-perf/global.json delete mode 100644 test/TestCases/napi-dotnet-init/global.json delete mode 100644 test/TestCases/napi-dotnet/global.json delete mode 100644 test/TestCases/node-addon-api/global.json delete mode 100644 test/TestCases/projects/js-cjs-dynamic/global.json delete mode 100644 test/TestCases/projects/js-cjs-module/global.json delete mode 100644 test/TestCases/projects/js-esm-dynamic/global.json delete mode 100644 test/TestCases/projects/js-esm-module/global.json delete mode 100644 test/TestCases/projects/ts-cjs-dynamic/global.json delete mode 100644 test/TestCases/projects/ts-cjs-module/global.json delete mode 100644 test/TestCases/projects/ts-esm-dynamic/global.json delete mode 100644 test/TestCases/projects/ts-esm-module/global.json diff --git a/test/TestCases/edgejs-perf/global.json b/test/TestCases/edgejs-perf/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/edgejs-perf/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/napi-dotnet-init/global.json b/test/TestCases/napi-dotnet-init/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/napi-dotnet-init/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/napi-dotnet/global.json b/test/TestCases/napi-dotnet/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/napi-dotnet/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/node-addon-api/global.json b/test/TestCases/node-addon-api/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/node-addon-api/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/js-cjs-dynamic/global.json b/test/TestCases/projects/js-cjs-dynamic/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/js-cjs-dynamic/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/js-cjs-module/global.json b/test/TestCases/projects/js-cjs-module/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/js-cjs-module/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/js-esm-dynamic/global.json b/test/TestCases/projects/js-esm-dynamic/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/js-esm-dynamic/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/js-esm-module/global.json b/test/TestCases/projects/js-esm-module/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/js-esm-module/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/ts-cjs-dynamic/global.json b/test/TestCases/projects/ts-cjs-dynamic/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/ts-cjs-dynamic/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/ts-cjs-module/global.json b/test/TestCases/projects/ts-cjs-module/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/ts-cjs-module/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/ts-esm-dynamic/global.json b/test/TestCases/projects/ts-esm-dynamic/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/ts-esm-dynamic/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file diff --git a/test/TestCases/projects/ts-esm-module/global.json b/test/TestCases/projects/ts-esm-module/global.json deleted file mode 100644 index e784e0df..00000000 --- a/test/TestCases/projects/ts-esm-module/global.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "sdk": { - "version": "10.0.100", - "allowPrerelease": true, - "rollForward": "latestFeature" - } -} \ No newline at end of file From 273ed611d104d25f4c078c953b81f2ba01a98bf9 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:03:15 -0700 Subject: [PATCH 4/8] Ignore generated test global.json files Prevents test/TestCases/**/global.json (written per-runtime by TestBuilder) from being re-staged by git add. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 78967e96..c5ebcc71 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,6 @@ examples/**/package-lock.json cache/ dist/ + +# Generated at test time by TestBuilder.WriteCurrentFrameworkGlobalJson (per-runtime SDK pin) +test/TestCases/**/global.json From 8376dbf0609a4b6c7deeb2ebd9137bf4d57415b3 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:08:34 -0700 Subject: [PATCH 5/8] Verify deferred DeleteReference runs in context-path finalizer test Addresses review feedback that FinalizeContextReferenceFromDifferentThreadDoesNotThrow only asserted IsDisposed (set before the delete is posted) and never confirmed the deferred cleanup actually released the native reference. Adds MockJSRuntime.RecordingSynchronizationContext (records posted callbacks for deterministic pumping) and MockJSRuntime.HasReference, then asserts the finalizer defers the delete off-thread, and that pumping the sync context runs it and releases the reference. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- test/JSReferenceTests.cs | 25 ++++++++++++++++++---- test/MockJSRuntime.cs | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index dce9e9cb..07d44241 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using Microsoft.JavaScript.NodeApi.Interop; using Xunit; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; @@ -12,9 +13,13 @@ public class JSReferenceTests private readonly MockJSRuntime _mockRuntime = new(); private JSValueScope TestScope(JSValueScopeType scopeType) + => TestScope(scopeType, new MockJSRuntime.SynchronizationContext()); + + private JSValueScope TestScope( + JSValueScopeType scopeType, JSSynchronizationContext synchronizationContext) { napi_env env = new(Environment.CurrentManagedThreadId); - return new(scopeType, env, _mockRuntime, new MockJSRuntime.SynchronizationContext()); + return new(scopeType, env, _mockRuntime, synchronizationContext); } [Fact] @@ -127,19 +132,31 @@ public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() Assert.True(reference.IsDisposed); } - // A reference with a runtime context posts its cleanup to the JS thread. The finalizer must - // likewise never throw when it runs on a thread with no current scope. + // A reference with a runtime context posts its cleanup to the JS thread instead of deleting it + // inline. The finalizer must never throw when it runs on a thread with no current scope, and + // the posted delete must actually release the native reference once the JS thread pumps it. [Fact] public void FinalizeContextReferenceFromDifferentThreadDoesNotThrow() { - using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + var syncContext = new MockJSRuntime.RecordingSynchronizationContext(); + using JSValueScope rootScope = TestScope(JSValueScopeType.Root, syncContext); JSValue value = JSValue.CreateObject(); var reference = new FinalizerTestReference(value); + napi_ref handle = reference.Handle; + Assert.True(_mockRuntime.HasReference(handle)); TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); Assert.True(reference.IsDisposed); + + // The delete is deferred to the JS thread, not run inline on the finalizer thread. + Assert.True(_mockRuntime.HasReference(handle)); + Assert.Equal(1, syncContext.PendingCount); + + // Pumping the sync context runs the posted delete, releasing the native reference. + Assert.Equal(1, syncContext.RunPendingCallbacks()); + Assert.False(_mockRuntime.HasReference(handle)); } // Explicit disposal (disposing: true) preserves the documented behavior of asserting thread diff --git a/test/MockJSRuntime.cs b/test/MockJSRuntime.cs index ccd923bc..39a84a9f 100644 --- a/test/MockJSRuntime.cs +++ b/test/MockJSRuntime.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Threading; using Microsoft.JavaScript.NodeApi.Interop; using Microsoft.JavaScript.NodeApi.Runtime; using Xunit; @@ -187,6 +188,12 @@ public override napi_status DeleteReference(napi_env env, napi_ref @ref) return _references.Remove(@ref.Handle) ? napi_ok : napi_invalid_arg; } + /// + /// Reports whether a reference handle is still present (that is, has not been deleted by + /// ). Lets a test assert that a deferred delete actually ran. + /// + public bool HasReference(napi_ref @ref) => _references.ContainsKey(@ref.Handle); + /// /// Simulates the behavior of the JS runtime when a weakly-referenced value is released. /// @@ -206,4 +213,43 @@ public class SynchronizationContext : JSSynchronizationContext public override void CloseAsyncScope() => throw new NotImplementedException(); public override void OpenAsyncScope() => throw new NotImplementedException(); } + + // A synchronization context that records posted callbacks instead of running them, so a test + // can deterministically pump the queue with and assert that + // the posted work (for example a deferred DeleteReference) actually executed. + public class RecordingSynchronizationContext : JSSynchronizationContext + { + private readonly Queue<(SendOrPostCallback Callback, object? State)> _posted = new(); + + public int PendingCount => _posted.Count; + + public override void Post(SendOrPostCallback callback, object? state) + { + if (IsDisposed) return; + _posted.Enqueue((callback, state)); + } + + public override void Send(SendOrPostCallback callback, object? state) + { + if (IsDisposed) return; + callback(state); + } + + // Runs every callback posted so far and returns how many were run. + public int RunPendingCallbacks() + { + int count = 0; + while (_posted.Count > 0) + { + (SendOrPostCallback callback, object? state) = _posted.Dequeue(); + callback(state); + count++; + } + + return count; + } + + public override void CloseAsyncScope() { } + public override void OpenAsyncScope() { } + } } From 30774f6b7592653ceb25317345d7155d93a365ad Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:23:26 -0700 Subject: [PATCH 6/8] Clarify no-context finalizer comment to match runtime behavior The delete branch guarded by JSValueScope.CurrentOrNull never runs on the real GC finalizer thread (CurrentOrNull is thread-static and null there), so a no-context reference is reclaimed when the JS environment is destroyed rather than immediately. The prior comment implied teardown was always in progress. No behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- src/NodeApi/JSReference.cs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index aab35264..34316a99 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -386,10 +386,14 @@ private void DisposeFromFinalizer() { if (_context == null) { - // A no-context reference (for example one created from the native host scope) must - // be deleted on the JS thread. Only delete it if this finalizer happens to run - // while the matching JS scope is current on this thread; otherwise skip -- the JS - // environment is being torn down and the reference is released along with it. + // A no-context reference (for example one created from the native host scope) can + // only be deleted on the JS thread. CurrentOrNull is thread-static, so on the real + // GC finalizer thread it is null and this delete is skipped; the napi_ref is then + // reclaimed when the JS environment is destroyed. The guarded delete still runs if + // Dispose(disposing: false) is ever invoked on the owning JS thread. A no-context + // scope has no synchronization context, so the finalizer cannot marshal the delete + // to the JS thread; doing so would require an env-scoped cleanup queue in the + // native host (tracked as a follow-up). JSValueScope? scope = JSValueScope.CurrentOrNull; if (scope != null && scope.UncheckedEnvironmentHandle == _env) { From d312c9c5b54ef7da0ccafbd71f6fab13312f44ec Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:39:23 -0700 Subject: [PATCH 7/8] Catch exceptions at the finalizer entry point to cover Dispose overrides ~JSReference() invokes the virtual Dispose(bool), so a derived override could throw before or after the base implementation runs and let the exception escape the finalizer, terminating the process. Wrap Dispose(disposing: false) in try/catch at the ~JSReference() entry point so the no-throw guarantee also covers overrides. Adds FinalizerSwallowsExceptionsFromDerivedDisposeOverride, which drives real GC finalization of a throwing override; without the entry-point catch the test host would crash instead of completing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- src/NodeApi/JSReference.cs | 15 +++++++++++++- test/JSReferenceTests.cs | 40 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 34316a99..250b0286 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -425,5 +425,18 @@ private void DisposeFromFinalizer() } } - ~JSReference() => Dispose(disposing: false); + ~JSReference() + { + // An exception escaping a finalizer terminates the process. Dispose(bool) is virtual, so a + // derived override may throw before or after the base implementation runs; catch here at + // the finalizer entry point so the no-throw guarantee also covers overrides. + try + { + Dispose(disposing: false); + } + catch + { + // Never allow an exception to escape the finalizer. + } + } } diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index 07d44241..0704a036 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Runtime.CompilerServices; using Microsoft.JavaScript.NodeApi.Interop; using Xunit; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; @@ -175,6 +176,32 @@ public void DisposeNoContextReferenceFromDifferentThreadThrows() }).Wait(); } + // The finalizer invokes the virtual Dispose(bool), so a derived override can throw before or + // after the base implementation runs. ~JSReference() must catch at its entry point, otherwise + // the exception escapes the finalizer and terminates the process. This drives real GC + // finalization of an override that throws; if the guarantee held only for the base method, the + // test host would crash instead of completing. + [Fact] + public void FinalizerSwallowsExceptionsFromDerivedDisposeOverride() + { + using JSValueScope rootScope = TestScope(JSValueScopeType.Root); + + CreateAndAbandonThrowingReference(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + } + + // Creates a throwing reference in a separate non-inlined frame and keeps no reference to it, so + // it becomes eligible for finalization once this method returns. + [MethodImpl(MethodImplOptions.NoInlining)] + private void CreateAndAbandonThrowingReference() + { + JSValue value = JSValue.CreateObject(); + _ = new ThrowingFinalizerReference(value); + } + // Exposes the protected finalizer code path (Dispose(disposing: false)) so a test can invoke it // directly on a non-JS thread, deterministically reproducing what the GC finalizer does. private sealed class FinalizerTestReference : JSReference @@ -189,4 +216,17 @@ public bool SimulateFinalize() return IsDisposed; } } + + // A reference whose Dispose(bool) override throws, to verify the finalizer entry point catches + // exceptions from derived overrides and not just from the base implementation. + private sealed class ThrowingFinalizerReference : JSReference + { + public ThrowingFinalizerReference(JSValue value) : base(value) { } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + throw new InvalidOperationException("Simulated failure in a derived finalizer."); + } + } } From 7151da56ea9f9acad1b90752cce9a3a660e96cd7 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Mon, 10 Aug 2026 15:56:18 -0700 Subject: [PATCH 8/8] Make CreateAndAbandonThrowingReference static to satisfy CA1822 The new test helper does not access instance state, so dotnet format --severity info (the CI formatting gate) flagged CA1822 and failed every matrix job. Mark it static. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- test/JSReferenceTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index 0704a036..ed994865 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -196,7 +196,7 @@ public void FinalizerSwallowsExceptionsFromDerivedDisposeOverride() // Creates a throwing reference in a separate non-inlined frame and keeps no reference to it, so // it becomes eligible for finalization once this method returns. [MethodImpl(MethodImplOptions.NoInlining)] - private void CreateAndAbandonThrowingReference() + private static void CreateAndAbandonThrowingReference() { JSValue value = JSValue.CreateObject(); _ = new ThrowingFinalizerReference(value);