ComWrappers API does not provide a mechanism to identify when it is called from old-style APIs

@AaronRobinsonMSFT

In #33485, we wired up the ComWrappers API to be called first-chance from the Marshal APIs and from the IL stub marshalers. This has started to prove a problem for CsWinRT, since there's no way to determine when to use the ComWrappers' implementation and when to use the runtime's built-in APIs.

In particular, the CsWinRT ComWrappers implementation of ComputeVtables has to be able to handle all types, including types that do not implement any WinRT APIs, to correctly implement WinRT semantics for WinUI. Additionally, since WinUI depends on the reference tracking support from ComWrappers, the CsWinRT ComWrappers implementation has to be registered as the global implementation. As a result, if someone such as WPF calls Marshal.GetIUnknownForObject, this call will go down the ComWrappers route first if the Windows SDK is also being used. WPF is expecting to get the built-in runtime-implemented CCWs, but by default CsWinRT would provide one instead.

This proves to be a problem when the user (either WPF or someone else directly using the Marshal APIs or using COM objects in P/Invoke signatures) is trying to pass a managed implementations of COM APIs to native. CsWinRT's ComWrappers only support the WinRT model (as it should be, they shouldn't have to completely re-implement the runtime's CCW/RCW implementation), so they would create a WinRT CCW of the managed object.

We could attempt to use a heuristic in the CsWinRT ComWrappers implementation to determine whether to use the ComWrappers or just return null and fall back, but that heuristic may be tricky or downright impossible to get exactly right. I've included an example below where the heuristic is extremely difficult if not impossible:

Let's take a class class Foo : System.Collections.IEnumerable { ... } and a P/Invoke signature void Bar(System.Collections.IEnumerable ienum). In the WinRT world, we would want to treat this as a Microsoft.UI.Xaml.Interop.IBindableIterable. However, the built-in CCW system would generate an IDispatch implementation that exposes the GetEnumerator method as DISPID_NEWENUM.

Since there is no way to determine from within a ComWrappers implementation if it is being called from the Marshal APIs or a P/Invoke marshaler (without attempting to just look up the call stack), the CsWinRT ComWrappers would never be able to know if it should marshal an instance of Foo itself or if it should let the runtime marshal it.

There is a corresponding problem for the RCW direction, but using a heuristic of "does this object implement IInspectable" works well as a heuristic for ComWrappers.

There are a few possible solutions here:

Provide an additional flag on the CreateObjectFlags and CreateComInterfaceFlags enumeration to denote when the ComWrappers methods are being called from the Marshal APIs or from an IL stub.
Remove the support wiring up a ComWrappers instance as a first-chance handler for the Marshal APIs or from an IL stub.

I'm also open to additional ideas.

cc: @AaronRobinsonMSFT @elinor-fung @jkotas

@jkoritzinsky

@jkoritzinsky My preference is for adding a flag to indicate to the ComWrappers implementer that the call is from the legacy system. However, I am concerned if there are other scenarios where even that information isn't enough to handle this correctly. This leads me to the reluctant position of severing the connection in #33485. I like the integration hence my preference for a new flag, but am leery of other issues that may arise.

Either way, this needs to be reconciled in .NET 5.0.

@elinor-fung @jkotas do either of you have an alternate perspective here?

I think we need to split the registration for the lifetime tracking and the registration as built-in COM interop provider to fix this. I do not think that the flag would be sufficient for that. (We can have the flag too, but there needs to be more.)

I would remove the RegisterAsGlobalInstance method, and instead introduce two new methods:

        // Registers the instance to participate in the GC tracking
        public void RegisterForTrackerSupport();  

        // Registers the instance as first chance provider of built-in interop
        public void RegisterForMarshalling();

Or, we can just severe all connections between the built-in interop and ComWrappers. I would still rename RegisterAsGlobalInstance to RegisterForTrackerSupport if we were do do this. Notice that it does not prevent us from adding RegisterForMarshalling later if we find it useful.

Also, the issue that started this shows that AsyncCausalityTracer implementation depends on the built-in WinRT interop support today. It will need to be redone (or deleted - is it used for anything?) if we were to remove the built-in WinRT support.

I believe it's used only in two places, both related to the manual IAsyncOperation and IAsyncAction to Task<T> and Task projections respectively. We should be able to remove it.

@jkotas

... rename RegisterAsGlobalInstance to RegisterForTrackerSupport if we were do do this. Notice that it does not prevent us from adding RegisterForMarshalling later if we find it useful.

@jkotas Only registering of the tracker scenario was in the original proposal so I am on board with this approach. I also like the idea of having a separate RegisterForMarshaling() call. This does mean that we could have one for tracker support and one for marshaling. Would the expectation be that they could be different or once one is called, only the same global instance can call the other function? (i.e. we only ever have a single global ComWrappers)

The expectations would be that they could be different.

Proposal to update the ComWrappers.RegisterAsGlobalInstance() API:

[Flags]
public enum GlobalInstanceSupport
{
    /// <summary>
    /// Handle global responsibility for Tracker object scenarios.
    /// </summary>
    Tracker,

    /// <summary>
    /// Participate in built-in Marshal API scenarios.
    /// </summary>
    Marshaling,
}

void ComWrappers.RegisterAsGlobalInstance(GlobalInstanceSupport support);

Using flags instead of individual methods is a "choke point API design pattern" that does not naturally work with linking. If somebody wants just one part of the functionality, but not the other part, there is no natural way for the linker to tell that it is fine to strip the other part. It is easy for the linker to tell with distinct methods.

It probably does not matter a ton here - there is not that much specific code for one part vs. the other part. Just pointing it out as it is something to be careful about.

Using flags instead of individual methods is a "choke point API design pattern" that does not naturally work with linking.

Didn't even think about that and is an important consideration - Thank you.

It probably does not matter a ton here - there is not that much specific code for one part vs. the other part. Just pointing it out as it is something to be careful about.

Yep, the paths will be identical. Will see how each approach looks.

Should we do one of those quick API reviews over email for this?

I didn't think it was going to be necessary, but if you think this warrants that I will start a thread.

I think it would not hurt, to follow the rules and nurture our collective taste for API design :-)

Here are some of the things that I am not sure about:

Two APIs vs. enum choice discussed
DO name flag enums with plural nouns or noun phrases
DO name the zero value of flag enums None.
Tracker vs. Tracking (to match modality of marshalling)
Marshaling vs. Marshalling

Navigation Menu

Repository navigation

ComWrappers API does not provide a mechanism to identify when it is called from old-style APIs #35530

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

ComWrappers API does not provide a mechanism to identify when it is called from old-style APIs #35530

Description

Activity

AaronRobinsonMSFT commented on Apr 27, 2020

jkotas commented on Apr 27, 2020

jkotas commented on Apr 27, 2020

jkoritzinsky commented on Apr 27, 2020

AaronRobinsonMSFT commented on Apr 27, 2020

jkotas commented on Apr 27, 2020

AaronRobinsonMSFT commented on Apr 28, 2020

jkotas commented on Apr 28, 2020

AaronRobinsonMSFT commented on Apr 28, 2020

jkotas commented on Apr 29, 2020

AaronRobinsonMSFT commented on Apr 29, 2020

jkotas commented on Apr 29, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions