Fix incorrect dtor call for non-owned instances (#1615)

* Introduce failing test for disposing bug

* Dispose(bool, bool) really shouldn't be public. Clients should call Dipose() instead.
Derived classes may need to call Dispose(bool, bool) so lets make it internal protected.

Author: szilvaa

src/Generator/Generators/CSharp/CSharpSources.cs

@@ -1879,7 +1879,7 @@ private void GenerateVTableManagedCall(Method method)
         {
             if (method.IsDestructor)
             {
-                WriteLine("{0}.Dispose(true);", Helpers.TargetIdentifier);
+                WriteLine("{0}.Dispose(disposing: true, callNativeDtor: true);", Helpers.TargetIdentifier);
                 return;
             }
 
@@ -2200,7 +2200,7 @@ private void GenerateClassFinalizer(INamedDecl @class)
 
             WriteLine("~{0}()", @class.Name);
             WriteOpenBraceAndIndent();
-            WriteLine("Dispose(false);");
+            WriteLine($"Dispose(false, callNativeDtor : { Helpers.OwnsNativeInstanceIdentifier} );");
             UnindentAndWriteCloseBrace();
 
             PopBlock(NewLineKind.BeforeNextBlock);
@@ -2217,21 +2217,27 @@ private void GenerateDisposeMethods(Class @class)
                 WriteLine("public void Dispose()");
                 WriteOpenBraceAndIndent();
 
-                WriteLine("Dispose(disposing: true);");
+                WriteLine($"Dispose(disposing: true, callNativeDtor : { Helpers.OwnsNativeInstanceIdentifier} );");
                 if (Options.GenerateFinalizers)
                     WriteLine("GC.SuppressFinalize(this);");
 
                 UnindentAndWriteCloseBrace();
                 PopBlock(NewLineKind.BeforeNextBlock);
             }
 
-            // Generate Dispose(bool) method
+            // Declare partial method that the partial class can implement to participate
+            // in dispose.
             PushBlock(BlockKind.Method);
-            Write("public ");
+            WriteLine("partial void DisposePartial(bool disposing);");
+            PopBlock(NewLineKind.BeforeNextBlock);
+
+            // Generate Dispose(bool, bool) method
+            PushBlock(BlockKind.Method);
+            Write("internal protected ");
             if (!@class.IsValueType)
                 Write(hasBaseClass ? "override " : "virtual ");
 
-            WriteLine("void Dispose(bool disposing)");
+            WriteLine("void Dispose(bool disposing, bool callNativeDtor )");
             WriteOpenBraceAndIndent();
 
             if (@class.IsRefType)
@@ -2251,6 +2257,13 @@ private void GenerateDisposeMethods(Class @class)
                 }
             }
 
+            // TODO: if disposing == true we should delegate to the dispose methods of references
+            // we hold to other generated type instances since those instances could also hold
+            // references to unmanaged memory.
+            //
+            // Delegate to partial method if implemented
+            WriteLine("DisposePartial(disposing);");
+
             var dtor = @class.Destructors.FirstOrDefault();
             if (dtor != null && dtor.Access != AccessSpecifier.Private &&
                 @class.HasNonTrivialDestructor && !@class.IsAbstract)
@@ -2259,7 +2272,29 @@ private void GenerateDisposeMethods(Class @class)
                 if (!Options.CheckSymbols ||
                     Context.Symbols.FindLibraryBySymbol(dtor.Mangled, out library))
                 {
-                    WriteLine("if (disposing)");
+                    // Normally, calling the native dtor should be controlled by whether or not we
+                    // we own the underlying instance. (i.e. Helpers.OwnsNativeInstanceIdentifier).
+                    // However, there are 2 situations when the caller needs to have direct control
+                    // 
+                    // 1. When we have a virtual dtor on the native side we detour the vtable entry
+                    // even when we don't own the underlying native instance. I think we do this
+                    // so that the managed side can null out the __Instance pointer and remove the
+                    // instance from the NativeToManagedMap. Of course, this is somewhat half-hearted
+                    // since we can't/don't do this when there's no virtual dtor available to detour.
+                    // Anyway, we must be able to call the native dtor in this case even if we don't 
+                    /// own the underlying native instance.
+                    //
+                    // 2. When we we pass a disposable object to a function "by value" then the callee
+                    // calls the dtor on the argument so our marshalling code must have a way from preventing
+                    // a duplicate call. Here's a native function that exhibits this behavior:
+                    // void f(std::string f)
+                    // {
+                    //   ....
+                    // compiler generates call to f.dtor() at the end of function
+                    // }
+                    // 
+                    // IDisposable.Dispose() and Object.Finalize() set callNativeDtor = Helpers.OwnsNativeInstanceIdentifier
+                    WriteLine($"if (callNativeDtor)");
                     if (@class.IsDependent || dtor.IsVirtual)
                         WriteOpenBraceAndIndent();
                     else
@@ -2282,9 +2317,6 @@ c is ClassTemplateSpecialization ?
             // unmanaged memory isn't always initialized and/or a reference may be owned by the
             // native side.
             //
-            // TODO: We should delegate to the dispose methods of references we hold to other
-            // generated type instances since those instances could also hold references to
-            // unmanaged memory.
             foreach (var field in @class.Layout.Fields.Where(f => f.QualifiedType.Type.IsConstCharString()))
             {
                 var ptr = $"(({Helpers.InternalStruct}*){Helpers.InstanceIdentifier})->{field.Name}";

src/Generator/Types/Std/Stdlib.CSharp.cs

@@ -343,7 +343,7 @@ public override void CSharpMarshalToNative(CSharpMarshalContext ctx)
                     assign.Name}({varBasicString}, {ctx.Parameter.Name});");
                 ctx.Return.Write($"{varBasicString}.{Helpers.InstanceIdentifier}");
                 ctx.Cleanup.WriteLine($@"{varBasicString}.Dispose({
-                    (!Type.IsAddress() || ctx.Parameter?.IsIndirect == true ? "false" : string.Empty)});");
+                    (!Type.IsAddress() || ctx.Parameter?.IsIndirect == true ? "disposing: true, callNativeDtor:false" : string.Empty)});");
             }
         }
 

tests/CSharp/CSharp.Tests.cs

@@ -497,6 +497,7 @@ public void TestNativeToManagedMapWithOwnObjects()
     [Test]
     public void TestCallingVirtualDtor()
     {
+        CallDtorVirtually.Destroyed = false;
         using (var callDtorVirtually = new CallDtorVirtually())
         {
             var hasVirtualDtor1 = CallDtorVirtually.GetHasVirtualDtor1(callDtorVirtually);
@@ -505,6 +506,15 @@ public void TestCallingVirtualDtor()
         Assert.That(CallDtorVirtually.Destroyed, Is.True);
     }
 
+    [Test]
+    public void TestNonOwning()
+    {
+        CallDtorVirtually.Destroyed = false;
+        var nonOwned = CallDtorVirtually.NonOwnedInstance;
+        nonOwned.Dispose();
+        Assert.That(CallDtorVirtually.Destroyed, Is.False);
+    }
+
     [Test]
     public void TestParamTypeToInterfacePass()
     {

tests/CSharp/CSharp.cpp

@@ -1019,6 +1019,12 @@ HasVirtualDtor1* CallDtorVirtually::getHasVirtualDtor1(HasVirtualDtor1* returned
     return returned;
 }
 
+CallDtorVirtually nonOwnedInstance;
+CallDtorVirtually* CallDtorVirtually::getNonOwnedInstance()
+{
+    return &nonOwnedInstance;
+}
+
 TestOverrideFromSecondaryBase::TestOverrideFromSecondaryBase()
 {
 }

tests/CSharp/CSharp.h

@@ -704,6 +704,7 @@ class DLL_API CallDtorVirtually : public HasVirtualDtor1
     ~CallDtorVirtually();
     static bool Destroyed;
     static HasVirtualDtor1* getHasVirtualDtor1(HasVirtualDtor1* returned);
+    static CallDtorVirtually* getNonOwnedInstance();
 };
 
 class HasProtectedNestedAnonymousType