Handle scope validation tests (#180)

Author: jasongin

src/NodeApi.DotNetHost/ManagedHost.cs

@@ -393,14 +393,15 @@ public JSValue LoadModule(JSCallbackArgs args)
             }
         }
 
+        JSValueScope scope = JSValueScope.Current;
         JSValue exports = JSValue.CreateObject();
 
         var result = (napi_value?)initializeMethod.Invoke(
-            null, new object[] { (napi_env)JSValueScope.Current, (napi_value)exports });
+            null, new object[] { (napi_env)scope, (napi_value)exports });
 
         if (result != null && result.Value != default)
         {
-            exports = new JSValue(result.Value);
+            exports = new JSValue(result.Value, scope);
         }
 
         if (exports.IsObject())

src/NodeApi.Generator/ModuleGenerator.cs

@@ -282,6 +282,10 @@ private SourceBuilder GenerateModuleInitializer(
         s += $"public static class {ModuleInitializerClassName}";
         s += "{";
 
+        // The module scope is not disposed after a successful initialization. It becomes
+        // the parent of callback scopes, allowing the JS runtime instance to be inherited.
+        s += "private static JSValueScope _moduleScope;";
+
         // The unmanaged entrypoint is used only when the AOT-compiled module is loaded.
         s += "#if !NETFRAMEWORK";
         s += $"[UnmanagedCallersOnly(EntryPoint = \"{ModuleRegisterFunctionName}\")]";
@@ -293,11 +297,11 @@ private SourceBuilder GenerateModuleInitializer(
         // The main initialization entrypoint is called by the `ManagedHost`, and by the unmanaged entrypoint.
         s += $"public static napi_value {ModuleInitializeMethodName}(napi_env env, napi_value exports)";
         s += "{";
-        s += "var scope = new JSValueScope(JSValueScopeType.Module, env);";
+        s += "_moduleScope = new JSValueScope(JSValueScopeType.Module, env, runtime: default);";
         s += "try";
         s += "{";
-        s += "JSRuntimeContext context = scope.RuntimeContext;";
-        s += "JSValue exportsValue = new(exports, scope);";
+        s += "JSRuntimeContext context = _moduleScope.RuntimeContext;";
+        s += "JSValue exportsValue = new(exports, _moduleScope);";
         s++;
 
         if (moduleInitializer is IMethodSymbol moduleInitializerMethod)
@@ -327,15 +331,12 @@ private SourceBuilder GenerateModuleInitializer(
             s += "return (napi_value)exportsValue;";
         }
 
-        // The module scope is not disposed before a successful return. It becomes the parent
-        // of callback scopes, allowing the JS runtime instance to be inherited.
-
         s += "}";
         s += "catch (System.Exception ex)";
         s += "{";
         s += "System.Console.Error.WriteLine($\"Failed to export module: {ex}\");";
         s += "JSError.ThrowError(ex);";
-        s += "scope.Dispose();";
+        s += "_moduleScope.Dispose();";
         s += "return exports;";
         s += "}";
         s += "}";

src/NodeApi/DotNetHost/NativeHost.cs

@@ -23,10 +23,12 @@ internal unsafe partial class NativeHost : IDisposable
     private static readonly string s_managedHostTypeName =
         typeof(NativeHost).Namespace + ".ManagedHost";
 
+    private static JSRuntime? s_jsRuntime;
     private string? _targetFramework;
     private string? _managedHostPath;
     private ICLRRuntimeHost* _runtimeHost;
     private hostfxr_handle _hostContextHandle;
+    private readonly JSValueScope _hostScope;
     private JSReference? _exports;
 
     public static bool IsTracingEnabled { get; } =
@@ -48,15 +50,18 @@ public static napi_value InitializeModule(napi_env env, napi_value exports)
     {
         Trace($"> NativeHost.InitializeModule({env.Handle:X8}, {exports.Handle:X8})");
 
-        JSRuntime runtime = new NodejsRuntime();
-        using JSValueScope scope = new(JSValueScopeType.NoContext, env, runtime);
+        s_jsRuntime ??= new NodejsRuntime();
+
+        // The native host JSValueScope is not disposed after a successful initialization. It
+        // becomes the parent of callback scopes, allowing the JS runtime instance to be inherited.
+        JSValueScope hostScope = new(JSValueScopeType.NoContext, env, s_jsRuntime);
         try
         {
-            NativeHost host = new();
+            NativeHost host = new(hostScope);
 
             // Do not use JSModuleBuilder here because it relies on having a current context.
             // But the context will be set by the managed host.
-            new JSValue(exports, scope).DefineProperties(
+            new JSValue(exports, hostScope).DefineProperties(
                 // The package index.js will invoke the initialize method with the path to
                 // the managed host assembly.
                 JSPropertyDescriptor.Function("initialize", host.InitializeManagedHost));
@@ -65,16 +70,18 @@ public static napi_value InitializeModule(napi_env env, napi_value exports)
         {
             string message = $"Failed to load CLR native host module: {ex}";
             Trace(message);
-            runtime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message));
+            s_jsRuntime.Throw(env, (napi_value)JSValue.CreateError(null, (JSValue)message));
+            hostScope.Dispose();
         }
 
         Trace("< NativeHost.InitializeModule()");
 
         return exports;
     }
 
-    public NativeHost()
+    private NativeHost(JSValueScope hostScope)
     {
+        _hostScope = hostScope;
     }
 
     /// <summary>

src/NodeApi/Interop/JSRuntimeContext.cs

@@ -8,6 +8,7 @@
 using System.IO;
 using System.Runtime.InteropServices;
 using System.Threading;
+using Microsoft.JavaScript.NodeApi.Runtime;
 using static Microsoft.JavaScript.NodeApi.Interop.JSCollectionProxies;
 using static Microsoft.JavaScript.NodeApi.JSNativeApi;
 using static Microsoft.JavaScript.NodeApi.Runtime.JSRuntime;
@@ -103,28 +104,53 @@ public sealed class JSRuntimeContext : IDisposable
 
     private readonly ConcurrentDictionary<Type, JSProxy.Handler> _collectionProxyHandlerMap = new();
 
-    public bool IsDisposed { get; private set; }
+    internal napi_env EnvironmentHandle
+    {
+        get
+        {
+            if (IsDisposed)
+            {
+                throw new ObjectDisposedException(nameof(JSRuntimeContext));
+            }
+
+            return _env;
+        }
+    }
+
+    public static explicit operator napi_env(JSRuntimeContext context)
+    {
+        if (context is null) throw new ArgumentNullException(nameof(context));
+        return context.EnvironmentHandle;
+    }
 
-    public static explicit operator napi_env(JSRuntimeContext context) => context?._env ??
-        throw new ArgumentNullException(nameof(context));
     public static explicit operator JSRuntimeContext(napi_env env)
         => GetInstanceData(env) as JSRuntimeContext
            ?? throw new InvalidCastException("Context is not found in napi_env instance data.");
 
+    public bool IsDisposed { get; private set; }
+
     /// <summary>
     /// Gets the current runtime context.
     /// </summary>
     /// <exception cref="InvalidOperationException">No runtime context was set for the current
     /// thread.</exception>
     public static JSRuntimeContext Current => JSValueScope.Current.RuntimeContext;
 
+    public JSRuntime Runtime { get; }
+
     public JSSynchronizationContext SynchronizationContext { get; }
 
-    public JSRuntimeContext(napi_env env)
+    internal JSRuntimeContext(
+        napi_env env,
+        JSRuntime runtime,
+        JSSynchronizationContext? synchronizationContext = null)
     {
+        if (env.IsNull) throw new ArgumentNullException(nameof(env));
+
         _env = env;
+        Runtime = runtime;
         SetInstanceData(env, this);
-        SynchronizationContext = JSSynchronizationContext.Create();
+        SynchronizationContext = synchronizationContext ?? JSSynchronizationContext.Create();
     }
 
     /// <summary>
@@ -692,22 +718,4 @@ internal void FreeGCHandle(GCHandle handle)
 
         handle.Free();
     }
-
-    /// <summary>
-    /// Frees a GC handle previously allocated via <see cref="AllocGCHandle(object)" />
-    /// and tracked on the runtime context obtained from environment instance data.
-    /// </summary>
-    /// <exception cref="InvalidOperationException">The handle was not previously allocated
-    /// by <see cref="AllocGCHandle(object)" />, or was already freed.</exception>
-    internal static void FreeGCHandle(GCHandle handle, napi_env env)
-    {
-        if (GetInstanceData(env) is JSRuntimeContext runtimeContext)
-        {
-            runtimeContext.FreeGCHandle(handle);
-        }
-        else
-        {
-            handle.Free();
-        }
-    }
 }

src/NodeApi/Interop/JSSynchronizationContext.cs

@@ -7,6 +7,25 @@
 
 namespace Microsoft.JavaScript.NodeApi.Interop;
 
+/// <summary>
+/// Manages the synchronization context for a JavaScript environment, allowing callbacks and
+/// asynchronous continuations to be invoked on the JavaScript thread that runs the environment.
+/// </summary>
+/// <remarks>
+/// All JavaScript values are bound to the thread that runs the JS environment and can only be
+/// accessed from the same thread. Attempts to access a JavaScript value from a different thread
+/// will throw <see cref="JSInvalidThreadAccessException" />.
+/// <para/>
+/// Use of <see cref="Task.ConfigureAwait(bool)"/> with <c>continueOnCapturedContext:false</c>
+/// can prevent execution from returning to the JS thread, though it isn't necessarily a problem
+/// as long as there is a top-level continuation that uses <c>continueOnCapturedContext:true</c>
+/// (the default) to return to the JS thread.
+/// <para/>
+/// Code that makes explicit use of .NET threads or thread pools may need to capture the
+/// <see cref="JSSynchronizationContext.Current" /> context (before switching off the JS thread)
+/// and hold it for later use to call back to JS via <see cref="JSSynchronizationContext.Post"/>,
+/// <see cref="JSSynchronizationContext.Run"/>, or <see cref="JSSynchronizationContext.RunAsync"/>.
+/// </remarks>
 public abstract class JSSynchronizationContext : SynchronizationContext, IDisposable
 {
     public bool IsDisposed { get; private set; }
@@ -224,7 +243,7 @@ public Task<T> RunAsync<T>(Func<Task<T>> asyncAction)
     }
 }
 
-public sealed class JSTsfnSynchronizationContext : JSSynchronizationContext
+internal sealed class JSTsfnSynchronizationContext : JSSynchronizationContext
 {
     private readonly JSThreadSafeFunction _tsfn;
 
@@ -233,7 +252,7 @@ public JSTsfnSynchronizationContext()
         _tsfn = new JSThreadSafeFunction(
             maxQueueSize: 0,
             initialThreadCount: 1,
-            asyncResourceName: (JSValue)"SynchronizationContext");
+            asyncResourceName: (JSValue)nameof(JSSynchronizationContext));
 
         // Unref TSFN to indicate that this TSFN is not preventing Node.JS shutdown.
         _tsfn.Unref();
@@ -295,7 +314,7 @@ public override void Send(SendOrPostCallback callback, object? state)
     }
 }
 
-public sealed class JSDispatcherSynchronizationContext : JSSynchronizationContext
+internal sealed class JSDispatcherSynchronizationContext : JSSynchronizationContext
 {
     private readonly JSDispatcherQueue _queue;
 

src/NodeApi/Interop/JSThreadSafeFunction.cs

@@ -235,7 +235,7 @@ private static unsafe void CustomCallJS(napi_env env, napi_value jsCallback, nin
 
         try
         {
-            using JSValueScope scope = new(JSValueScopeType.Callback, env);
+            using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null);
 
             object? callbackData = null;
             if (data != default)
@@ -267,7 +267,7 @@ private static unsafe void DefaultCallJS(napi_env env, napi_value jsCallback, ni
 
         try
         {
-            using JSValueScope scope = new(JSValueScopeType.Callback, env);
+            using JSValueScope scope = new(JSValueScopeType.Callback, env, runtime: null);
 
             if (data != default)
             {
@@ -299,6 +299,9 @@ private static unsafe void DefaultCallJS(napi_env env, napi_value jsCallback, ni
         }
         catch (Exception ex)
         {
+#if DEBUG
+            Console.Error.WriteLine(ex);
+#endif
             JSError.Fatal(ex.Message);
         }
     }

src/NodeApi/JSException.cs

@@ -7,7 +7,7 @@ namespace Microsoft.JavaScript.NodeApi;
 
 /// <summary>
 /// An exception that was caused by an error thrown by JavaScript code or
-/// interactions with the JavaScript engine.
+/// interactions with JavaScript objects.
 /// </summary>
 public class JSException : Exception
 {

src/NodeApi/JSInvalidThreadAccessException.cs

@@ -0,0 +1,87 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+using System;
+using System.Threading;
+using Microsoft.JavaScript.NodeApi.Interop;
+
+namespace Microsoft.JavaScript.NodeApi;
+
+/// <summary>
+/// An exception that was caused by an attempt to access a JavaScript value without any
+/// <see cref="JSValueScope" /> established on the current thread, or from a thread associated
+/// with a different environment / root scope.
+/// </summary>
+/// <remarks>
+/// All JavaScript values are created within a scope that is bound to the thread that runs the
+/// JS environment. They can only be accessed from the same thread and only as long as the scope
+/// is still valid (not disposed).
+/// </remarks>
+/// <seealso cref="JSSynchronizationContext"/>
+public class JSInvalidThreadAccessException : InvalidOperationException
+{
+    /// <summary>
+    /// Creates a new instance of <see cref="JSInvalidThreadAccessException" /> with a
+    /// current scope and message.
+    /// </summary>
+    public JSInvalidThreadAccessException(
+        JSValueScope? currentScope,
+        string? message = null)
+        : this(currentScope, targetScope: null, message)
+    {
+    }
+
+    /// <summary>
+    /// Creates a new instance of <see cref="JSInvalidThreadAccessException" /> with current
+    /// and target scopes and a message.
+    /// </summary>
+    public JSInvalidThreadAccessException(
+        JSValueScope? currentScope,
+        JSValueScope? targetScope,
+        string? message = null)
+        : base(message ?? GetMessage(currentScope, targetScope))
+    {
+        CurrentScope = currentScope;
+        TargetScope = targetScope;
+    }
+
+    /// <summary>
+    /// Gets the scope associated with the current thread (<see cref="JSValueScope.Current" />)
+    /// when the exception was thrown, or null if there was no scope for the thread.
+    /// </summary>
+    public JSValueScope? CurrentScope { get; }
+
+    /// <summary>
+    /// Gets the scope of the value (<see cref="JSValue.Scope" />) that was being accessed when
+    /// the exception was thrown, or null if a static operation was attempted.
+    /// </summary>
+    public JSValueScope? TargetScope { get; }
+
+    private static string GetMessage(JSValueScope? currentScope, JSValueScope? targetScope)
+    {
+        int threadId = Environment.CurrentManagedThreadId;
+        string? threadName = Thread.CurrentThread.Name;
+        string threadDescription = string.IsNullOrEmpty(threadName) ?
+            $"#{threadId}" : $"#{threadId} \"{threadName}\"";
+
+        if (targetScope == null)
+        {
+            // If the target scope is null, then this was an attempt to access either a static
+            // operation or a JS reference (which has an environment but no scope).
+            if (currentScope != null)
+            {
+                // In that case if the current scope is NOT null this exception
+                // shouldn't be thrown.
+                throw new ArgumentException("Current scope must be null if target scope is null.");
+            }
+
+            return $"There is no active JS value scope.\nCurrent thread: {threadDescription}. " +
+                $"Consider using the synchronization context to switch to the JS thread.";
+        }
+
+        return "The JS value scope cannot be accessed from the current thread.\n" +
+            $"The scope of type {targetScope.ScopeType} was created on thread" +
+            $"#{targetScope.ThreadId} and is being accessed from {threadDescription}. " +
+            $"Consider using the synchronization context to switch to the JS thread.";
+    }
+}

src/NodeApi/JSProxy.cs

@@ -72,7 +72,7 @@ public JSProxy(
     /// <exception cref="InvalidOperationException">The proxy is not revocable.</exception>
     public void Revoke()
     {
-        if (!_revoke.Handle.HasValue)
+        if (_revoke == default)
         {
             throw new InvalidOperationException("Proxy is not revokable.");
         }