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 diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 2bf0cccb..250b0286 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; + } - // 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. + IsDisposed = true; + + 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,7 +369,74 @@ 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) 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) + { + 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); + ~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/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 0e4a3e14..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,9 +84,17 @@ 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); + /// + /// 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. + /// + [field: ThreadStatic] + internal static JSValueScope? CurrentOrNull { get; private set; } + /// /// Gets the environment handle for the scope, or throws an exception if the scope is /// disposed or access from the current thread is invalid. @@ -139,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; } @@ -175,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( @@ -197,7 +203,7 @@ public JSValueScope( } else if (scopeType == JSValueScopeType.Root) { - _parentScope = s_currentScope; + _parentScope = CurrentOrNull; if (_parentScope != null) { if (_parentScope.ScopeType == JSValueScopeType.Root) @@ -230,7 +236,7 @@ public JSValueScope( } else { - _parentScope = s_currentScope; + _parentScope = CurrentOrNull; if (scopeType == JSValueScopeType.Module && _parentScope != null && _parentScope.ScopeType == JSValueScopeType.Module) @@ -317,10 +323,10 @@ public JSValueScope( _ => default, }; - JSValueScope? previousScope = s_currentScope; + JSValueScope? previousScope = CurrentOrNull; try { - s_currentScope = this; + CurrentOrNull = this; if (scopeType == JSValueScopeType.NoContext) { @@ -350,7 +356,7 @@ public JSValueScope( } catch (Exception) { - s_currentScope = previousScope; + CurrentOrNull = previousScope; throw; } } @@ -380,7 +386,7 @@ public void Dispose() } } - s_currentScope = _parentScope; + CurrentOrNull = _parentScope; } public JSValue Escape(JSValue value) @@ -420,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 6e487afc..ed994865 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -2,6 +2,8 @@ // 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; @@ -12,9 +14,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] @@ -107,4 +113,120 @@ 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 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() + { + 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 + // 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(); + } + + // 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 static 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 + { + public FinalizerTestReference(JSValue value) : base(value) { } + + // 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; + } + } + + // 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."); + } + } } 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() { } + } }