Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ComWrappers supports controlled revoking of object mapping #51968

Closed
AaronRobinsonMSFT opened this issue Apr 28, 2021 · 2 comments
Closed

ComWrappers supports controlled revoking of object mapping #51968

AaronRobinsonMSFT opened this issue Apr 28, 2021 · 2 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr
Milestone

Comments

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Apr 28, 2021

Background and Motivation

The ComWrappers API has a deficiency/race during native object wrapper (RCW) cleanup. This is rare but has been observed in tight loops with complex object types. The scenario occurs when native object A is freed, its memory is reused for native object B, and that memory is provided back to the runtime but the previous mapping remains to the wrapper for native object A. The issue occurs based on an incorrect expectation between when the RCW's Finalizer will run and when the mapping between native IUnknown* to RCW will be removed from the internal cache. There is a workaround, but for .NET 6 we would like to improve this deficiency as the workaround is inefficient – see Current Workaround below.

Proposed API

namespace System.Runtime.InteropServices
{
    public abstract partial class ComWrappers
    {
+        /// <summary>
+        /// Revoke the created or registered object wrapper for the supplied native object.
+        /// </summary>
+        /// <param name="externalComObject">The native object.</param>
+        /// <returns>True if the mapping was remove, otherwise false.</returns>
+        public bool RevokeComInstanceMappingToObject(IntPtr externalComObject);
    }
}

Usage Examples

In the Finalizer of the RCW, the internal mapping between the native object and managed wrapper is revoked. After this mapping is revoked, the ComWrappers instance will no longer have a record of the mapping between the two objects. This means that if the previous memory is reused then the mapping will not exist and the ComWrappers APIs and a new wrapper will be created.

class RCW
{
    private IntPtr ptr;
    public RCW(IntPtr ptr) => this.ptr = ptr;
    ~RCW()
    {
        MyComWrappers.Instance.RevokeComInstanceMappingToObject(this.ptr);
        Marshal.Release(this.ptr);
    }
}

Current Workaround

Instead of immediately cleaning up the native object, a new managed object with a Finalizer is created to ensure the RCW instance is properly cleaned. This is ensured because the CleanUpInteral type has a Finalizer that will survive the current running Finalizer group and permit the cleanup of the RCW instance on the next GC.

class CleanUpInternal
{
    private IntPtr ptr;
    public CleanUpInternal(IntPtr ptr) => this.ptr = ptr;
    ~CleanUpInternal() => Marshal.Release(this.ptr);
}

class RCW
{
    private IntPtr ptr;
    public RCW(IntPtr ptr) => this.ptr = ptr;
    ~RCW()
    {
        var cleanup = new CleanUpInternal(this.ptr);
        GC.KeepAlive(cleanup);
    }
}

Risks

There are minimal risks with this change. The primary motivation here is to improve a deficiency in the ComWrappers API. A workaround is possible but is inefficient and approach difficult is maintain.

@AaronRobinsonMSFT AaronRobinsonMSFT added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr labels Apr 28, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 6.0.0 milestone Apr 28, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 28, 2021
@AaronRobinsonMSFT
Copy link
Member Author

/cc @jkoritzinsky @elinor-fung

@AaronRobinsonMSFT
Copy link
Member Author

This API is not correct. A correct proposed solution is in #52771.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-Interop-coreclr
Projects
None yet
Development

No branches or pull requests

1 participant