Ujjwalchadha/fix value type caching 2

src/Tests/UnitTest/TestComponentCSharp_Tests.cs

@@ -29,7 +29,7 @@
 using Windows.Security.Cryptography.Core;
 using System.Reflection;
 using Windows.Devices.Enumeration.Pnp;
-
+
 #if NET
 using WeakRefNS = System;
 #else
@@ -50,11 +50,11 @@ public class TestCSharp
         public TestCSharp()
         {
             TestObject = new Class();
-        }
-
+        }
+
 
         // Test a fix for a bug in Mono.Cecil that was affecting the IIDOptimizer when it encountered long class names 
-        [Fact]
+        [Fact]
         public void TestLongClassNameEventSource()
         {
             bool flag = false;
@@ -744,10 +744,10 @@ public void TestCustomProjections()
             Assert.Equal("name", propertyName);
 
             bool eventCalled = false;
-            TestObject.CanExecuteChanged += (object sender, EventArgs e) =>
-            {
-                eventCalled = true;
-            };
+            TestObject.CanExecuteChanged += (object sender, EventArgs e) =>
+            {
+                eventCalled = true;
+            };
 
             TestObject.RaiseCanExecuteChanged();
             Assert.True(eventCalled);
@@ -761,8 +761,8 @@ public void TestCustomProjections()
             // Ensure robustness with bad runtime class names (parsing errors, type not found, etc)
             var badRuntimeClassName = Class.BadRuntimeClassName;
             Assert.NotNull(badRuntimeClassName);
-        }
-
+        }
+
         [Fact]
         public void TestKeyValuePair()
         {
@@ -2566,8 +2566,8 @@ public void TestEventSourceCaching()
             GC.WaitForPendingFinalizers();
             Assert.False(weakClassInstance.TryGetTarget(out _));
             Assert.False(weakEventHandlerClass.TryGetTarget(out _));
-        }
-
+        }
+
         class EventHandlerClass
         {
             private readonly Action eventCalled;
@@ -2580,6 +2580,58 @@ public EventHandlerClass(Action eventCalled)
             public void IntPropertyChanged(object sender, int e) => eventCalled();
         }
 
+        [Fact]
+        private async Task TestPnpPropertiesInLoop()
+        {
+            for (int i = 0; i < 10; i++)
+            {
+                await TestPnpPropertiesAsync();
+            }
+        }
+
+        private async Task TestPnpPropertiesAsync()
+        {
+            var requestedDeviceProperties = new List<string>()
+                {
+                    "System.Devices.ClassGuid",
+                    "System.Devices.ContainerId",
+                    "System.Devices.DeviceHasProblem",
+                    "System.Devices.DeviceInstanceId",
+                    "System.Devices.Parent",
+                    "System.Devices.Present",
+                    "System.ItemNameDisplay",
+                    "System.Devices.Children",
+                };
+            var devicefilter = "System.Devices.Present:System.StructuredQueryType.Boolean#True";
+            var presentDevices = (await PnpObject.FindAllAsync(PnpObjectType.Device, requestedDeviceProperties, devicefilter).AsTask().ConfigureAwait(false)).Select(pnpObject => {
+                var prop = pnpObject.Properties;
+                // Iterating through each key is necessary for this test even though we do not use each key directly
+                // This makes it more probable for a native pointer to get repeated and a value type to be cached and seen again.
+                foreach (var key in prop.Keys)
+                {
+                    var val = prop[key];
+                    if (key == "System.Devices.ContainerId" && val != null)
+                    {
+                        var val4 = pnpObject.Properties[key];
+                        if (val is not Guid || val4 is not Guid)
+                        {
+                            throw new Exception("Incorrect value type Guid. Actual type: " + val.GetType() + "  " + val4.GetType());
+                        }
+                    }
+                    if (key == "System.Devices.Parent" && val != null)
+                    {
+                        var val4 = pnpObject.Properties[key];
+                        if (val is not string || val4 is not string)
+                        {
+                            throw new Exception("Incorrect value type string Actual type: " + val.GetType() + "  " + val4.GetType());
+                        }
+                    }
+
+                }
+                return pnpObject;
+            }).ToList();
+        }
+
 #if NET
         [TestComponentCSharp.Warning]  // NO warning CA1416
         class WarningManaged { };
@@ -2628,49 +2680,5 @@ private void TestSupportedOSPlatformWarnings()
             WarningStatic.WarningEvent += (object s, Int32 v) => { }; // warning CA1416
         }
 #endif
-
-        [Fact]
-        private async Task TestPnpPropertiesAsync()
-        {
-            var requestedDeviceProperties = new List<string>()
-                {
-                    "System.Devices.ClassGuid",
-                    "System.Devices.ContainerId",
-                    "System.Devices.DeviceHasProblem",
-                    "System.Devices.DeviceInstanceId",
-                    "System.Devices.Parent",
-                    "System.Devices.Present",
-                    "System.ItemNameDisplay",
-                    "System.Devices.Children",
-                };
-            var devicefilter = "System.Devices.Present:System.StructuredQueryType.Boolean#True";
-            var presentDevices = (await PnpObject.FindAllAsync(PnpObjectType.Device, requestedDeviceProperties, devicefilter).AsTask().ConfigureAwait(false)).Select(pnpObject => {
-                var prop = pnpObject.Properties;
-                // Iterating through each key is necessary for this test even though we do not use each key directly
-                // This makes it more probable for a native pointer to get repeated and a value type to be cached and seen again.
-                foreach (var key in prop.Keys)
-                {
-                    var val = prop[key];
-                    if (key == "System.Devices.ContainerId" && val != null)
-                    {
-                        var val4 = pnpObject.Properties[key];
-                        if (val is not Guid || val4 is not Guid)
-                        {
-                            throw new Exception("Incorrect value type Guid");
-                        }
-                    }
-                    if (key == "System.Devices.Parent" && val != null)
-                    {
-                        var val4 = pnpObject.Properties[key];
-                        if (val is not string || val4 is not string)
-                        {
-                            throw new Exception("Incorrect value type string");
-                        }
-                    }
-
-                }
-                return pnpObject;
-            }).ToList();
-        }
     }
 }

src/WinRT.Runtime/ComWrappersSupport.cs

@@ -280,11 +280,10 @@ private static Func<IInspectable, object> CreateNullableTFactory(Type implementa
             ParameterExpression[] parms = new[] { Expression.Parameter(typeof(IInspectable), "inspectable") };
             var createInterfaceInstanceExpression = Expression.New(helperType.GetConstructor(new[] { typeof(ObjectReference<>).MakeGenericType(vftblType) }),
                     Expression.Call(parms[0],
-                        typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));
-
-            var getValueExpression = Expression.Convert(Expression.Property(createInterfaceInstanceExpression, "Value"), typeof(object));
-            var setValueInWrapperexpression = Expression.New(typeof(ValueTypeWrapper).GetConstructor(new[] { typeof(object) }), getValueExpression);
-            return Expression.Lambda<Func<IInspectable, object>>(setValueInWrapperexpression, parms).Compile();
+                        typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));
+
+            return Expression.Lambda<Func<IInspectable, object>>(
+                Expression.Convert(Expression.Property(createInterfaceInstanceExpression, "Value"), typeof(object)), parms).Compile();
         }
 
         private static Func<IInspectable, object> CreateArrayFactory(Type implementationType)
@@ -297,9 +296,22 @@ private static Func<IInspectable, object> CreateArrayFactory(Type implementation
                     Expression.Call(parms[0],
                         typeof(IInspectable).GetMethod(nameof(IInspectable.As)).MakeGenericMethod(vftblType)));
 
-            var getValueExpression = Expression.Property(createInterfaceInstanceExpression, "Value");
-            var setValueInWrapperexpression = Expression.New(typeof(ValueTypeWrapper).GetConstructor(new[] { typeof(object) }), getValueExpression);
-            return Expression.Lambda<Func<IInspectable, object>>(setValueInWrapperexpression, parms).Compile();
+            return Expression.Lambda<Func<IInspectable, object>>(
+                Expression.Property(createInterfaceInstanceExpression, "Value"), parms).Compile();
+        }
+
+        // This is used to hold the reference to the native value type object (IReference) until the actual value in it (boxed as an object) gets cleaned up by GC
+        // This is done to avoid pointer reuse until GC cleans up the boxed object
+        private static ConditionalWeakTable<object, IInspectable> _boxedValueReferenceCache = new();
+
+        private static Func<IInspectable, object> CreateReferenceCachingFactory(Func<IInspectable, object> internalFactory)
+        {
+            return inspectable =>
+            {
+                object resultingObject = internalFactory(inspectable);
+                _boxedValueReferenceCache.Add(resultingObject, inspectable);
+                return resultingObject;
+            };
         }
 
         internal static Func<IInspectable, object> CreateTypedRcwFactory(string runtimeClassName)
@@ -312,11 +324,11 @@ internal static Func<IInspectable, object> CreateTypedRcwFactory(string runtimeC
             // PropertySet and ValueSet can return IReference<String> but Nullable<String> is illegal
             if (runtimeClassName == "Windows.Foundation.IReference`1<String>")
             {
-                return (IInspectable obj) => new ABI.System.Nullable<String>(obj.ObjRef);
+                return CreateReferenceCachingFactory((IInspectable obj) => new ABI.System.Nullable<String>(obj.ObjRef));
             }
             else if (runtimeClassName == "Windows.Foundation.IReference`1<Windows.UI.Xaml.Interop.TypeName>")
             {
-                return (IInspectable obj) => new ABI.System.Nullable<Type>(obj.ObjRef);
+                return CreateReferenceCachingFactory((IInspectable obj) => new ABI.System.Nullable<Type>(obj.ObjRef));
             }
 
             Type implementationType = TypeNameSupport.FindTypeByNameCached(runtimeClassName);
@@ -329,23 +341,23 @@ internal static Func<IInspectable, object> CreateTypedRcwFactory(string runtimeC
 
             if (implementationType.IsGenericType && implementationType.GetGenericTypeDefinition() == typeof(System.Collections.Generic.KeyValuePair<,>))
             {
-                return CreateKeyValuePairFactory(implementationType);
+                return CreateReferenceCachingFactory(CreateKeyValuePairFactory(implementationType));
             }
 
             if (implementationType.IsValueType)
             {
                 if (IsNullableT(implementationType))
                 {
-                    return CreateNullableTFactory(implementationType);
+                    return CreateReferenceCachingFactory(CreateNullableTFactory(implementationType));
                 }
                 else
                 {
-                    return CreateNullableTFactory(typeof(System.Nullable<>).MakeGenericType(implementationType));
+                    return CreateReferenceCachingFactory(CreateNullableTFactory(typeof(System.Nullable<>).MakeGenericType(implementationType)));
                 }
             }
             else if (IsIReferenceArray(implementationType))
             {
-                return CreateArrayFactory(implementationType);
+                return CreateReferenceCachingFactory(CreateArrayFactory(implementationType));
             }
 
             return CreateFactoryForImplementationType(runtimeClassName, implementationType);
@@ -728,20 +740,5 @@ internal InspectableInfo(Type type, Guid[] iids)
             }
 
         }
-
-        internal class ValueTypeWrapper
-        {
-            private readonly object value;
-
-            public ValueTypeWrapper(object value)
-            {
-                this.value = value;
-            }
-
-            public object Value
-            {
-                get => this.value;
-            }
-        }
     }
 }

src/WinRT.Runtime/ComWrappersSupport.net5.cs

@@ -92,25 +92,24 @@ private static T CreateRcwForComObject<T>(IntPtr ptr, bool tryUseCache)
             var flags = tryUseCache ? CreateObjectFlags.TrackerObject : CreateObjectFlags.TrackerObject | CreateObjectFlags.UniqueInstance;
             var rcw = ComWrappers.GetOrCreateObjectForComInstance(ptr, flags);
             CreateRCWType.Value = null;
-
-            // Resurrect IWinRTObject's disposed IObjectReferences, if necessary
-            if (rcw is IWinRTObject winrtObj)
-            {
-                winrtObj.Resurrect();
-            }
-
             // Because .NET will de-duplicate strings and WinRT doesn't,
             // our RCW factory returns a wrapper of our string instance.
             // This ensures that ComWrappers never sees the same managed object for two different
             // native pointers. We unwrap here to ensure that the user-experience is expected
             // and consumers get a string object for a Windows.Foundation.IReference<String>.
             // We need to do the same thing for System.Type because there can be multiple WUX.Interop.TypeName's
             // for a single System.Type.
+
+            // Resurrect IWinRTObject's disposed IObjectReferences, if necessary
+            if (rcw is IWinRTObject winrtObj)
+            {
+                winrtObj.Resurrect();
+            }
+
             return rcw switch
             {
-                ABI.System.Nullable<string> ns => (T)(object)ns.Value,
+                ABI.System.Nullable<string> ns => (T)(object)ns.Value,
                 ABI.System.Nullable<Type> nt => (T)(object)nt.Value,
-                ValueTypeWrapper vt => (T) vt.Value,
                 T castRcw => castRcw,
                 _ when tryUseCache => CreateRcwForComObject<T>(ptr, false),
                 _ => throw new ArgumentException(string.Format("Unable to create a wrapper object. The WinRT object {0} has type {1} which cannot be assigned to type {2}", ptr, rcw.GetType(), typeof(T)))
@@ -446,6 +445,7 @@ private static object CreateObject(IObjectReference objRef)
                     // we use the Inspectable wrapper directly.
                     return inspectable;
                 }
+
                 return ComWrappersSupport.GetTypedRcwFactory(runtimeClassName)(inspectable);
             }
             else if (objRef.TryAs<ABI.WinRT.Interop.IWeakReference.Vftbl>(out var weakRef) == 0)

src/WinRT.Runtime/ComWrappersSupport.netstandard2.0.cs

@@ -74,8 +74,7 @@ public static T CreateRcwForComObject<T>(IntPtr ptr)
             {
                 ABI.System.Nullable<string> ns => (T)(object)ns.Value,
                 ABI.System.Nullable<Type> nt => (T)(object)nt.Value,
-                ValueTypeWrapper vt => (T)vt.Value,
-                _ => (T)rcw
+                _ => (T)rcw
             };
         }