Fix memory leak from marshalling objects (#252)

src/NodeApi/Interop/JSCallbackDescriptor.cs

@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Diagnostics;
 
 namespace Microsoft.JavaScript.NodeApi.Interop;
 
@@ -10,6 +11,7 @@ namespace Microsoft.JavaScript.NodeApi.Interop;
 /// callback or standalone function callback. Enables passing a data object via the callback
 /// args data.
 /// </summary>
+[DebuggerDisplay("{Name,nq}()")]
 public readonly struct JSCallbackDescriptor
 {
     /// <summary>

src/NodeApi/Interop/JSRuntimeContext.cs

@@ -6,6 +6,8 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
+using System.Linq;
+using System.Runtime.CompilerServices;
 using System.Runtime.InteropServices;
 using System.Threading;
 using System.Threading.Tasks;
@@ -66,7 +68,8 @@ public sealed class JSRuntimeContext : IDisposable
     private readonly ConcurrentDictionary<string, JSReference> _staticClassMap = new();
 
     /// <summary>
-    /// Maps from C# class objects to (weak references to) JS wrappers for each object.
+    /// Maps from (weak references to) C# class objects to (weak references to) JS wrappers for
+    /// each object.
     /// </summary>
     /// <remarks>
     /// Enables re-using the same JS wrapper objects for the same C# objects, so that
@@ -75,8 +78,7 @@ public sealed class JSRuntimeContext : IDisposable
     /// re-constructed as needed. This is not used with C# structs which are always
     /// passed to/from JS by value.
     /// </remarks>
-    private readonly ConcurrentDictionary<object, JSReference> _objectMap
-        = new(ReferenceEqualityComparer.Instance);
+    private readonly ConditionalWeakTable<object, JSReference> _objectMap = new();
 
     /// <summary>
     /// Maps from exported struct types to (strong references to) JS constructors for classes
@@ -307,14 +309,9 @@ internal JSValue InitializeObjectWrapper(JSValue wrapper, JSValue externalInstan
         // GetOrCreateObjectWrapper() will create a new JS wrapper if requested.
         wrapper.Wrap(obj, out JSReference wrapperWeakRef);
 
-        _objectMap.AddOrUpdate(
-            obj,
-            (_) => wrapperWeakRef,
-            (_, oldReference) =>
-            {
-                oldReference.Dispose();
-                return wrapperWeakRef;
-            });
+        // There should not be an existing wrapper in the map, because this is the
+        // first initialization of a wrapper for the .NET object.
+        _objectMap.Add(obj, wrapperWeakRef);
 
         return wrapper;
     }
@@ -333,7 +330,7 @@ public JSValue GetOrCreateObjectWrapper<T>(T obj) where T : class
     {
         if (obj == null)
         {
-            // Marshal null object reference to JS undefined. 
+            // Marshal null object reference to JS undefined.
             return JSValue.Undefined;
         }
 
@@ -357,27 +354,36 @@ JSReference CreateWrapper(T obj)
             return new(wrapper.Value, isWeak: true);
         }
 
-        _objectMap.AddOrUpdate(
-            obj,
-            (_) =>
-            {
-                // No wrapper was found in the map for the object. Create a new one.
-                return CreateWrapper(obj);
-            },
-            (_, wrapperReference) =>
-            {
-                wrapper = wrapperReference.GetValue();
-                if (wrapper.HasValue)
-                {
-                    // A valid reference was found in the map. Return it to keep the same mapping.
-                    return wrapperReference;
-                }
+        if (!_objectMap.TryGetValue(obj, out JSReference? wrapperReference))
+        {
+            // No wrapper was found in the map for the object. Create a new one.
+            wrapperReference = CreateWrapper(obj);
 
+            // Use AddOrUpdate() in case the constructor just added the object.
+#if NETFRAMEWORK
+            _objectMap.Remove(obj);
+            _objectMap.Add(obj, wrapperReference);
+#else
+            _objectMap.AddOrUpdate(obj, wrapperReference);
+#endif
+        }
+        else
+        {
+            wrapper = wrapperReference.GetValue();
+            if (!wrapper.HasValue)
+            {
                 // A reference was found in the map, but the JS object was released.
-                // Create a new wrapper JS object and update the reference in the map.l
+                // Create a new wrapper JS object and update the reference in the map.
                 wrapperReference.Dispose();
-                return CreateWrapper(obj);
-            });
+                wrapperReference = CreateWrapper(obj);
+#if NETFRAMEWORK
+                _objectMap.Remove(obj);
+                _objectMap.Add(obj, wrapperReference);
+#else
+                _objectMap.AddOrUpdate(obj, wrapperReference);
+#endif
+            }
+        }
 
         return wrapper!.Value;
     }
@@ -541,29 +547,30 @@ private JSValue GetOrCreateCollectionProxy(
     {
         JSValue? wrapper = null;
 
-        _objectMap.AddOrUpdate(
-            collection,
-            (_) =>
-            {
-                // No wrapper was found in the map for the object. Create a new one.
-                wrapper = createWrapper();
-                return new JSReference(wrapper.Value, isWeak: true);
-            },
-            (_, wrapperReference) =>
+        if (!_objectMap.TryGetValue(collection, out JSReference? wrapperReference))
+        {
+            // No wrapper was found in the map for the object. Create a new one.
+            wrapper = createWrapper();
+            _objectMap.Add(collection, new JSReference(wrapper.Value, isWeak: true));
+        }
+        else
+        {
+            wrapper = wrapperReference.GetValue();
+            if (!wrapper.HasValue)
             {
-                wrapper = wrapperReference.GetValue();
-                if (wrapper.HasValue)
-                {
-                    // A valid reference was found in the map. Return it to keep the same mapping.
-                    return wrapperReference;
-                }
-
                 // A reference was found in the map, but the JS object was released.
                 // Create a new wrapper JS object and update the reference in the map.
                 wrapperReference.Dispose();
                 wrapper = createWrapper();
-                return new JSReference(wrapper.Value, isWeak: true);
-            });
+                wrapperReference = new JSReference(wrapper.Value, isWeak: true);
+#if NETFRAMEWORK
+                _objectMap.Remove(collection);
+                _objectMap.Add(collection, wrapperReference);
+#else
+                _objectMap.AddOrUpdate(collection, wrapperReference);
+#endif
+            }
+        }
 
         return wrapper!.Value;
     }
@@ -707,16 +714,21 @@ public void Dispose()
         IsDisposed = true;
 
         SynchronizationContext.Dispose();
-        DisposeReferences(_objectMap);
-        DisposeReferences(_classMap);
-        DisposeReferences(_staticClassMap);
-        DisposeReferences(_structMap);
+
+#if !NETFRAMEWORK
+        // ConditionalWeakTable<> is not enumerable in .NET Framework.
+        // The JS references will still be released eventually by their finalizers.
+        DisposeReferences(_objectMap.Select((entry) => entry.Value));
+#endif
+        DisposeReferences(_classMap.Values);
+        DisposeReferences(_staticClassMap.Values);
+        DisposeReferences(_structMap.Values);
     }
 
-    private static void DisposeReferences<TKey>(
-        ConcurrentDictionary<TKey, JSReference> references) where TKey : notnull
+    private static void DisposeReferences(
+        IEnumerable<JSReference> references)
     {
-        foreach (JSReference reference in references.Values)
+        foreach (JSReference reference in references)
         {
             try
             {
@@ -726,8 +738,6 @@ private static void DisposeReferences<TKey>(
             {
             }
         }
-
-        references.Clear();
     }
 
 

src/NodeApi/JSPropertyDescriptor.cs

@@ -2,10 +2,12 @@
 // Licensed under the MIT License.
 
 using System;
+using System.Diagnostics;
 using Microsoft.JavaScript.NodeApi.Interop;
 
 namespace Microsoft.JavaScript.NodeApi;
 
+[DebuggerDisplay("{Name,nq}")]
 public readonly struct JSPropertyDescriptor
 {
     /// <summary>

src/NodeApi/Runtime/NodejsEnvironment.cs

@@ -57,17 +57,12 @@ internal NodejsEnvironment(NodejsPlatform platform, string? baseDir, string? mai
             scope = new JSValueScope(JSValueScopeType.Root, env, platform.Runtime);
             syncContext = scope.RuntimeContext.SynchronizationContext;
 
-            if (string.IsNullOrEmpty(baseDir))
-            {
-                baseDir = ".";
-            }
-            else
+            if (!string.IsNullOrEmpty(baseDir))
             {
                 JSValue.Global.SetProperty("__dirname", baseDir!);
+                InitializeModuleImportFunctions(scope.RuntimeContext, baseDir!);
             }
 
-            InitializeModuleImportFunctions(scope.RuntimeContext, baseDir!);
-
             loadedEvent.Set();
 
             // Run the JS event loop until disposal completes the completion source.

src/NodeApi/Runtime/NodejsPlatform.cs

@@ -78,7 +78,7 @@ public void Dispose()
     /// </summary>
     /// <param name="baseDir">Optional directory that is used as the base directory when resolving
     /// imported modules, and also as the value of the global `__dirname` property. If unspecified,
-    /// modules are resolved relative to the process CWD and `__dirname` is undefined.</param>
+    /// importing modules is not enabled and `__dirname` is undefined.</param>
     /// <param name="mainScript">Optional script to run in the environment. (Literal script content,
     /// not a path to a script file.)</param>
     /// <returns>A new <see cref="NodejsEnvironment" /> instance.</returns>

test/GCTests.cs

@@ -13,15 +13,14 @@ public class GCTests
     private static string LibnodePath { get; } = GetLibnodePath();
 
     [SkippableFact]
-    public void GCTest()
+    public void GCHandles()
     {
         Skip.If(
             NodejsEmbeddingTests.NodejsPlatform == null,
             "Node shared library not found at " + LibnodePath);
         using NodejsEnvironment nodejs = NodejsEmbeddingTests.NodejsPlatform.CreateEnvironment();
 
-        long gcHandleCount = 0;
-        nodejs.SynchronizationContext.Run(() =>
+        nodejs.Run(() =>
         {
             Assert.Equal(0, JSRuntimeContext.Current.GCHandleCount);
 
@@ -31,7 +30,7 @@ public void GCTest()
                 "property",
                 (x) => x.Property,
                 (x, value) => x.Property = (string)value);
-            classBuilder.AddMethod("method", (x) => (args) => DotnetClass.Method());
+            classBuilder.AddMethod("method", (x) => (args) => x.Method());
             JSObject dotnetClass = (JSObject)classBuilder.DefineClass();
 
             JSFunction jsCreateInstanceFunction = (JSFunction)JSValue.RunScript(
@@ -49,18 +48,70 @@ public void GCTest()
             using JSValueScope innerScope = new(JSValueScopeType.Callback);
             jsCreateInstanceFunction.CallAsStatic(dotnetClass);
 
-            gcHandleCount = JSRuntimeContext.Current.GCHandleCount;
+            // Two more handles should have been allocated by the JS create-instance function call.
+            // - One for the 'external' type value passed to the constructor.
+            // - One for the JS object wrapper.
+            Assert.Equal(7, JSRuntimeContext.Current.GCHandleCount);
         });
 
-        // Some GC handles should have been allocated by the JS create-instance function call.
-        Assert.True(gcHandleCount > 5);
+        nodejs.GC();
 
+        nodejs.Run(() =>
+        {
+            // After GC, the handle count should have reverted back to the original set.
+            Assert.Equal(5, JSRuntimeContext.Current.GCHandleCount);
+        });
+    }
+
+    [SkippableFact]
+    public void GCObjects()
+    {
+        Skip.If(
+            NodejsEmbeddingTests.NodejsPlatform == null,
+            "Node shared library not found at " + LibnodePath);
+        using NodejsEnvironment nodejs = NodejsEmbeddingTests.NodejsPlatform.CreateEnvironment();
+
+        nodejs.Run(() =>
+        {
+            JSClassBuilder<DotnetClass> classBuilder =
+                new(nameof(DotnetClass), () => new DotnetClass());
+            classBuilder.AddProperty(
+                "property",
+                (x) => x.Property,
+                (x, value) => x.Property = (string)value);
+            classBuilder.AddMethod("method", (x) => (args) => x.Method());
+            JSObject dotnetClass = (JSObject)classBuilder.DefineClass();
+
+            JSFunction jsCreateInstanceFunction = (JSFunction)JSValue.RunScript(
+                "function jsCreateInstanceFunction(Class) { new Class() }; " +
+                "jsCreateInstanceFunction");
+
+            Assert.Equal(5, JSRuntimeContext.Current.GCHandleCount);
+
+            using (JSValueScope innerScope = new(JSValueScopeType.Callback))
+            {
+                jsCreateInstanceFunction.CallAsStatic(dotnetClass);
+            }
+        });
+
+        // One .NET object instance was created by the JS function.
+        Assert.Equal(1ul, DotnetClass.Instances);
+
+        // Request a JS GC, which should release the JS object referencing the .NET object.
+        // Pump the Node event loop with an empty Run() callback to complete the GC.
         nodejs.GC();
+        nodejs.Run(() => { });
+
+        // The JS object released its reference to the .NET object, but it hasn't been GC'd yet.
+        Assert.Equal(1ul, DotnetClass.Instances);
+
+        // Request a .NET GC, and wait for finalizers (which run on another thread after the GC).
+        System.GC.Collect();
+        System.GC.WaitForPendingFinalizers();
 
-        // After GC, the handle count should have reverted back to the original set.
-        gcHandleCount = nodejs.SynchronizationContext.Run(
-                () => JSRuntimeContext.Current.GCHandleCount);
-        Assert.Equal(5, gcHandleCount);
+        // Now the .NET object should have been finalized/GC'd, as indicated by the
+        // instance count decremented by the finalizer.
+        Assert.Equal(0ul, DotnetClass.Instances);
     }
 
     private class DotnetClass
@@ -75,7 +126,7 @@ public DotnetClass()
         public string Property { get; set; } = string.Empty;
 
 #pragma warning disable CA1822 // Method does not access instance data and can be marked as static
-        public static void Method() { }
+        public void Method() { }
 #pragma warning restore CA1822
 
         ~DotnetClass()