From ee3e750aa81cda85f668febabf520652cbbfc95a Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Fri, 14 Aug 2026 17:26:44 -0700 Subject: [PATCH 1/2] Delete no-context JSReference on the JS thread instead of leaking it A JSReference created in a NoContext scope has no JSSynchronizationContext, so when its finalizer runs on the GC finalizer thread it cannot marshal DeleteReference back to the owning JS thread. Previously the finalizer simply skipped the delete, leaking the napi_ref (and any strongly-referenced JS value) until the JS environment was destroyed. Defer such deletions into an environment-scoped queue keyed by napi_env. The queue is drained on the JS thread the next time any scope for that environment is entered, and finally when the environment's Root/NoContext scope is disposed (which also removes the registry entry so it does not retain dead environments). A cheap interlocked counter gates the fast path, so the common no-pending case costs only a volatile read per scope entry. Deferred handles are always deleted while the environment is still alive, keeping the queued napi_ref valid. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- src/NodeApi/JSReference.cs | 114 ++++++++++++++++++++++++++++++++++-- src/NodeApi/JSValueScope.cs | 15 +++++ test/JSReferenceTests.cs | 61 ++++++++++++++++++- 3 files changed, 183 insertions(+), 7 deletions(-) diff --git a/src/NodeApi/JSReference.cs b/src/NodeApi/JSReference.cs index 250b0286..d0c6e862 100644 --- a/src/NodeApi/JSReference.cs +++ b/src/NodeApi/JSReference.cs @@ -2,9 +2,11 @@ // Licensed under the MIT License. using System; +using System.Collections.Concurrent; using System.Diagnostics.CodeAnalysis; using System.Threading; using Microsoft.JavaScript.NodeApi.Interop; +using Microsoft.JavaScript.NodeApi.Runtime; using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime; namespace Microsoft.JavaScript.NodeApi; @@ -32,6 +34,30 @@ public class JSReference : IDisposable private readonly napi_env _env; private readonly JSRuntimeContext? _context; + /// + /// Environment-scoped queue of no-context references whose finalizers could not release them + /// on the JS thread, keyed by the owning environment handle. + /// + /// + /// A no-context reference has no , so when its finalizer + /// runs on the GC finalizer thread (which has no JS scope) it cannot marshal + /// DeleteReference back to the owning JS thread. Rather than leaking the + /// napi_ref until the environment is destroyed, the finalizer enqueues the handle here. + /// The deletion is then performed the next time a scope for the same environment is active on + /// its JS thread (see ), and a final drain runs when that + /// environment's root scope is disposed. Deferred handles are always drained while the + /// environment is still alive, so the queued napi_ref remains valid. + /// + private static readonly ConcurrentDictionary> + s_pendingFinalizerDeletions = new(); + + /// + /// Total number of deferred deletions pending across all environments. Used as a cheap + /// fast-path gate so that the common case (nothing pending) avoids a dictionary lookup on + /// every scope entry. + /// + private static int s_pendingFinalizerDeletionCount; + /// /// Creates a new instance of a that holds a strong or weak /// reference to a JS value. @@ -388,17 +414,21 @@ private void DisposeFromFinalizer() { // 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). + // GC finalizer thread it is null and the inline delete below is skipped; the + // handle is instead deferred to be deleted the next time a scope for this + // environment is active on its JS thread (see JSValueScope). The guarded delete + // still runs directly if Dispose(disposing: false) is ever invoked on the owning + // JS thread. This releases the napi_ref promptly instead of leaking it until the + // JS environment is destroyed. JSValueScope? scope = JSValueScope.CurrentOrNull; if (scope != null && scope.UncheckedEnvironmentHandle == _env) { scope.Runtime.DeleteReference(_env, _handle); } + else + { + EnqueuePendingDeletion(_env, _handle); + } } else { @@ -425,6 +455,78 @@ private void DisposeFromFinalizer() } } + /// + /// Records a no-context reference handle whose deletion was deferred because its finalizer + /// could not run on the owning JS thread. The handle is deleted later by + /// or . + /// + private static void EnqueuePendingDeletion(napi_env env, napi_ref handle) + { + s_pendingFinalizerDeletions + .GetOrAdd(env, _ => new ConcurrentQueue()) + .Enqueue(handle); + Interlocked.Increment(ref s_pendingFinalizerDeletionCount); + } + + /// + /// Deletes any no-context reference handles that were deferred by a finalizer for the given + /// environment. Must be called on the JS thread that owns with an + /// active scope, so that DeleteReference is valid. + /// + /// The environment whose deferred deletions should be released. + /// The JS runtime for the environment. + internal static void DrainPendingDeletions(napi_env env, JSRuntime runtime) + { + // Fast path: avoid a dictionary lookup on every scope entry when nothing is pending. + if (Volatile.Read(ref s_pendingFinalizerDeletionCount) == 0) + { + return; + } + + if (!s_pendingFinalizerDeletions.TryGetValue( + env, out ConcurrentQueue? queue)) + { + return; + } + + DeletePendingHandles(env, runtime, queue); + } + + /// + /// Performs a final drain of deferred deletions for an environment that is being torn down and + /// removes its queue, so the registry does not retain entries for environments that no longer + /// exist. Must be called on the owning JS thread while the environment is still alive. + /// + /// The environment being torn down. + /// The JS runtime for the environment. + internal static void RemovePendingDeletions(napi_env env, JSRuntime runtime) + { + if (!s_pendingFinalizerDeletions.TryRemove( + env, out ConcurrentQueue? queue)) + { + return; + } + + DeletePendingHandles(env, runtime, queue); + } + + private static void DeletePendingHandles( + napi_env env, JSRuntime runtime, ConcurrentQueue queue) + { + while (queue.TryDequeue(out napi_ref handle)) + { + Interlocked.Decrement(ref s_pendingFinalizerDeletionCount); + try + { + runtime.DeleteReference(env, handle); + } + catch + { + // Best effort: the environment may already be gone. + } + } + } + ~JSReference() { // An exception escaping a finalizer terminates the process. Dispose(bool) is virtual, so a diff --git a/src/NodeApi/JSValueScope.cs b/src/NodeApi/JSValueScope.cs index 2aabc8e2..7e938a8e 100644 --- a/src/NodeApi/JSValueScope.cs +++ b/src/NodeApi/JSValueScope.cs @@ -359,6 +359,11 @@ public JSValueScope( CurrentOrNull = previousScope; throw; } + + // Release any no-context reference deletions that a finalizer had to defer because it could + // not run on the JS thread. This scope is active on the owning JS thread, so deleting the + // deferred handles here is valid. The call is a cheap no-op when nothing is pending. + JSReference.DrainPendingDeletions(_env, Runtime); } public void Dispose() @@ -366,6 +371,16 @@ public void Dispose() if (IsDisposed) return; IsDisposed = true; + if (ScopeType == JSValueScopeType.Root || ScopeType == JSValueScopeType.NoContext) + { + // This environment-scoped scope is going away, so perform a final drain of any deferred + // no-context reference deletions and remove the environment's queue, preventing the + // registry from retaining entries for environments that no longer exist. The scope is + // still current here, so deletion runs on the owning JS thread while the environment is + // alive. + JSReference.RemovePendingDeletions(_env, Runtime); + } + if (ScopeType != JSValueScopeType.NoContext) { napi_env env = RuntimeContext.EnvironmentHandle; diff --git a/test/JSReferenceTests.cs b/test/JSReferenceTests.cs index ed994865..fb2cbffd 100644 --- a/test/JSReferenceTests.cs +++ b/test/JSReferenceTests.cs @@ -133,7 +133,66 @@ public void FinalizeNoContextReferenceFromDifferentThreadDoesNotThrow() Assert.True(reference.IsDisposed); } - // A reference with a runtime context posts its cleanup to the JS thread instead of deleting it + // A no-context reference finalized off the JS thread cannot be deleted inline (there is no + // synchronization context to marshal the delete back to the JS thread). Instead of leaking the + // napi_ref until the environment is destroyed, the finalizer defers the deletion, which is then + // performed the next time a scope for the same environment is active on its JS thread. + [Fact] + public void FinalizeNoContextReferenceDefersDeletionUntilNextScope() + { + napi_env env = new(Environment.CurrentManagedThreadId); + using JSValueScope noContextScope = new( + JSValueScopeType.NoContext, env, _mockRuntime, + new MockJSRuntime.SynchronizationContext()); + + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + napi_ref handle = reference.Handle; + Assert.True(_mockRuntime.HasReference(handle)); + + // Simulate the GC finalizer thread: no current scope, so the delete cannot run inline. + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + Assert.True(reference.IsDisposed); + + // The delete is deferred, not run on the finalizer thread, so the reference still exists. + Assert.True(_mockRuntime.HasReference(handle)); + + // Entering a scope for the same environment on the JS thread drains the deferred deletion. + using (JSValueScope drainScope = new(JSValueScopeType.NoContext)) + { + } + + Assert.False(_mockRuntime.HasReference(handle)); + } + + // When the environment-scoped scope that owns a deferred no-context deletion is disposed, a + // final drain runs so the napi_ref is released even if no further scope is entered. + [Fact] + public void DisposingScopeDrainsDeferredNoContextDeletion() + { + napi_env env = new(Environment.CurrentManagedThreadId); + napi_ref handle; + + var noContextScope = new JSValueScope( + JSValueScopeType.NoContext, env, _mockRuntime, + new MockJSRuntime.SynchronizationContext()); + try + { + JSValue value = JSValue.CreateObject(); + var reference = new FinalizerTestReference(value); + handle = reference.Handle; + + TestUtils.RunInThread(() => reference.SimulateFinalize()).Wait(); + Assert.True(_mockRuntime.HasReference(handle)); + } + finally + { + // Disposing the environment-scoped scope performs the final drain. + noContextScope.Dispose(); + } + + Assert.False(_mockRuntime.HasReference(handle)); + } // 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] From 62ad2c65191130e030ec558db6b414cad8bf5eb6 Mon Sep 17 00:00:00 2001 From: Saul Ponce Razo Date: Fri, 14 Aug 2026 17:26:59 -0700 Subject: [PATCH 2/2] Close TSFN post-then-release race in JSSynchronizationContext JSTsfnSynchronizationContext.Post/Send checked IsDisposed and then called the thread-safe function via NonBlockingCall. The environment cleanup hook could run Dispose (releasing the TSFN) between the check and the call, causing a native use-after-release that a managed try/catch cannot guard. Introduce TsfnCallGate, an interlocked gate that reference-counts in-flight NonBlockingCall invocations. Post/Send wrap each native call in TryEnter/Exit; Dispose calls Close before releasing the TSFN. Close atomically rejects new entrants and then waits for in-flight calls to drain, guaranteeing no native call is in progress and none can start once the TSFN is released. The gate is held only around the native call, not around Send's completion wait, so TSFN release cannot deadlock against a callback running on the JS thread. Expose internals to the test assembly so the gate's concurrency behavior can be unit-tested directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 144c87db-8d78-474f-bff9-21031a23e3e7 --- .../Interop/JSSynchronizationContext.cs | 43 +++++-- src/NodeApi/Interop/TsfnCallGate.cs | 97 ++++++++++++++ src/NodeApi/NodeApi.csproj | 4 + test/TsfnCallGateTests.cs | 121 ++++++++++++++++++ 4 files changed, 257 insertions(+), 8 deletions(-) create mode 100644 src/NodeApi/Interop/TsfnCallGate.cs create mode 100644 test/TsfnCallGateTests.cs diff --git a/src/NodeApi/Interop/JSSynchronizationContext.cs b/src/NodeApi/Interop/JSSynchronizationContext.cs index 48e2463b..4a729708 100644 --- a/src/NodeApi/Interop/JSSynchronizationContext.cs +++ b/src/NodeApi/Interop/JSSynchronizationContext.cs @@ -254,6 +254,10 @@ internal sealed unsafe class JSTsfnSynchronizationContext : JSSynchronizationCon private readonly JSRuntime _runtime; private readonly napi_env _env; private readonly JSThreadSafeFunction _tsfn; + + // Coordinates in-flight NonBlockingCall invocations with TSFN release during teardown, so that + // a poster can never call into the TSFN after (or while) it is being released. + private readonly TsfnCallGate _callGate = new(); private GCHandle _cleanupHandle; public JSTsfnSynchronizationContext() @@ -298,6 +302,11 @@ public override void Dispose() base.Dispose(); + // Prevent any new NonBlockingCall from starting and wait for in-flight calls to finish + // before releasing the TSFN. This closes the post-then-release race in which a poster could + // call into the TSFN after the environment cleanup hook has released it. + _callGate.Close(); + // Destroy TSFN by releasing last thread use count. // TSFN is deleted after this point and must not be used. _tsfn.Release(); @@ -348,9 +357,16 @@ public override void CloseAsyncScope() public override void Post(SendOrPostCallback callback, object? state) { - if (IsDisposed) return; + if (!_callGate.TryEnter()) return; - _tsfn.NonBlockingCall(() => callback(state)); + try + { + _tsfn.NonBlockingCall(() => callback(state)); + } + finally + { + _callGate.Exit(); + } } public override void Send(SendOrPostCallback callback, object? state) @@ -361,14 +377,25 @@ public override void Send(SendOrPostCallback callback, object? state) return; } - if (IsDisposed) return; - using ManualResetEvent syncEvent = new(false); - _tsfn.NonBlockingCall(() => + + // The gate is held only around the native NonBlockingCall, not around the wait below: the + // posted callback runs on the JS thread and could otherwise deadlock TSFN release. + if (!_callGate.TryEnter()) return; + + try { - callback(state); - syncEvent.Set(); - }); + _tsfn.NonBlockingCall(() => + { + callback(state); + syncEvent.Set(); + }); + } + finally + { + _callGate.Exit(); + } + syncEvent.WaitOne(); } } diff --git a/src/NodeApi/Interop/TsfnCallGate.cs b/src/NodeApi/Interop/TsfnCallGate.cs new file mode 100644 index 00000000..0ba64da0 --- /dev/null +++ b/src/NodeApi/Interop/TsfnCallGate.cs @@ -0,0 +1,97 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Threading; + +namespace Microsoft.JavaScript.NodeApi.Interop; + +/// +/// Coordinates in-flight native thread-safe-function (TSFN) calls with TSFN release, so that a +/// caller cannot invoke the TSFN after (or while) it is being released. +/// +/// +/// +/// posts work to the JS thread by calling into a native +/// TSFN, and releases that TSFN from an environment cleanup hook during teardown. Without +/// coordination there is a race: a poster can observe the context as not-yet-disposed, then the +/// cleanup hook can release the TSFN, and then the poster calls into the released TSFN — a +/// native use-after-release that a managed try/catch cannot guard. +/// +/// +/// This gate closes that window. A caller wraps each native TSFN call in +/// /; the owner calls before releasing +/// the TSFN. atomically prevents any further from +/// succeeding and then waits for all in-flight calls to finish, guaranteeing that no native call +/// is in progress and none can start once returns. +/// +/// +internal sealed class TsfnCallGate +{ + // High bit marks the gate as closed; the remaining bits count in-flight calls. Because Enter + // never increments once the closed bit is set, and Exit is only called after a successful + // Enter, the count part never borrows into the closed bit. + private const int ClosedFlag = unchecked((int)0x80000000); + private const int CountMask = 0x7FFFFFFF; + + private int _state; + + /// + /// Gets a value indicating whether the gate has been closed. + /// + public bool IsClosed => (Volatile.Read(ref _state) & ClosedFlag) != 0; + + /// + /// Attempts to enter the gate for a single native call. When this returns true the caller must + /// pair it with exactly one call to once the native call has returned. + /// + /// True if the call may proceed; false if the gate is closed and the call must be + /// skipped. + public bool TryEnter() + { + int state = Volatile.Read(ref _state); + while ((state & ClosedFlag) == 0) + { + int updated = Interlocked.CompareExchange(ref _state, state + 1, state); + if (updated == state) + { + return true; + } + + state = updated; + } + + return false; + } + + /// + /// Marks completion of a native call previously admitted by . + /// + public void Exit() => Interlocked.Decrement(ref _state); + + /// + /// Closes the gate so that no further calls can enter, then blocks until all in-flight calls + /// have exited. After this returns it is safe to release the underlying TSFN. Calling this more + /// than once is safe. + /// + public void Close() + { + int state = Volatile.Read(ref _state); + while ((state & ClosedFlag) == 0) + { + int updated = Interlocked.CompareExchange( + ref _state, state | ClosedFlag, state); + if (updated == state) + { + break; + } + + state = updated; + } + + SpinWait spin = default; + while ((Volatile.Read(ref _state) & CountMask) != 0) + { + spin.SpinOnce(); + } + } +} diff --git a/src/NodeApi/NodeApi.csproj b/src/NodeApi/NodeApi.csproj index b6aa76a6..3727e35e 100644 --- a/src/NodeApi/NodeApi.csproj +++ b/src/NodeApi/NodeApi.csproj @@ -28,6 +28,10 @@ + + + + diff --git a/test/TsfnCallGateTests.cs b/test/TsfnCallGateTests.cs new file mode 100644 index 00000000..6b6d362d --- /dev/null +++ b/test/TsfnCallGateTests.cs @@ -0,0 +1,121 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.JavaScript.NodeApi.Interop; +using Xunit; + +namespace Microsoft.JavaScript.NodeApi.Test; + +public class TsfnCallGateTests +{ + [Fact] + public void EnterAndExitWhileOpen() + { + var gate = new TsfnCallGate(); + + Assert.True(gate.TryEnter()); + gate.Exit(); + + Assert.False(gate.IsClosed); + } + + [Fact] + public void CloseRejectsFurtherEntry() + { + var gate = new TsfnCallGate(); + + gate.Close(); + + Assert.True(gate.IsClosed); + Assert.False(gate.TryEnter()); + } + + [Fact] + public void CloseIsIdempotent() + { + var gate = new TsfnCallGate(); + + gate.Close(); + gate.Close(); + + Assert.True(gate.IsClosed); + Assert.False(gate.TryEnter()); + } + + // Close must not return while a call is in flight: it has to wait for the corresponding Exit, + // guaranteeing no native TSFN call is in progress once the TSFN is released. + [Fact] + public void CloseWaitsForInFlightCallToExit() + { + var gate = new TsfnCallGate(); + + // Simulate an in-flight native call that has entered but not yet exited. + Assert.True(gate.TryEnter()); + + Task closeTask = Task.Run(() => gate.Close()); + + // Close cannot complete while the call is in flight. + Assert.False(closeTask.Wait(TimeSpan.FromMilliseconds(200))); + + // Once the in-flight call exits, Close completes. + gate.Exit(); + Assert.True(closeTask.Wait(TimeSpan.FromSeconds(5))); + + // After closing, no further calls are admitted. + Assert.False(gate.TryEnter()); + } + + // Once Close has set the closed flag, an entry that races with it must be rejected, so the + // in-flight count cannot rise again after Close begins draining. + [Fact] + public void EntryDoesNotSucceedAfterCloseFlagSet() + { + var gate = new TsfnCallGate(); + + gate.Close(); + + // A burst of concurrent entry attempts after Close must all fail. + Parallel.For(0, 1000, _ => Assert.False(gate.TryEnter())); + } + + // Stress the gate with concurrent enter/exit callers while another thread closes it, then + // assert the invariant that after Close returns no caller is inside the gate and none can + // enter. + [Fact] + public void ConcurrentCallersDrainBeforeCloseCompletes() + { + var gate = new TsfnCallGate(); + using var start = new ManualResetEventSlim(false); + + Task[] callers = new Task[8]; + for (int i = 0; i < callers.Length; i++) + { + callers[i] = Task.Run(() => + { + start.Wait(); + for (int j = 0; j < 5000; j++) + { + if (gate.TryEnter()) + { + // Represents a brief native call. + Thread.SpinWait(10); + gate.Exit(); + } + } + }); + } + + start.Set(); + + // Close concurrently with the callers; it must wait for any in-flight call to exit. + gate.Close(); + + // After Close returns, no caller may enter and the callers finish without error. + Assert.False(gate.TryEnter()); + Assert.True(Task.WaitAll(callers, TimeSpan.FromSeconds(30))); + Assert.True(gate.IsClosed); + } +}