[API Proposal]: Allow the user to provide state for a ComWrappers.GetOrCreateObjectForComInstance operation

[jkoritzinsky]

Background and motivation

As part of the Interop Type Mapping Proposal, one important aspect we've discussed for foreign language projections to improve their performance (and avoid relying on the type map in the majority of cases), is to use call-site information about types to avoid needing to go through the map for strongly-typed APIs.
 
Currently, there is no mechanism to pass down additional state to ComWrappers.CreateObject, so any foreign language projection based on COM (ie. CsWinRT) must use something else such as [ThreadStatic] statics on their ComWrappers implementation to pass down call-site state as a side channel.

Additionally, CsWinRT has had to add additional tracking and lifetime extensions to support some of their primitives that should be immediately unwrapped, ie. IReference<T> implementations, as mentioned in #113591.

In #113591, an API change is proposed to allow CsWinRT to use another side-channel to return an object without going through ComWrappers. In the comments, a protected property is proposed to provide operation-scoped state.

This API proposal aims to solve both of the above problems. It allows the user to provide their own "state" object that will be available in CreateObjectWithState and allows the user to specify flags based on the created wrapper object with the wrapperFlags out parameter.

API Proposal

namespace System.Runtime.InteropServices;

+public enum CreatedWrapperFlags
+{
+    None = 0,
+    // Same as CreateObjectFlags.TrackerObject, but decided based on inspecting the COM object directly while creating the wrapper.
+    TrackerObject = 0x1,
+    /// <summary>
+    /// The managed object doesn't keep the native object alive. It represents an equivalent value.
+    /// </summary>
+    /// <remarks>
+    /// Using this flag results in the following changes:
+    /// <see cref="ComWrappers.TryGetComInstance" /> will return <c>false</c> for the returned object.
+    /// The features provided by the <see cref="CreateObjectFlags.TrackerObject" /> flag will be disabled.
+    /// Integration between <see cref="System.WeakReference" /> and the returned object via the native <c>IWeakReferenceSource</c> interface will not work.
+    /// <see cref="CreateObjectFlags.UniqueInstance" /> behavior is implied.
+    /// Diagnostics tooling support to unwrap objects returned by `CreateObject` will not see this object as a wrapper.
+    /// The same object can be returned from `CreateObject` wrapping different COM objects.
+    /// </remarks>
+    NonWrapping = 0x2
+}

public abstract class ComWrappers
{
   protected abstract object? CreateObject(IntPtr externalComObject, CreateObjectFlags flags);
+    protected virtual object? CreateObject(IntPtr externalComObject, CreateObjectFlags flags, object? userState, out CreatedWrapperFlags wrapperFlags)
+    {
+        wrapperFlags = CreatedWrapperFlags.None;
+        return CreateObject(externalComObject, flags);
+    }

   public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateObjectFlags flags);
+    public object GetOrCreateObjectForComInstance(IntPtr externalComObject, CreateObjectFlags flags, object? userState);
}

API Usage

class WinRTComWrappers  : ComWrappers
{
    protected override object? CreateObject(IntPtr externalComObject, ref CreateObjectFlags flags, object? userState, out CreatedWrapperFlags wrapperFlags)
    {
        if (CheckIfShouldCreateAnRcw(...))
        {
            wrapperFlags = CreatedWrapperFlags.None;
            return rcw;
        }

        // Otherwise, unbox directly
        wrapperFlags = CreatedWrapperFlags.NonWrapping;
        return UnboxTheValue(...);
    }
}

Alternative Designs

The designs in #113581 and #113591 are alternative options we've considered.

Risks

These API changes would make it less obvious which members on ComWrappers to implement in subclasses, as there are now two overloads of CreateObject.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

[AaronRobinsonMSFT]

Currently, there is no mechanism to pass down additional state to ComWrappers.CreateObject, so any foreign language projection based on COM (ie. CsWinRT) must use something else such as [ThreadStatic] statics on their ComWrappers implementation to pass down call-site state as a side channel.

What is an example of state that is needed? Are their specific downside risks or performance issues with the ThreadStatic approach?

[AaronRobinsonMSFT]

+    /// <summary>
+    /// The managed object doesn't keep the native object alive. It represents an equivalent value.
+    /// </summary>
+    NonWrapping = 0x10

I'm not sure what this does in practice. Can you please update the summary to describe what users can expect from ComWrappers? I assume that this means we won't be placing it in the cache. If so, then I think it might be better to state that, SkipCache?, rather than describe what the CreateObjectWithState() implementation is doing.

protected virtual

This change means users will have a very different implementing experience. At present, since it is abstract, the editor will provide implementations and dictate what needs to be implemented. Changing this model means it won't be obvious which APIs to implement or even that one of the CreateObject versions needs to be implemented. Perhaps this is okay, but does create another annoying barrier for an already complex interop type.

[AaronRobinsonMSFT]

This change means users will have a very different implementing experience. At present, since it is abstract, the editor will provide implementations and dictate what needs to be implemented.

I think in this case I would prefer if we didn't change the signature for CreateObject. I'd like to keep the current editor behavior as-is. This will avoid any confusion about what is expected to be written by the user and if they need the state the documentation will clearly define they should override CreateObjectWithState() in their implementation.

[AaronRobinsonMSFT]

NonWrapping = 0x10

We should also error out if this is an input value to any ComWrappers call. It is only an out for the state scenario. Open to hearing there are cases where this isn't true.

[jkoritzinsky]

I've updated the proposal above with the list of behavior changes this API would cause. I'll include them here as well:

• ComWrappers.TryGetComInstance will return false for the returned object.
• The features provided by the TrackerObject flag will be disabled.
• WeakReference interop won't work for the returned object.
• UniqueInstance behavior is implied.
• Diagnostics tooling support to unwrap objects returned by CreateObject will not see this object as a wrapper.
• The same object can be returned from CreateObject wrapping different COM objects

This is primarily to ensure that the following scenarios don't occur:

• ComWrappers.TryGetComInstance gives back a pointer to an already released (and possibly re-allocated) COM object.
• Unwrapping a COM object to either a string or System.Type that's cached in the runtime doesn't block other values from being unwrapped to those instances.

I'm fine with CreateObject continuing to be abstract and adding a new virtual instead, so I've updated the proposal.

[AaronRobinsonMSFT]

@jkoritzinsky I'm assuming the following relationship between the APIs:

GetOrCreateObjectForComInstance - current

• Will always call CreateObject

GetOrCreateObjectForComInstance - with state argument overload

• Will always call CreateObjectWithState
• The default implementation will call CreateObject, ignoring the state argument

GetOrRegisterObjectForComInstance - no changes

• Will always call CreateObject
• Doesn't call any create function.

[jkoritzinsky]

GetOrRegisterObjectForComInstance never calls CreateObject today, but other than that yes, I would agree with that spec.

[AaronRobinsonMSFT]

GetOrRegisterObjectForComInstance never calls CreateObject today, but other than that yes, I would agree with that spec.

Right. It shouldn't call any create functions.

Then the last thing, NonWrapping is never valid for input or do you think it has utility?

[jkotas]

CreateObjectWithState

It can be CreateObject. The state is implied by the signature. The proposal does not have WithState suffix for GetOrCreateObjectForComInstance either - the two should be symmetric.

Nit: The proposal should include existing CreateObject and GetOrCreateObjectForComInstance methods to make it easier to see what's being added.

[jkoritzinsky]

GetOrRegisterObjectForComInstance never calls CreateObject today, but other than that yes, I would agree with that spec.

Right. It shouldn't call any create functions.

Then the last thing, NonWrapping is never valid for input or do you think it has utility?

I think if the user knows the COM object that they're passing in is something they'd want to set NonWrapping for, then we should allow them to be explicit and avoid interrogation QI's in their ComWrappers implementation.

[Sergio0694]

Was re-reading this, question: if it's true that

"GetOrCreateObjectForComInstance - current

Will always call CreateObject"

Does this mean that you can only ever mutate the create object flags if you're also passed an argument? That is, if someone calls GetOrCreateObjectForComInstance without passing a state (either a caller in user code, or the tracker manager), then not only you don't get the state (expected), but you also are not able to mutate the flags within CreateObject for that call.

Wondering if it might be a bit weird and/or unexpected that the ability to mutate flags is tied to having an additional state being passed in, regardless of whether that CreateObject implementation even cares about having a state at all. Is this intended?

[AaronRobinsonMSFT]

Does this mean that you can only ever mutate the create object flags if you're also passed an argument?

Yes.

Is this intended?

Yes. The existing CreateObject can't be changed and if any other users need this functionality they can use the other API.

[AaronRobinsonMSFT]

I think if the user knows the COM object that they're passing in is something they'd want to set NonWrapping for, then we should allow them to be explicit and avoid interrogation QI's in their ComWrappers implementation.

We should be careful with this. The issue becomes we create situations where prior to calling CreateObject we respect them, but then we also support them post CreateObject call - they shouldn't have different meaning pre/post CreateObject call. I personally think we should block this unless we have a customer that needs it. It will avoid us creating the very issue I am referring to and it will allow us to understand the user need. Additionally, users can already handle that case because they have the state object so it isn't like they are blocked from handling the scenario in question.

[Sergio0694]

"Yes. The existing CreateObject can't be changed and if any other users need this functionality they can use the other API."

I guess I was more concerned about implicit calls from the tracker manager, since you can't control those, and those would pass no state. Meaning that you'd also not be able to tweak flags in that scenario. Perhaps it would make sense to update the tracker manager to invoke the new overload instead? That way people wanting to use the new overload would be able to always do so (either manually, or the runtime would do it from the tracker manager), and people only using the old one would also still work (as the default implementation of the new one would just call the old one and not mutate the flags). Would that make sense? 🤔

[AaronRobinsonMSFT]

Meaning that you'd also not be able to tweak flags in that scenario. Perhaps it would make sense to update the tracker manager to invoke the new overload instead?

The concern then would be properly documenting when state object would be passed. Ostensibly people say to themselves, I want to always call the API with the state and then they do something odd or naive like implementing the stateful to always expect the state and/or never forward to the non-state version or make it throw. The issue becomes we start changing behavior that has negative downstream impact for no win and it simply makes us write complex documentation that is painful to be accurate about and even more so to test.

The alternative is to leave it as-is. That has the benefit of keeping the established and predictable behavior and if we see users have a need, not desire but actual blocking need, then we can make an informed decision based on data rather than peoples wants and desires. This is part of leaving the other CreateObject as abstract. This entire feature is for a niche case that already has a solution and I think it should be targeted on this path until we understand the needs based on data.

[Sergio0694]

Makes sense to me, thank you for the additional context! 🙂

[jkoritzinsky]

Aaron and I spoke offline about the behavior of tweaking flags. Instead of tweaking flags in-flight and requiring ComWrappers to validate that only valid flags are passed to GetOrCreateObjectFromComInstance and only valid changes are made in CreateObject, we're going to change the signature to have a second enum type for the flags that can be edited.

I'll update the proposal with the changes.

[Sergio0694]

@jkoritzinsky I like the new idea of output-only flags, would it also be possible to have one to disable TrackerObject? In CsWinRT, when marshalling objects we have to enable that by default, as chances are the object will be a tracker object. However, when we're inspecting it to produce a wrapper, we also have to QI for the reference tracker (as we need to track it internally). It would be nice for us to pass that info back to ComWrappers if we see that the object is not a tracker object, so that ComWrappers can avoid checking for it (and doing a QI that we already know would fail anyway).

Something like this perhaps:

public enum CreatedWrapperFlags
{
    NonTrackerObject = 0x1,
    NonWrapping = 0x2
}

Basically it'd be a similar optimization to #113632.

Thoughts?

[AaronRobinsonMSFT]

However, when we're inspecting it to produce a wrapper, we also have to QI for the reference tracker (as we need to track it internally).

How do we know that was done properly? I don't think it is appropriate for these optimization to occur as it influences our own checks for correctness.

[Sergio0694]

"How do we know that was done properly?"

My rationale was that it'd be the responsibility of the interop stack to ensure it's correct, in the same way as if we weren't passing TrackerObject in the first place, that would also result in the same kind of memory leak when dealing with XAML objects anyway.

[AaronRobinsonMSFT]

This isn't about memory leaking. It is about the runtime doing the work it needs to do to provide guarantees. The TrackerObject flag could have been entirely removed by asking the caller to pass in the IReferenceTracker itself, but we didn't because we own the operation. Optimizing QIs within the runtime itself seems fine as long as it isn't too cumbersome to pass around state. This is relying on the CreateObject, an external user supplied API, to make a determination that we have the ability to correctly and safely do within the runtime.

[Sergio0694]

I see, that's fair. Once again I appreciate you taking the time to share the additional context 🙂

[AaronRobinsonMSFT]

This is relying on the CreateObject, an external user supplied API, to make a determination that we have the ability to correctly and safely do within the runtime.

I've thought more about this. I'm going to defer to @jkoritzinsky on this one. Given the number of teams (assuming 1) that will be using this API to add support for the TrackerObject flag and the fact that I'm not fully educated on the exact implementation details in native AOT it is possible the radius of impact is less than I'm imagining.

Up to you @jkoritzinsky.

[jkoritzinsky]

I think I'd prefer going in the opposite direction: Adding a TrackerObject flag to CreatedWrapperFlags. Basically, allow either the caller of GetOrCreateObjectForComInstance to decide "hey you should check for tracker support" or let the implementor of CreateObject say "hey this object needs tracker support even if the user didn't request it.

Then ComWrappers would check for tracker support if either TrackerObject flag is set.

That way, we don't have a "yes do that. No wait actually no don't do that" API design. CsWinRT could move their calls that will pass state to say "don't use tracker support by default" and allow the implementation to decide if its needed. The places where the runtime calls into ComWrappers would continue to ask for TrackerObject unconditionally.

[Sergio0694]

"That way, we don't have a "yes do that. No wait actually no don't do that" API design."

Mmh not entirely sure I follow, isn't it the same anyway just the opposite way? "don't need tracker" "oh actually yes". The idea with NonTrackerObject was so that CreateObject could tell ComWrappers "oh by the way I already checked for reference tracker, this object doesn't need it, don't worry about it". In the same way that CreateObjectFlags.TrackerObject doesn't mean "always use ref tracker" but it means "check and use it if the object implements it". It'd just skip that secondary check if we're already doing that within CsWinRT anyway.

Basically it doesn't seem unlike how NonWrapperObject also means "register a wrapper" "no actually don't do that".

EDIT: I re-read your message, to clarify, I see what you mean now, we could use either approach in CsWinRT, I guess with the one currently in the proposal we'd just update our callsites to never pass TrackerObject then, and we'd only ever set it from our own CreateObject, so the end result should effectively still be the same. I suppose I was just trying to follow what you meant, but yeah in practice either should work for us, so if you feel strongly about going that way, it should also be fine for us 🙂

EDIT 2: thinking about this some more, the only possible concern I have with setting ref tracker later is:

runtime/src/coreclr/nativeaot/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.NativeAot.cs

Lines 908 to 911 in 5e7d681

	bool refTrackerInnerScenario = flags.HasFlag(CreateObjectFlags.TrackerObject)
	&& flags.HasFlag(CreateObjectFlags.Aggregation);
	if (refTrackerInnerScenario &&
	Marshal.QueryInterface(externalComObject, IID_IReferenceTracker, out IntPtr referenceTrackerPtr) == HResults.S_OK)

How would this work if at this point TrackerObject isn't set yet, but it's only set later from CreateObject?

[AaronRobinsonMSFT]

EDIT 2: thinking about this some more, the only possible concern I have with setting ref tracker later is:

This is the sort of implementation detail and complexity I was alluding to in #113622 (comment). I genuinely don't like the NonTrackerObject because it attempts to ask us to revert work - that is hard. The other direction, "no wait, do this too" is a bit easier conceptually, but the native AOT implementation appears to be what we will eventually have so I'm deferring to @jkoritzinsky to make it all work and be a relatively straightforward change with robust testing.

[jkoritzinsky]

I looked at that after proposing the above.

That check there is only for the .NET 5 backcompat scenario for CsWinRT 1.x. If you always specify an inner in the aggregation scenario, then there would be no observable change in behavior.

[Sergio0694]

Ooh I see. Correct me if I'm wrong (just making sure I'm fully following here):

• CsWinRT would always be passing the inner
• That first check would always be skipped, since refTrackerInnerScenario wouldn't be true (TrackerObject isn't passed)
• We take the second branch and just do a normal QI for IUnknown on the external object
• Our CreateObject is invoked
• We set CreatedWrapperFlags.TrackerObject if we see the interface
• ComWrappers resumes and just does its own QI for IReferenceTracker (since it wouldn't have it already now)

So we keep just 1 QI for IReferenceTracker in ComWrappers (after #113632), but we can now also skip it entirely (and therefore skip creating a tracker managed object wrapper) if we don't set CreatedWrapperFlags.TrackerObject on our end.

Seems great! Thank you for looking into this and updating the proposal as well 🙂

---

"That check there is only for the .NET 5 backcompat scenario for CsWinRT 1.x."

Not entirely sure I understand why that check is in the NAOT implementation though. CsWinRT 1.x doesn't support AOT at all. Using CsWinRT on NAOT requires at least CsWinRT 2.1, which always passes the inner. Perhaps a leftover from the port?

[jkoritzinsky]

It's a .NET 5 backcompat scenario, but because we support the same experience on CoreCLR and NativeAOT, we have the same code even though CsWinRT doesn't use it (as theoretically someone could depend on it and we don't have a good justification to explicitly make the behavior differ.)