Use WeakReference<T> in NativeToManagedMap to allow finalizers to run. (#1648)

src/Generator/Generators/C/CppHeaders.cs

@@ -469,7 +469,7 @@ namespace CppSharp.Generators.Cpp
                 if (destructor != null)
                 {
                     GenerateClassDestructor(@class);
-                    if (Options.GenerateFinalizers)
+                    if (Options.GenerateFinalizerFor(@class))
                         GenerateClassFinalizer(@class);
                 }
             }

src/Generator/Generators/C/CppSources.cs

@@ -173,7 +173,7 @@ namespace CppSharp.Generators.Cpp
                 {
                     GenerateClassDestructor(@class);
 
-                    if (Options.GenerateFinalizers)
+                    if (Options.GenerateFinalizerFor(@class))
                         GenerateClassFinalizer(@class);
                 }
             }

src/Generator/Generators/CLI/CLIHeaders.cs

@@ -400,7 +400,7 @@ namespace CppSharp.Generators.CLI
                 if (destructor != null)
                 {
                     GenerateClassDestructor(@class);
-                    if (Options.GenerateFinalizers)
+                    if (Options.GenerateFinalizerFor(@class))
                         GenerateClassFinalizer(@class);
                 }
             }

src/Generator/Generators/CLI/CLISources.cs

@@ -193,7 +193,7 @@ namespace CppSharp.Generators.CLI
                 if (destructor != null)
                 {
                     GenerateClassDestructor(@class);
-                    if (Options.GenerateFinalizers)
+                    if (Options.GenerateFinalizerFor(@class))
                         GenerateClassFinalizer(@class);
                 }
             }

src/Generator/Generators/CSharp/CSharpSources.cs

@@ -456,9 +456,44 @@ namespace CppSharp.Generators.CSharp
                     var @interface = @class.Namespace.Classes.FirstOrDefault(
                         c => c.IsInterface && c.OriginalClass == @class);
                     var printedClass = (@interface ?? @class).Visit(TypePrinter);
-                    var dict = $@"global::System.Collections.Concurrent.ConcurrentDictionary<IntPtr, {
-                        printedClass}>";
+                    var valueType = Options.GenerateFinalizerFor(@class) ? $"global::System.WeakReference<{printedClass}>" : $"{printedClass}";
+                    var dict = $@"global::System.Collections.Concurrent.ConcurrentDictionary<IntPtr, {valueType}>";
+
                     WriteLine("internal static readonly {0} NativeToManagedMap = new {0}();", dict);
+                    PopBlock(NewLineKind.BeforeNextBlock);
+
+                    // Create a method to record/recover a mapping to make it easier to call from template instantiation
+                    // implementations. If finalizers are in play, use weak references; otherwise, the finalizer
+                    // will never get invoked: unless the managed object is Disposed, there will always be a reference
+                    // in the dictionary.
+                    PushBlock(BlockKind.Method);
+                    if (Options.GenerateFinalizerFor(@class))
+                    {
+                        WriteLines($@"
+internal static void {Helpers.RecordNativeToManagedMappingIdentifier}(IntPtr native, {printedClass} managed)
+{{
+    NativeToManagedMap[native] = new global::System.WeakReference<{printedClass}>(managed);
+}}
+
+internal static bool {Helpers.TryGetNativeToManagedMappingIdentifier}(IntPtr native, out {printedClass} managed)
+{{
+    managed = default;
+    return NativeToManagedMap.TryGetValue(native, out var wr) && wr.TryGetTarget(out managed);
+}}");
+                    } 
+                    else
+                    {
+                        WriteLines($@"
+internal static void {Helpers.RecordNativeToManagedMappingIdentifier}(IntPtr native, {printedClass} managed)
+{{
+    NativeToManagedMap[native] = managed;
+}}
+
+internal static bool {Helpers.TryGetNativeToManagedMappingIdentifier}(IntPtr native, out {printedClass} managed)
+{{
+    return NativeToManagedMap.TryGetValue(native, out managed);
+}}");
+                    }
                 }
                 PopBlock(NewLineKind.BeforeNextBlock);
             }
@@ -2194,7 +2229,9 @@ namespace CppSharp.Generators.CSharp
 
             if (@class.IsRefType)
             {
-                GenerateClassFinalizer(@class);
+                // We used to always call GenerateClassFinalizer here. However, the
+                // finalizer calls Dispose which is conditionally implemented below.
+                // Instead, generate the finalizer only if Dispose is also implemented.
 
                 // ensure any virtual dtor in the chain is called
                 var dtor = @class.Destructors.FirstOrDefault(d => d.Access != AccessSpecifier.Private);
@@ -2203,13 +2240,17 @@ namespace CppSharp.Generators.CSharp
                     // virtual destructors in abstract classes may lack a pointer in the v-table
                     // so they have to be called by symbol; thus we need an explicit Dispose override
                     @class.IsAbstract)
-                    if(!@class.IsOpaque)GenerateDisposeMethods(@class);
+                    if (!@class.IsOpaque)
+                    {
+                        GenerateClassFinalizer(@class);
+                        GenerateDisposeMethods(@class);
+                    }
             }
         }
 
-        private void GenerateClassFinalizer(INamedDecl @class)
+        private void GenerateClassFinalizer(Class @class)
         {
-            if (!Options.GenerateFinalizers)
+            if (!Options.GenerateFinalizerFor(@class))
                 return;
 
             PushBlock(BlockKind.Finalizer);
@@ -2234,7 +2275,7 @@ namespace CppSharp.Generators.CSharp
                 WriteOpenBraceAndIndent();
 
                 WriteLine($"Dispose(disposing: true, callNativeDtor : { Helpers.OwnsNativeInstanceIdentifier} );");
-                if (Options.GenerateFinalizers)
+                if (Options.GenerateFinalizerFor(@class))
                     WriteLine("GC.SuppressFinalize(this);");
 
                 UnindentAndWriteCloseBrace();
@@ -2401,11 +2442,11 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetOrCreateInst
 {{
     if (native == {TypePrinter.IntPtrType}.Zero)
         return null;
-    if (NativeToManagedMap.TryGetValue(native, out var managed))
+    if ({Helpers.TryGetNativeToManagedMappingIdentifier}(native, out var managed))
         return ({printedClass})managed;
     var result = {Helpers.CreateInstanceIdentifier}(native, skipVTables);
     if (saveInstance)
-        NativeToManagedMap[native] = result;
+        {Helpers.RecordNativeToManagedMappingIdentifier}(native, result);
     return result;
 }}");
                     NewLine();
@@ -2417,7 +2458,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetOrCreateInst
                         WriteLines($@"
 internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({TypePrinter.IntPtrType} native)
 {{
-    if (!NativeToManagedMap.TryGetValue(native, out var managed))
+    if (!{Helpers.TryGetNativeToManagedMappingIdentifier}(native, out var managed))
         throw new global::System.Exception(""No managed instance was found"");
     var result = ({printedClass})managed;
     if (result.{Helpers.OwnsNativeInstanceIdentifier})
@@ -2534,7 +2575,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({Ty
                 if (@class.IsRefType)
                 {
                     WriteLine($"{Helpers.OwnsNativeInstanceIdentifier} = true;");
-                    WriteLine($"NativeToManagedMap[{Helpers.InstanceIdentifier}] = this;");
+                    WriteLine($"{Helpers.RecordNativeToManagedMappingIdentifier}({Helpers.InstanceIdentifier}, this);");
                 }
                 else
                 {
@@ -2979,7 +3020,7 @@ internal static{(@new ? " new" : string.Empty)} {printedClass} __GetInstance({Ty
                 @class.IsAbstractImpl ? @class.BaseClass : @class);
             WriteLine($"{Helpers.InstanceIdentifier} = Marshal.AllocHGlobal(sizeof({@internal}));");
             WriteLine($"{Helpers.OwnsNativeInstanceIdentifier} = true;");
-            WriteLine($"NativeToManagedMap[{Helpers.InstanceIdentifier}] = this;");
+            WriteLine($"{Helpers.RecordNativeToManagedMappingIdentifier}({Helpers.InstanceIdentifier}, this);");
 
             if (method.IsCopyConstructor)
             {

src/Generator/Generators/CodeGenerator.cs

@@ -1302,6 +1302,8 @@ namespace CppSharp.Generators
 
         public static readonly string CreateInstanceIdentifier = Generator.GeneratedIdentifier("CreateInstance");
         public static readonly string GetOrCreateInstanceIdentifier = Generator.GeneratedIdentifier("GetOrCreateInstance");
+        public static readonly string RecordNativeToManagedMappingIdentifier = Generator.GeneratedIdentifier("RecordNativeToManagedMapping");
+        public static readonly string TryGetNativeToManagedMappingIdentifier = Generator.GeneratedIdentifier("TryGetNativeToManagedMapping");
 
         public static string GetSuffixForInternal(Class @class)
         {

src/Generator/Options.cs

@@ -114,11 +114,43 @@ namespace CppSharp
         public List<string> NoGenIncludeDirs;
 
         /// <summary>
-        /// Enable this option to enable generation of finalizers.
-        /// Works in both CLI and C# backends.
+        /// Enable this option to enable generation of finalizers. Works in both CLI and
+        /// C# backends. 
         /// </summary>
+        /// <remarks>
+        /// Use <see cref="GenerateFinalizersFilter"/> to specify a filter so that
+        /// finalizers are generated for only a subset of classes.
+        /// </remarks>
         public bool GenerateFinalizers;
 
+        /// <summary>
+        /// A filter that can restrict the classes for which finalizers are generated when
+        /// <see cref="GenerateFinalizers"/> is <c>true</c>. 
+        /// </summary>
+        /// <remarks>
+        /// The default filter performs no filtering so that whenever <see
+        /// cref="GenerateFinalizers"/> is <c>true</c>, finalizers will be generated for
+        /// all classes. If finalizers should be generated selectively, inspect the
+        /// supplied <see cref="Class"/> parameter and return <c>true</c> if a finalizer
+        /// should be generated and <c>false</c> if a finalizer should not be generated.
+        /// </remarks>
+        public Func<Class, bool> GenerateFinalizersFilter = (@class) => true;
+
+        /// <summary>
+        /// An internal convenience method that combines the effect of <see
+        /// cref="GenerateFinalizers"/> and <see cref="GenerateFinalizersFilter"/>.
+        /// </summary>
+        /// <param name="class">
+        /// The <see cref="Class"/> to be filtered by <see
+        /// cref="GenerateFinalizersFilter"/>.
+        /// </param>
+        /// <returns>
+        /// <c>true</c> if a finalizer should be generated for <paramref name="class"/>.
+        /// <c>false</c> if a finalizer should not be generated for <paramref
+        /// name="class"/>.
+        /// </returns>
+        internal bool GenerateFinalizerFor(Class @class) => GenerateFinalizers && GenerateFinalizersFilter(@class);
+
         /// <summary>
         /// If <see cref="ZeroAllocatedMemory"/> returns <c>true</c> for a given <see cref="Class"/>,
         /// unmanaged memory allocated in the class' default constructor will be initialized with

tests/CSharp/CSharp.CSharp.csproj

@@ -1 +1,5 @@
-<Project Sdk="Microsoft.NET.Sdk" />
+<Project Sdk="Microsoft.NET.Sdk">
+  <ItemGroup>
+    <Compile Include="CSharpPartialMethods.cs" />
+  </ItemGroup>
+</Project>

tests/CSharp/CSharp.Gen.cs

@@ -66,6 +66,10 @@ namespace CppSharp.Tests
                     Namespace = ctx.TranslationUnits.First(u => u.IsValid && !u.IsSystemHeader)
                 };
 
+            // Generate a finalizer for just the one test case.
+            driver.Options.GenerateFinalizers = true;
+            driver.Options.GenerateFinalizersFilter = (@class) => @class.Name == "TestFinalizer";
+
             // Preserve the original semantics except for our one test class.
             driver.Options.ZeroAllocatedMemory = (@class) => @class.Name == "ClassZeroAllocatedMemoryTest";
         }

tests/CSharp/CSharp.Tests.cs

@@ -520,6 +520,67 @@ public unsafe class CSharpTests
         Assert.That(CallDtorVirtually.Destroyed, Is.False);
     }
 
+    // Verify that the finalizer gets called if an instance is not disposed. Note that
+    // we've arranged to have the generator turn on finalizers for (just) the
+    // TestFinalizer class.
+    [Test]
+    public void TestFinalizerGetsCalledWhenNotDisposed()
+    {
+        using var callbackRegistration = new TestFinalizerDisposeCallbackRegistration();
+        var nativeAddr = CreateAndReleaseTestFinalizerInstance(false);
+
+        GC.Collect();
+        GC.WaitForPendingFinalizers();
+
+        Assert.That(callbackRegistration.NativeAddr, Is.EqualTo(nativeAddr));
+        Assert.That(callbackRegistration.IsDisposing, Is.False);
+    }
+
+    // Verify that the finalizer is not called if an instance is disposed. Note that we've
+    // arranged to have the generator turn on finalizers for (just) the TestFinalizer
+    // class.
+    [Test]
+    public void TestFinalizerNotCalledWhenDisposed()
+    {
+        using var callbackRegistration = new TestFinalizerDisposeCallbackRegistration();
+        var nativeAddr = CreateAndReleaseTestFinalizerInstance(true);
+
+        GC.Collect();
+        GC.WaitForPendingFinalizers();
+
+        Assert.That(callbackRegistration.NativeAddr, Is.EqualTo(nativeAddr));
+        Assert.That(callbackRegistration.IsDisposing, Is.True);
+    }
+
+    // Empirically, the finalizer won't release a reference until the stack frame in which
+    // the reference was created has been released. Use this method to create/release the
+    // instance.
+    private Int64 CreateAndReleaseTestFinalizerInstance(bool dispose)
+    {
+        var instance = new TestFinalizer();
+        Assert.That(instance.__Instance, !Is.EqualTo(IntPtr.Zero));
+        var nativeAddr = instance.__Instance.ToInt64();
+        if (dispose) instance.Dispose();
+        return nativeAddr;
+    }
+
+    class TestFinalizerDisposeCallbackRegistration : IDisposable
+    {
+        public bool? IsDisposing = null;
+        public long? NativeAddr = null;
+
+        public TestFinalizerDisposeCallbackRegistration()
+        {
+            TestFinalizer.DisposeCallback = (isDisposing, intPtr) =>
+            {
+                IsDisposing = isDisposing;
+                NativeAddr = intPtr.ToInt64();
+            };
+        }
+
+        public void Dispose() => TestFinalizer.DisposeCallback = null;
+    }
+
     [Test]
     public void TestParamTypeToInterfacePass()
     {

tests/CSharp/CSharp.h

@@ -1131,6 +1131,11 @@ public:
     const char16_t* RetrieveString();
 };
 
+class DLL_API TestFinalizer
+{
+public:
+    const int Data[1024];
+};
 
 DLL_API void decltypeFunctionPointer();
 

tests/CSharp/CSharpPartialMethods.cs

@@ -0,0 +1,16 @@
+using System;
+using System.Collections.Generic;
+using System.Text;
+
+namespace CSharp
+{
+    public partial class TestFinalizer
+    {
+        public static Action<bool, IntPtr> DisposeCallback { get; set; }
+
+        partial void DisposePartial(bool disposing)
+        {
+            DisposeCallback?.Invoke(disposing, __Instance);
+        }
+    }
+}