[NativeAOT] allow creating CCW for resurrected objects

[AustinWise]

This PR adds a test that on main passes for CoreCLR but segfaults for NativeAOT. It also includes fixes to make the test pass for NativeAOT.

The key problem is the conditional weak table will keep the ManagedObjectWrapperHolder alive but not prevent it from being finalized. This means that if the wrapped object is resurrected, calls to GetOrCreateComInterfaceForObject could return a pointer freed memory.

The solution this PR implements is to move the finialization of the ManagedObjectWrapper to a separate class. This class defers finialization until the wrapped object has been completely collected. The new ManagedObjectWrapperReleaser is referenced by the ManagedObjectWrapperHolder class, so it should not be finialized until the wrapped object becomes eligible for finialization.

The references between objects looks like this:

graph TD
WO[wrapped object];
MOW["ManagedObjectWrapper (unmanaged struct)"];
MOWH[ManagedObjectWrapperHolder];
Releaser[ManagedObjectWrapperReleaser];
WO -->|conditional weak table| MOWH;
MOWH --> WO;
MOWH --> MOW;
MOW -->|ref-counted handle| MOWH;
Releaser --> MOW;
MOWH --> Releaser;

Loading

Some other race conditions are also fixed by this PR:

• Previously the ref-counted handle in the ManagedObjectWrapper was not set until after the ManagedObjectWrapperHolder was added to the CWT. Hypothetically if a holder was added to the CWT but not yet initialized with the ref-counted handle, another thread could read the object out of the CWT and observe the null handle. This is fixed by doing all the initialization of the holder in its constructor.
• The GetOrCreateComInterfaceForObject was assuming that ConditionalWeakTable.GetValue always returns a new object and calling AddRef on the holder was not needed. However, GetValue can potentially call TryGetValue and return an existing instance. This means that if many threads called GetOrCreateComInterfaceForObject at the same time, the ref count on the ManagedObjectWrapper might not be as high as it should be. This is fixed by unconditionally calling AddRef on the holder.

[MichalStrehovsky]

Cc @dotnet/interop-contrib

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR adds a test that on main passes for CoreCLR but fails for NativeAOT. It also includes fixes to make the test pass for NativeAOT.

The key problem is the conditional weak table will keep the ManagedObjectWrapperHolder alive but not prevent it from being finalized. This means that if the wrapped object is resurrected, calls to GetOrCreateComInterfaceForObject could return a pointer freed memory.

The solution this PR implements is to extend the lifetime of the ManagedObjectWrapperHolder and ManagedObjectWrapper as long as the wrapped object is alive. The responsibility for the keeping the wrapped object alive as long as the MOW has a non-zero refcount is separated out into a new WrappedObjectHolder class. This object is kept alive for the duration of the wrapped object's lifetime by adding a dependent handle to the to the ManagedObjectWrapperHolder. The ManagedObjectWrapperHolder also uses this dependent handle to detect whether the wrapped object is still alive and delay finalization until the wrapped object is completely dead.

The references between objects looks like this:

graph TD
WO[wrapped object];
MOW["ManagedObjectWrapper (unmanaged struct)"];
MOWH[ManagedObjectWrapperHolder];
WOH[WrappedObjectHolder];
WO -->|CWT| MOWH;
MOWH --> MOW;
MOW -->|ref-counted handle| WOH;
WOH --> WO;
WO -->|dependant handle owned by MOWH| WOH;

Loading

Some other race conditions are also fixed by this PR:

• Previously the ref-counted handle in the ManagedObjectWrapper was not set until after the ManagedObjectWrapperHolder was added to the CWT. Hypothetically if a holder was added to the CWT but not yet initialized with the ref-counted handle, another thread could read the object out of the CWT and observe the null handle. This is fixed by doing all the initialization of the holder in its constructor.
• The GetOrCreateComInterfaceForObject was assuming that ConditionalWeakTable.GetValue always returns a new object and calling AddRef on the holder was not needed. However, GetValue can potentially call TryGetValue and return an existing instance. This means that if many threads called GetOrCreateComInterfaceForObject at the same time, the ref count on the ManagedObjectWrapper might not be as high as it should be. This is fixed by unconditionally calling AddRef on the holder and removing the superfluous call to TryGetValue in GetOrCreateComInterfaceForObject.

Author:	AustinWise
Assignees:	-
Labels:	community-contribution, area-NativeAOT-coreclr
Milestone:	-

[AaronRobinsonMSFT]

I will need to take some time to look at this. Based on a quick look at the test I'm not sure I understand what this is validating.

[AustinWise]

Based on a quick look at the test I'm not sure I understand what this is validating.

On the main branch, the test passes on CoreClr but segfaults on NativeAOT. This is because the ManagedObjectWrapperHolder gets finalized and frees the ManagedObjectWrapper too early. I have updated the description of the PR to be a little more clear about this.

[AustinWise]

I updated how the responsibility is split for keeping objects alive and collecting them. I think this new design is much easier to understand. The description of the PR has been updated.

The main problem this PR does not address is the issue of extra ManagedObjectWrappers hanging around for the lifetime of a wrapped object when there is a race condition adding values to the _ccwTable.

[AustinWise]

I realized that I did not need to use an extra handle to monitor the lifetime of the holder object from the releaser object. Instead I could just use the ref counted handle. That saves allocating an extra GC handle.

[jkoritzinsky]

This looks good to me, but I want @AaronRobinsonMSFT to take another look.

[AaronRobinsonMSFT]

The key problem is the conditional weak table will keep the ManagedObjectWrapperHolder alive but not prevent it from being finalized.

My biggest issue here is the above statement. If an object is "alive", I assume "reachable", then it will not be finalized. The GC only adds objects to the finalizer queue if they are no longer reachable so I am confused by this statement.

[AaronRobinsonMSFT]

The key problem is the conditional weak table will keep the ManagedObjectWrapperHolder alive but not prevent it from being finalized.

My biggest issue here is the above statement. If an object is "alive", I assume "reachable", then it will not be finalized. The GC only adds objects to the finalizer queue if they are no longer reachable so I am confused by this statement.

Oh right. The conditional weak table doesn't keep it alive, merely a reference to acquire. Its state is indeterminant in this case and that is the issue you are attempting to mitigate. This is simply loose terminology.