Skip to content

Commit

Permalink
Ujjwalchadha/fix value type caching 2 (#980)
Browse files Browse the repository at this point in the history 
* revert incorrect Fix for value type caching (#932)

This reverts commit 8e04090.

* Added conditionalweaktable reference to native value type

* Refactor

* Moved cache to ComWrappersSupport

* Added cache factory for String and type
  • Loading branch information
@ujjwalchadha
ujjwalchadha authored Aug 31, 2021
1 parent dcec6e0 commit c9b8e38
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 96 deletions.
120 changes: 64 additions & 56 deletions src/Tests/UnitTest/TestComponentCSharp_Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
using Windows.Security.Cryptography.Core;
using System.Reflection;
using Windows.Devices.Enumeration.Pnp;


#if NET
using WeakRefNS = System;
#else
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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()
{
Expand Down Expand Up @@ -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;
Expand All @@ -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 { };
Expand Down Expand Up @@ -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();
}
}
}
55 changes: 26 additions & 29 deletions src/WinRT.Runtime/ComWrappersSupport.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
}
}
}
18 changes: 9 additions & 9 deletions src/WinRT.Runtime/ComWrappersSupport.net5.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 1 addition & 2 deletions src/WinRT.Runtime/ComWrappersSupport.netstandard2.0.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
}

Expand Down

0 comments on commit c9b8e38

Please sign in to comment.