Make JSReference finalizer thread-safe to fix worker-teardown crash (exit 139)

.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

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.
+        }
+    }
 }

src/NodeApi/JSValueScope.cs

@@ -77,18 +77,24 @@ public sealed class JSValueScope : IDisposable
     private readonly SynchronizationContext? _previousSyncContext;
     private readonly nint _scopeHandle;
 
-    [ThreadStatic] private static JSValueScope? s_currentScope;
-
     public JSValueScopeType ScopeType { get; }
 
     /// <summary>
     /// Gets the current JS value scope.
     /// </summary>
     /// <exception cref="JSInvalidThreadAccessException">No scope was established for the current
     /// thread.</exception>
-    public static JSValueScope Current => s_currentScope ??
+    public static JSValueScope Current => CurrentOrNull ??
         throw new JSInvalidThreadAccessException(currentScope: null);
 
+    /// <summary>
+    /// Gets the current JS value scope for the calling thread, or null if no scope is
+    /// established. Unlike <see cref="Current"/>, this never throws, so it is safe to use from
+    /// contexts that must not throw, such as finalizers.
+    /// </summary>
+    [field: ThreadStatic]
+    internal static JSValueScope? CurrentOrNull { get; private set; }
+
     /// <summary>
     /// 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.</exception>
     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);
         }
     }
 }

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<JSInvalidThreadAccessException>(() => 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.");
+        }
+    }
 }