From 68cb6a0aee4b974ba69252692a0496ace51e953e Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Fri, 4 May 2018 22:40:06 -0700 Subject: [PATCH 1/4] Add test for implementing internal metadata view interfaces from two assemblies This repros #91 --- .../ExportMetadataTests.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs b/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs index 0a102eda3..b567e470b 100644 --- a/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs +++ b/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs @@ -130,6 +130,17 @@ public void ImportManyWithInternalMetadataInterface(IContainer container) Assert.IsType(importingPart.ImportingProperty.Single().Value); } + /// + /// Verifies that we can skip visibility checks for more than one assembly. + /// + [Trait("Access", "NonPublic")] + [MefFact(CompositionEngines.V3EmulatingV1, typeof(ImportManyPartWithInternalMetadataInterface), typeof(ImportManyPartWithInternalMetadataInterface2), typeof(PartWithExportMetadata))] + public void ThreadSafetyTestForInternalInterfaceProxyGeneration_1Then2(IContainer container) + { + container.GetExportedValue(); + container.GetExportedValue(); + } + [MefFact(CompositionEngines.Unspecified, typeof(ImportingPartWithMetadataInterface), typeof(PartWithExportMetadata))] [Trait("Efficiency", "InstanceReuse")] public void MetadataViewInterfaceInstanceSharedAcrossImports(IContainer container) @@ -645,6 +656,14 @@ public class ImportManyPartWithInternalMetadataInterface internal IEnumerable> ImportingProperty { get; set; } } + [MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)] + [Export] + public class ImportManyPartWithInternalMetadataInterface2 + { + [ImportMany, MefV1.ImportMany] + internal IEnumerable> ImportingProperty { get; set; } + } + [MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)] [Export] public class ImportManyPartWithMetadataClass From 8c3054afa281cd313369d44eccc74039e08af6ab Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Mon, 7 May 2018 10:57:31 -0700 Subject: [PATCH 2/4] Generate each metadata view proxy into its own dynamic assembly This worksaround the CLR limitation that IgnoresAccessChecksToAttribute attributes are only observed once then cached, preventing any later-added attributes from being noticed. Fixes #91 --- .../Configuration/MetadataViewGenerator.cs | 25 ++++--------------- 1 file changed, 5 insertions(+), 20 deletions(-) diff --git a/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs b/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs index 54e90b778..d6860dc38 100644 --- a/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs +++ b/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs @@ -79,7 +79,6 @@ internal static class MetadataViewGenerator private const string MetadataViewFactoryName = "Create"; private static readonly Dictionary MetadataViewFactories = new Dictionary(); - private static readonly AssemblyName ProxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid())); private static readonly Type[] CtorArgumentTypes = new Type[] { typeof(IReadOnlyDictionary), typeof(IReadOnlyDictionary) }; private static readonly MethodInfo MdvDictionaryTryGet = CtorArgumentTypes[0].GetTypeInfo().GetMethod("TryGetValue"); @@ -87,28 +86,12 @@ internal static class MetadataViewGenerator private static readonly MethodInfo ObjectGetType = typeof(object).GetTypeInfo().GetMethod("GetType", Type.EmptyTypes); private static readonly ConstructorInfo ObjectCtor = typeof(object).GetTypeInfo().GetConstructor(Type.EmptyTypes); - private static ModuleBuilder transparentProxyModuleBuilder; - private static SkipClrVisibilityChecks skipClrVisibilityChecks; - public delegate object MetadataViewFactory(IReadOnlyDictionary metadata, IReadOnlyDictionary defaultMetadata); private static AssemblyBuilder CreateProxyAssemblyBuilder(ConstructorInfo constructorInfo) { - return AssemblyBuilder.DefineDynamicAssembly(ProxyAssemblyName, AssemblyBuilderAccess.Run); - } - - private static ModuleBuilder GetProxyModuleBuilder() - { - Assumes.True(Monitor.IsEntered(MetadataViewFactories)); - if (transparentProxyModuleBuilder == null) - { - // make a new assemblybuilder and modulebuilder - var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes)); - transparentProxyModuleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule"); - skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, transparentProxyModuleBuilder); - } - - return transparentProxyModuleBuilder; + var proxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid())); + return AssemblyBuilder.DefineDynamicAssembly(proxyAssemblyName, AssemblyBuilderAccess.Run); } public static MetadataViewFactory GetMetadataViewFactory(Type viewType) @@ -141,7 +124,9 @@ private static TypeInfo GenerateInterfaceViewProxyType(Type viewType) TypeBuilder proxyTypeBuilder; Type[] interfaces = { viewType }; - var proxyModuleBuilder = GetProxyModuleBuilder(); + var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes)); + var proxyModuleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule"); + var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, proxyModuleBuilder); skipClrVisibilityChecks.SkipVisibilityChecksFor(viewType.GetTypeInfo()); proxyTypeBuilder = proxyModuleBuilder.DefineType( From 5e8bc49af65bbe86f6675e8706899959ff454d6b Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Mon, 7 May 2018 22:52:58 -0700 Subject: [PATCH 3/4] Reduce # of dynamic assemblies to just the minimal set required We generate a new dynamic assembly for each unique set of skip visiblity checks now, which is the minimum we can get away with given the CLR limitations. --- .../Configuration/MetadataViewGenerator.cs | 60 +++++++++++++++++-- .../Reflection/SkipClrVisibilityChecks.cs | 53 +++++++++++----- 2 files changed, 94 insertions(+), 19 deletions(-) diff --git a/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs b/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs index d6860dc38..4c4ee1366 100644 --- a/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs +++ b/src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs @@ -10,6 +10,7 @@ namespace Microsoft.VisualStudio.Composition { using System; using System.Collections.Generic; + using System.Collections.Immutable; using System.Globalization; using System.Linq; using System.Reflection; @@ -86,6 +87,8 @@ internal static class MetadataViewGenerator private static readonly MethodInfo ObjectGetType = typeof(object).GetTypeInfo().GetMethod("GetType", Type.EmptyTypes); private static readonly ConstructorInfo ObjectCtor = typeof(object).GetTypeInfo().GetConstructor(Type.EmptyTypes); + private static readonly Dictionary, ModuleBuilder> TransparentProxyModuleBuilderByVisibilityCheck = new Dictionary, ModuleBuilder>(new ByContentEqualityComparer()); + public delegate object MetadataViewFactory(IReadOnlyDictionary metadata, IReadOnlyDictionary defaultMetadata); private static AssemblyBuilder CreateProxyAssemblyBuilder(ConstructorInfo constructorInfo) @@ -94,6 +97,33 @@ private static AssemblyBuilder CreateProxyAssemblyBuilder(ConstructorInfo constr return AssemblyBuilder.DefineDynamicAssembly(proxyAssemblyName, AssemblyBuilderAccess.Run); } + /// + /// Gets the to use for generating a proxy for the given type. + /// + /// The type of the interface to generate a proxy for. + /// The to use. + private static ModuleBuilder GetProxyModuleBuilder(TypeInfo viewType) + { + Requires.NotNull(viewType, nameof(viewType)); + Assumes.True(Monitor.IsEntered(MetadataViewFactories)); + + // Dynamic assemblies are relatively expensive. We want to create as few as possible. + // For each unique set of skip visibility check assemblies, we need a new dynamic assembly + // because the CLR will not honor any additions to that set once the first generated type is closed. + // So maintain a dictionary to point at dynamic modules based on the set of skip visiblity check assemblies they were generated with. + var skipVisibilityCheckAssemblies = SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(viewType); + if (!TransparentProxyModuleBuilderByVisibilityCheck.TryGetValue(skipVisibilityCheckAssemblies, out ModuleBuilder moduleBuilder)) + { + var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes)); + moduleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule"); + var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, moduleBuilder); + skipClrVisibilityChecks.SkipVisibilityChecksFor(skipVisibilityCheckAssemblies); + TransparentProxyModuleBuilderByVisibilityCheck.Add(skipVisibilityCheckAssemblies, moduleBuilder); + } + + return moduleBuilder; + } + public static MetadataViewFactory GetMetadataViewFactory(Type viewType) { Assumes.NotNull(viewType); @@ -124,11 +154,7 @@ private static TypeInfo GenerateInterfaceViewProxyType(Type viewType) TypeBuilder proxyTypeBuilder; Type[] interfaces = { viewType }; - var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes)); - var proxyModuleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule"); - var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, proxyModuleBuilder); - skipClrVisibilityChecks.SkipVisibilityChecksFor(viewType.GetTypeInfo()); - + var proxyModuleBuilder = GetProxyModuleBuilder(viewType.GetTypeInfo()); proxyTypeBuilder = proxyModuleBuilder.DefineType( string.Format(CultureInfo.InvariantCulture, "_proxy_{0}_{1}", viewType.FullName, Guid.NewGuid()), TypeAttributes.Public, @@ -254,5 +280,29 @@ private static IEnumerable GetAllProperties(this Type type) { return type.GetTypeInfo().GetInterfaces().Concat(new Type[] { type }).SelectMany(itf => itf.GetTypeInfo().GetProperties()); } + + private class ByContentEqualityComparer : IEqualityComparer> + { + public bool Equals(ImmutableHashSet x, ImmutableHashSet y) + { + if (x.Count != y.Count) + { + return false; + } + + return !x.Except(y).Any(); + } + + public int GetHashCode(ImmutableHashSet obj) + { + int hashCode = 0; + foreach (var item in obj) + { + hashCode += item.GetHashCode(); + } + + return hashCode; + } + } } } diff --git a/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs b/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs index 10ef3646c..d9bbdf6ca 100644 --- a/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs +++ b/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs @@ -4,6 +4,7 @@ namespace Microsoft.VisualStudio.Composition.Reflection { using System; using System.Collections.Generic; + using System.Collections.Immutable; using System.Linq; using System.Reflection; using System.Reflection.Emit; @@ -64,33 +65,57 @@ internal SkipClrVisibilityChecks(AssemblyBuilder assemblyBuilder, ModuleBuilder } /// - /// Ensures the CLR will skip visibility checks when accessing - /// the assembly that contains the specified member. + /// Gets the set of assemblies that a generated assembly must be granted the ability to skip visiblity checks for + /// in order to access the specified type. /// - /// The member that may not be publicly accessible. - internal void SkipVisibilityChecksFor(MemberInfo memberInfo) + /// The type which may be internal. + /// The set of names of assemblies to skip visibility checks for. + internal static ImmutableHashSet GetSkipVisibilityChecksRequirements(TypeInfo typeInfo) { - this.SkipVisibilityChecksFor(memberInfo.Module.Assembly); + Requires.NotNull(typeInfo, nameof(typeInfo)); + + if (ReflectionHelpers.IsPublic(typeInfo.AsType())) + { + return ImmutableHashSet.Empty; + } + else + { + var result = ImmutableHashSet.Empty + .Add(typeInfo.Assembly.GetName()); + + // If this type has a base type defined in another assembly that is also internal + // (with InternalsVisibleTo to the assembly hosting the original type) + // then we'll need to add that. + if (typeInfo.BaseType != null) + { + result = result.Union(GetSkipVisibilityChecksRequirements(typeInfo.BaseType.GetTypeInfo())); + } + + return result; + } } /// - /// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks - /// for the specified assembly. + /// Add attributes to a dynamic assembly so that the CLR will skip visibility checks + /// for the assemblies with the specified names. /// - /// The assembly to skip visibility checks for. - private void SkipVisibilityChecksFor(Assembly assembly) + /// The names of the assemblies to skip visibility checks for. + internal void SkipVisibilityChecksFor(IEnumerable assemblyNames) { - Requires.NotNull(assembly, nameof(assembly)); - var assemblyName = assembly.GetName(); - this.SkipVisibilityChecksFor(assemblyName); + Requires.NotNull(assemblyNames, nameof(assemblyNames)); + + foreach (var assemblyName in assemblyNames) + { + this.SkipVisibilityChecksFor(assemblyName); + } } /// - /// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks + /// Add an attribute to a dynamic assembly so that the CLR will skip visibility checks /// for the assembly with the specified name. /// /// The name of the assembly to skip visibility checks for. - private void SkipVisibilityChecksFor(AssemblyName assemblyName) + internal void SkipVisibilityChecksFor(AssemblyName assemblyName) { Requires.NotNull(assemblyName, nameof(assemblyName)); From b970746ff9a8937b7a0df35f2f538e57aa2ae1c8 Mon Sep 17 00:00:00 2001 From: Andrew Arnott Date: Mon, 7 May 2018 23:02:13 -0700 Subject: [PATCH 4/4] Update test name and add comment --- .../Reflection/SkipClrVisibilityChecks.cs | 15 +++++++++++---- .../ExportMetadataTests.cs | 2 +- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs b/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs index d9bbdf6ca..ee1c62d41 100644 --- a/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs +++ b/src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs @@ -86,10 +86,17 @@ internal static ImmutableHashSet GetSkipVisibilityChecksRequiremen // If this type has a base type defined in another assembly that is also internal // (with InternalsVisibleTo to the assembly hosting the original type) // then we'll need to add that. - if (typeInfo.BaseType != null) - { - result = result.Union(GetSkipVisibilityChecksRequirements(typeInfo.BaseType.GetTypeInfo())); - } + // TODO: Learn we somehow we don't need to do this, even given our test defines an internal interface + // that derives from another internal interface defined in another assembly. + ////if (typeInfo.BaseType != null) + ////{ + //// result = result.Union(GetSkipVisibilityChecksRequirements(typeInfo.BaseType.GetTypeInfo())); + ////} + + ////foreach (var iface in typeInfo.GetInterfaces()) + ////{ + //// result = result.Union(GetSkipVisibilityChecksRequirements(iface.GetTypeInfo())); + ////} return result; } diff --git a/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs b/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs index b567e470b..eaa8d2c10 100644 --- a/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs +++ b/src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs @@ -135,7 +135,7 @@ public void ImportManyWithInternalMetadataInterface(IContainer container) /// [Trait("Access", "NonPublic")] [MefFact(CompositionEngines.V3EmulatingV1, typeof(ImportManyPartWithInternalMetadataInterface), typeof(ImportManyPartWithInternalMetadataInterface2), typeof(PartWithExportMetadata))] - public void ThreadSafetyTestForInternalInterfaceProxyGeneration_1Then2(IContainer container) + public void InternalMetadataViewInterfacesAcrossMultipleAssemblies(IContainer container) { container.GetExportedValue(); container.GetExportedValue();