[release/6.0] Notify Reference Tracker runtime of disconnect at the right time (...

* Notify Reference Tracker runtime of disconnect at the right time

Based on misunderstanding of SyncBlock clean-up modes the indication of
when an Native Object Wrapper was being collected was being done after
the wrapper's Finalizer was run. However, this information must be conveyed
prior to wrapper finalization and synchronously during the GC.

* Review feedback.

Co-authored-by: Aaron Robinson <arobins@microsoft.com>

diff --git a/src/coreclr/interop/comwrappers.cpp b/src/coreclr/interop/comwrappers.cpp
index 5600b6a69f50dd17192c282f0e2b222ddaced09a..aad3d6fa2359131c940fe5e078fdfc4470c141fa 100644 (file)
--- a/src/coreclr/interop/comwrappers.cpp
+++ b/src/coreclr/interop/comwrappers.cpp
@@ -791,17 +791,6 @@ void NativeObjectWrapperContext::Destroy(_In_ NativeObjectWrapperContext* wrappe
 {
     _ASSERTE(wrapper != nullptr);
 
-    // Check if the tracker object manager should be informed prior to being destroyed.
-    IReferenceTracker* trackerMaybe = wrapper->GetReferenceTracker();
-    if (trackerMaybe != nullptr)
-    {
-        // We only call this during a GC so ignore the failure as
-        // there is no way we can handle it at this point.
-        HRESULT hr = TrackerObjectManager::BeforeWrapperDestroyed(trackerMaybe);
-        _ASSERTE(SUCCEEDED(hr));
-        (void)hr;
-    }
-
     // Manually trigger the destructor since placement
     // new was used to allocate the object.
     wrapper->~NativeObjectWrapperContext();

diff --git a/src/coreclr/interop/comwrappers.hpp b/src/coreclr/interop/comwrappers.hpp
index bd383beabc5dce69908226345b2dafd818a296a7..6217b0ebe7e7b9fc28ce1b57f437d624796f8b50 100644 (file)
--- a/src/coreclr/interop/comwrappers.hpp
+++ b/src/coreclr/interop/comwrappers.hpp
@@ -212,8 +212,8 @@ public:
     // Called after wrapper has been created.
     static HRESULT AfterWrapperCreated(_In_ IReferenceTracker* obj);
 
-    // Called before wrapper is about to be destroyed (the same lifetime as short weak handle).
-    static HRESULT BeforeWrapperDestroyed(_In_ IReferenceTracker* obj);
+    // Called before wrapper is about to be finalized (the same lifetime as short weak handle).
+    static HRESULT BeforeWrapperFinalized(_In_ IReferenceTracker* obj);
 
 public:
     // Begin the reference tracking process for external objects.

diff --git a/src/coreclr/interop/inc/interoplib.h b/src/coreclr/interop/inc/interoplib.h
index 96f157929e0a5c192f451e913216a0f7e014e13e..37237f5f7cab3cc50c46c9a33c2c3814cb15d638 100644 (file)
--- a/src/coreclr/interop/inc/interoplib.h
+++ b/src/coreclr/interop/inc/interoplib.h
@@ -87,8 +87,12 @@ namespace InteropLib
             _In_ size_t contextSize,
             _Out_ ExternalWrapperResult* result) noexcept;
 
-        // Destroy the supplied wrapper.
-        void DestroyWrapperForExternal(_In_ void* context) noexcept;
+        // Inform the wrapper it is being collected.
+        void NotifyWrapperForExternalIsBeingCollected(_In_ void* context) noexcept;
+
+         // Destroy the supplied wrapper.
+        // Optionally notify the wrapper of collection at the same time.
+        void DestroyWrapperForExternal(_In_ void* context, _In_ bool notifyIsBeingCollected = false) noexcept;
 
         // Separate the supplied wrapper from the tracker runtime.
         void SeparateWrapperFromTrackerRuntime(_In_ void* context) noexcept;

diff --git a/src/coreclr/interop/interoplib.cpp b/src/coreclr/interop/interoplib.cpp
index 7af7af9e7adcf118930f6abfcf55753da8a64888..db9e9800fbd46e13657a3c8afee2b933f65c8471 100644 (file)
--- a/src/coreclr/interop/interoplib.cpp
+++ b/src/coreclr/interop/interoplib.cpp
@@ -166,15 +166,37 @@ namespace InteropLib
             return S_OK;
         }
 
-        void DestroyWrapperForExternal(_In_ void* contextMaybe) noexcept
+        void NotifyWrapperForExternalIsBeingCollected(_In_ void* contextMaybe) noexcept
+         {
+             NativeObjectWrapperContext* context = NativeObjectWrapperContext::MapFromRuntimeContext(contextMaybe);
+
+             // A caller should not be destroying a context without knowing if the context is valid.
+             _ASSERTE(context != nullptr);
+
+            // Check if the tracker object manager should be informed of collection.
+            IReferenceTracker* trackerMaybe = context->GetReferenceTracker();
+            if (trackerMaybe != nullptr)
+            {
+                // We only call this during a GC so ignore the failure as
+                // there is no way we can handle it at this point.
+                HRESULT hr = TrackerObjectManager::BeforeWrapperFinalized(trackerMaybe);
+                _ASSERTE(SUCCEEDED(hr));
+                (void)hr;
+            }
+        }
+
+        void DestroyWrapperForExternal(_In_ void* contextMaybe, _In_ bool notifyIsBeingCollected) noexcept
         {
             NativeObjectWrapperContext* context = NativeObjectWrapperContext::MapFromRuntimeContext(contextMaybe);
 
             // A caller should not be destroying a context without knowing if the context is valid.
             _ASSERTE(context != nullptr);
 
-            NativeObjectWrapperContext::Destroy(context);
-        }
+            if (notifyIsBeingCollected)
+                NotifyWrapperForExternalIsBeingCollected(contextMaybe);
+
+             NativeObjectWrapperContext::Destroy(context);
+         }
 
         void SeparateWrapperFromTrackerRuntime(_In_ void* contextMaybe) noexcept
         {

diff --git a/src/coreclr/interop/trackerobjectmanager.cpp b/src/coreclr/interop/trackerobjectmanager.cpp
index 0a199b71690a46d326373b5c95ff72504a36d46a..5214e6b8349d18b5f213475f0413a834ade8fb31 100644 (file)
--- a/src/coreclr/interop/trackerobjectmanager.cpp
+++ b/src/coreclr/interop/trackerobjectmanager.cpp
@@ -305,13 +305,13 @@ HRESULT TrackerObjectManager::AfterWrapperCreated(_In_ IReferenceTracker* obj)
     return S_OK;
 }
 
-HRESULT TrackerObjectManager::BeforeWrapperDestroyed(_In_ IReferenceTracker* obj)
+HRESULT TrackerObjectManager::BeforeWrapperFinalized(_In_ IReferenceTracker* obj)
 {
     _ASSERTE(obj != nullptr);
 
     HRESULT hr;
 
-    // Notify tracker runtime that we are about to destroy a wrapper
+    // Notify tracker runtime that we are about to finalize a wrapper
     // (same timing as short weak handle) for this object.
     // They need this information to disconnect weak refs and stop firing events,
     // so that they can avoid resurrecting the object.

diff --git a/src/coreclr/vm/interoplibinterface_comwrappers.cpp b/src/coreclr/vm/interoplibinterface_comwrappers.cpp
index 030cabfc3e92ea5db0c424d67a5eb4f7722d462c..500e8e88e9f9ffb43a578332db84e49a198707fa 100644 (file)
--- a/src/coreclr/vm/interoplibinterface_comwrappers.cpp
+++ b/src/coreclr/vm/interoplibinterface_comwrappers.cpp
@@ -176,7 +176,10 @@ namespace
             if (Result.Context != NULL)
             {
                 GCX_PREEMP();
-                InteropLib::Com::DestroyWrapperForExternal(Result.Context);
+                // We also request collection notification since this holder is presently
+                // only used for new activation of wrappers therefore the notification won't occur
+                // at the typical time of before finalization.
+                InteropLib::Com::DestroyWrapperForExternal(Result.Context, /* notifyIsBeingCollected */ true);
             }
         }
         InteropLib::Com::ExternalWrapperResult* operator&()
@@ -478,7 +481,11 @@ namespace
                 if (!cxt->IsSet(ExternalObjectContext::Flags_Detached)
                     && !GCHeapUtilities::GetGCHeap()->IsPromoted(OBJECTREFToObject(cxt->GetObjectRef())))
                 {
+                    // Indicate the wrapper entry has been detached.
                     cxt->MarkDetached();
+
+                    // Notify the wrapper it was not promoted and is being collected.
+                    InteropLib::Com::NotifyWrapperForExternalIsBeingCollected(cxt);
                 }
             }
         }

diff --git a/src/tests/Interop/COM/ComWrappers/API/Program.cs b/src/tests/Interop/COM/ComWrappers/API/Program.cs
index 52da8f8d060cfd8c0a1bb7195224fdc9ab24180f..98f09ac4d00a702e7647a8fe730746d36bd52419 100644 (file)
--- a/src/tests/Interop/COM/ComWrappers/API/Program.cs
+++ b/src/tests/Interop/COM/ComWrappers/API/Program.cs
@@ -624,6 +624,9 @@ namespace ComWrappersTests
                 ValidateQueryInterfaceAfterManagedObjectCollected();
                 ValidateAggregationWithComObject();
                 ValidateAggregationWithReferenceTrackerObject();
+
+                // Ensure all objects have been cleaned up.
+                ForceGC();
             }
             catch (Exception e)
             {

diff --git a/src/tests/Interop/COM/ComWrappers/Common.cs b/src/tests/Interop/COM/ComWrappers/Common.cs
index d100744cb271fc5ddb59a859ce12aba684aef7ee..bd4e7404ef202742e37e17f80711aad0d6943e84 100644 (file)
--- a/src/tests/Interop/COM/ComWrappers/Common.cs
+++ b/src/tests/Interop/COM/ComWrappers/Common.cs
@@ -156,6 +156,12 @@ namespace ComWrappersTests.Common
 
         [DllImport(nameof(MockReferenceTrackerRuntime))]
         extern public static int TrackerTarget_ReleaseFromReferenceTracker(IntPtr ptr);
+
+        // Suppressing the GC transition here as we want to make sure we are in-sync
+        // with the GC which is setting the connected value.
+        [SuppressGCTransition]
+        [DllImport(nameof(MockReferenceTrackerRuntime))]
+        extern public static byte IsTrackerObjectConnected(IntPtr instance);
     }
 
     [Guid("42951130-245C-485E-B60B-4ED4254256F8")]
@@ -219,6 +225,12 @@ namespace ComWrappersTests.Common
             }
             else
             {
+                byte isConnected = MockReferenceTrackerRuntime.IsTrackerObjectConnected(this.classNative.Instance);
+                if (isConnected != 0)
+                {
+                    throw new Exception("TrackerObject should be disconnected prior to finalization");
+                }
+
                 ComWrappersHelper.Cleanup(ref this.classNative);
             }
         }

diff --git a/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp b/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp
index 63a013b5cb484b891d64e1771ae8d6a1b601442c..cacd112eb7afb5d13d79d0e14d157380e2d1ecf2 100644 (file)
--- a/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp
+++ b/src/tests/Interop/COM/ComWrappers/MockReferenceTrackerRuntime/ReferenceTrackerRuntime.cpp
@@ -218,6 +218,11 @@ namespace
                 }
             }
 
+            bool IsConnected()
+            {
+                return _connected;
+            }
+
             STDMETHOD(AddObjectRef)(_In_ IUnknown* c, _Out_ int* id)
             {
                 assert(c != nullptr && id != nullptr);
@@ -546,6 +551,12 @@ extern "C" DLL_EXPORT int STDMETHODCALLTYPE Trigger_NotifyEndOfReferenceTracking
     return TrackerRuntimeManager.NotifyEndOfReferenceTrackingOnThread();
 }
 
+extern "C" DLL_EXPORT bool STDMETHODCALLTYPE IsTrackerObjectConnected(IUnknown* inst)
+{
+    auto trackerObject = reinterpret_cast<TrackerObject::TrackerObjectImpl*>(inst);
+    return trackerObject->IsConnected();
+}
+
 extern "C" DLL_EXPORT void* STDMETHODCALLTYPE TrackerTarget_AddRefFromReferenceTrackerAndReturn(IUnknown *obj)
 {
     assert(obj != nullptr);