ComWrappers instances do not provide isolation

[jkotas]

Repro:

using System;
using System.Collections;
using System.Runtime.InteropServices;

class ComWrappers1 : ComWrappers
{
    protected override unsafe ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count)
    {
        throw new NotImplementedException();
    }

    protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags)
    {
        return new ComWrapper1(externalComObject);
    }

    protected override void ReleaseObjects(IEnumerable objects)
    {
        throw new NotImplementedException();
    }
}

class ComWrapper1
{
    IntPtr _punk;

    public ComWrapper1(IntPtr punk)
    {
        _punk = punk;
    }
}

class ComWrappers2 : ComWrappers
{
    protected override unsafe ComInterfaceEntry* ComputeVtables(object obj, CreateComInterfaceFlags flags, out int count)
    {
        throw new NotImplementedException();
    }

    protected override object CreateObject(IntPtr externalComObject, CreateObjectFlags flags)
    {
        return new ComWrapper2(externalComObject);
    }

    protected override void ReleaseObjects(IEnumerable objects)
    {
        throw new NotImplementedException();
    }
}

class ComWrapper2
{
    IntPtr _punk;

    public ComWrapper2(IntPtr punk)
    {
        _punk = punk;
    }
}

class Program
{
    [DllImport("api-ms-win-core-winrt-l1-1-0.dll", PreserveSig = true)]
    internal static extern int RoGetActivationFactory(
          [MarshalAs(UnmanagedType.HString)] string activatableClassId,
          [In] ref Guid iid,
          [Out] out IntPtr factory);

    static Guid IID_IUnknown = new Guid("00000000-0000-0000-c000-000000000046");

    static void Component1()
    {
        var wrappers = new ComWrappers1();

        if (RoGetActivationFactory("Windows.UI.Xaml.Interop.NotifyCollectionChangedEventArgs", ref IID_IUnknown, out IntPtr punk) != 0)
            throw new COMException();

        var wrapper1 = (ComWrapper1)wrappers.GetOrCreateObjectForComInstance(punk, CreateObjectFlags.None);
    }

    static void Component2()
    {
        var wrappers = new ComWrappers2();

        if (RoGetActivationFactory("Windows.UI.Xaml.Interop.NotifyCollectionChangedEventArgs", ref IID_IUnknown, out IntPtr punk) != 0)
            throw new COMException();

        // Returns ComWrapper1!
        var wrapper2 = (ComWrapper2)wrappers.GetOrCreateObjectForComInstance(punk, CreateObjectFlags.None);
    }

    static void Main(string[] args)
    {
        Component1();
        Component2();
    }
}

Expected behavior: Succeeds
Actual behavior: System.InvalidCastException: 'Unable to cast object of type 'ComWrapper1' to type 'ComWrapper2'.

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[jkotas]

This makes it dangerous to use component-local instances of ComWrappers, like we are doing in dotnet/wpf#3092

@AaronRobinsonMSFT @elinor-fung @jkoritzinsky Thoughts?

[jkoritzinsky]

In your example you're never creating an instance of ComWrappers2. Component2 is still creating a ComWrappers1 instance. Is that intentional?

[jkotas]

It is not intentional. Fixed above. The failure is still the same.

[AaronRobinsonMSFT]

This makes it dangerous to use component-local instances of ComWrappers, like we are doing in dotnet/wpf#3092

Absolutely agreed. This example though seems to be a similar argument as permitting IDynamicInterfaceCastable to cast to a class instead of the intent which is to an interface. The two ComWrappers could have had different implementations - which is going to be something to be careful of - but their contract is the same or at least should be. The contract would be shared between the two instances and casting to that would work as expected.

It is also possible to pass the CreateObjectFlags.UniqueInstance which should result in a new object. To that end we could store some metadata on the instance and throw if a different ComWrappers instance is used to create/get an object and/or add a new flag instead to validate this invarant.

[jkotas]

This example though seems to be a similar argument as permitting IDynamicInterfaceCastable to cast to a class instead of the intent which is to an interface.

I am not following. The point of allowing multiple instances of ComWrappers is that two different components (that do not know about each other) can bring their own marshaling logic and not collide. For example, one can map LPCWSTR to string and other can map LPCWSTR to ReadOnlySpan . It means that the two different instances of ComWrappers need to be able to bring their own set of types and not collide.

Can we fix this by making the punk -> wrapper mapping to be per ComWrappers instance, instead of global?

[AaronRobinsonMSFT]

Okay. I think I see how this issue comes up. We will need to handle this.

[AaronRobinsonMSFT]

Anyone have thoughts on whether it makes sense to tie the CCW/RCW to a specific ComWrappers instance or perhaps the most derived type? It seems to me the type might actually be what is interesting here rather than the specific instance. Making this associated with a type however could force users into complex subclassing scenarios instead of creating a configurable class with constructor arguments. Looking for other opinions.

[jkotas]

I think it should be tied to a specific instance.

[elinor-fung]

I'd prefer the instance. From the implementer side, it seems clearer to me to have to use the same ComWrappers to get the non-isolated behaviour than to have to make separate types to make sure the instances are isolated

[jkoritzinsky]

I think tying it to the same instance would be cleaner than requiring extra subclassing just for isolation.

[jkoritzinsky]

We should extend the native weak reference support to also understand the isolation mechanism for when it has to rehydrate the managed wrapper to ensure we rehydrate the object via the same instance that created it.

[AaronRobinsonMSFT]

We should extend the native weak reference support to also understand the isolation mechanism for when it has to rehydrate the managed wrapper to ensure we rehydrate the object via the same instance that created it.

This may present a serious challenge to solve. Initially the mechanism by which we identify a ComWrappers instance was going to be a simple integer. However, if we need to get back at the real object we would need some kind of GC handle - not an issue really but does need to be called out.

This begs the questions of what kind of GC handle? I would prefer a weak GC handle since I don't want the ComWrapper instance to be extended simply to support the possible use of WeakReference. What this means is we will rely on a weak GC handle and if the ComWrappers instance has been removed then the recreation fail.

Alternatively, we create a strong GC handle and ensure the recreation always works. I am a proponent of the weak GC handle approach, but will admit the strong GC handle seems to be easier to validate and is definitely easy to reason about in practice.

[jkoritzinsky]

We should extend the native weak reference support to also understand the isolation mechanism for when it has to rehydrate the managed wrapper to ensure we rehydrate the object via the same instance that created it.

This may present a serious challenge to solve. Initially the mechanism by which we identify a ComWrappers instance was going to be a simple integer. However, if we need to get back at the real object we would need some kind of GC handle - not an issue really but does need to be called out.

Could we use a similar mechanism to how LOADERHANDLEs work in the runtime, where the value is an integer that points into an array that holds the ComWrappers instances or weak references to them?

This begs the questions of what kind of GC handle? I would prefer a weak GC handle since I don't want the ComWrapper instance to be extended simply to support the possible use of WeakReference. What this means is we will rely on a weak GC handle and if the ComWrappers instance has been removed then the recreation fail.

Alternatively, we create a strong GC handle and ensure the recreation always works. I am a proponent of the weak GC handle approach, but will admit the strong GC handle seems to be easier to validate and is definitely easy to reason about in practice.

I'd prefer to use a strong handle since that would ensure we have new behavior match the old behavior, but I can understand not wanting to keep the object alive too long.

[AaronRobinsonMSFT]

Could we use a similar mechanism to how LOADERHANDLEs work in the runtime, where the value is an integer that points into an array that holds the ComWrappers instances or weak references to them?

Not sure that would be needed, but it is an option. Honestly whether it is an int or void* doesn't really matter much.

I'd prefer to use a strong handle since that would ensure we have new behavior match the old behavior, but I can understand not wanting to keep the object alive too long.

I hear you. Definitely worth thinking about. @elinor-fung will be able to provide some insight into what makes the most sense as she works through the design.

[jkotas]

Can we support the weak handles special sauce just for the global instance registered for lifetime tracking? I do not see a good reason for supporting it for the local islands.

[AaronRobinsonMSFT]

@jkotas I think that would be possible since the global is what is being used in that scenario anyways. However, that does make the scenario a bit confusing because then those objects can't ever be used as a WeakReference. But I guess that uniqueness doesn't really matter anymore even if we need to fallback to the global to try and create a new wrapper since the identity issue would be moot since there would be no other object to check identity against.

[AaronRobinsonMSFT]

fallback to the global to try and create a new wrapper

Actually strike that. We can't do that since it would create an escape hatch out of the embedded scenario. We need to be able to track this and not call the global fallback if the wrapper inserted was collected and not created by the global.

[elinor-fung]

Another possibility could be to have a map of ID -> WeakReference<ComWrappers> in managed (similar to what we have for ALCs)?

Options:

• Strong GC handle to the ComWrappers instance
  • Rehydration should always work for all instances
  • Extends object lifetime just for WeakReference rehydration support
• Weak GC handle to the ComWrappers instance
  • Error on rehydration if associated instance was collected
• Only allow rehydration with global instance for tracker support
  • Error on rehydration for any other associated instance
  • Either handle or ID (int) could identify the global instance for tracker support
• ID identifying the ComWrappers instance with a mapping in managed
  • Error on rehydration if associated instance was collected
  • Map would only be for WeakReference rehydration support

[jkotas]

The scenario for the non-global ComWrappers is using few WinRT APIs as internal implementation in a library. Do you see a reason why a scenario like this would need the special weak reference handling?

I look at the special weak reference handling as part of "make WinRT Xaml work well" bundle of features. WinRT Xaml will work with the globally registered ComWrappers only, so it should be fine to limit all features in this feature bundle to the globally registered ComWrappers as well.

[AaronRobinsonMSFT]

Do you see a reason why a scenario like this would need the special weak reference handling?

I don't see that as obvious or controllable by us. Users see a managed object, the fact that it is backed by WinRT may not be interesting they just want a WeakReference for some reason. I don't see this support as causing too much additional risk or bloat. Am I missing some aspect of adding this support for WinRT in any scenario?

[jkotas]

Users can still get a weak reference to an object backed by WinRT object without the special weak reference support. They are just not going to get the automatic collection of cycles over the managed/COM boundary.

I think it would help to find a real-world looking piece of code that:

• Calls (existing) WinRT API
• It is likely to be used with non-global ComWrappers (e.g. it does not use Xaml)
• The special weak reference support makes a difference for it

If you find an example like this, it would help me see the value of doing this work.

I don't see this support as causing too much additional risk

The weak references and life time management in general tend to be complex, with a lot of interesting corner cases that nobody can think about upfront. I would expect this to have tail of stress bugs.