Porting StandardOleMarshalObject and IMarshal #30931

Anipik · 2018-07-09T23:51:39Z

Fixes https://github.com/dotnet/corefx/issues/29956

stephentoub · 2018-07-10T01:56:10Z

src/Common/src/Microsoft/Win32/IMarshal.cs

@@ -0,0 +1,33 @@
+// Licensed to the .NET Foundation under one or more agreements.


Why is this being added to src/Common/src? Is it going to be consumed into projects other than System.Runtime.InteropServices? Seems like it should either a) be in System.Runtime.InteropServices's directory, or b) be in src/Common/src/Interop/... somewhere... and I don't believe there's any reason it needs to remain in the Microsoft.Win32 namespace.

Even in desktop it looks like this was the only consumer.

@stephentoub What are your thoughts on the System.Runtime.InteropServices.ComTypes namespace? COM tends to be a four letter word, but that namespace really represents this interface's provenance.

It really doesn't matter since it is an internal-only interface. IMHO I'd just put it in the same namespace as the type that uses it.

stephentoub · 2018-07-10T02:00:02Z

...ystem.Runtime.InteropServices/src/System/Runtime/InteropServices/StandardOleMarshalObject.cs

+            return NativeMethods.E_NOTIMPL;
+        }
+    }
+}


Any tests we can add as part of this?

Added issue #30952 to track this.

jkotas · 2018-07-10T08:13:21Z

src/Common/src/Interop/Windows/Ole32/Interop.CoGetStandardMarshal.cs

+{
+    internal static partial class Ole32
+    {
+        [DllImport("ole32.dll")]


Interop.Libraries.Ole32

jkotas · 2018-07-10T08:14:35Z

src/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs


+namespace Microsoft.Win32
+{
+    internal partial interface IMarshal


Internal types should not be in refererence assembly.

jkotas · 2018-07-10T08:20:50Z

...ystem.Runtime.InteropServices/src/System/Runtime/InteropServices/StandardOleMarshalObject.cs

+{
+    public class StandardOleMarshalObject : MarshalByRefObject, IMarshal
+    {
+        private static readonly Guid Clsid_StdMarshal = new Guid("00000017-0000-0000-c000-000000000046");


This should be CLSID_StdMarshal. https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/interop-guidelines.md#naming

ericstj · 2018-07-10T15:18:28Z

src/System.Runtime.InteropServices/ref/System.Runtime.InteropServices.cs

+    public partial class StandardOleMarshalObject : MarshalByRefObject, Microsoft.Win32.IMarshal
+    {
+        protected StandardOleMarshalObject() { }
+        int Microsoft.Win32.IMarshal.GetUnmarshalClass(ref Guid riid, IntPtr pv, int dwDestContext, IntPtr pvDestContext, int mshlflags, out Guid pCid) { throw null; }


Similarly, these are effectively not public.

AaronRobinsonMSFT · 2018-07-10T17:19:27Z

src/Common/src/Microsoft/Win32/IMarshal.cs

+        int DisconnectObject(int dwReserved);
+    }
+
+    internal static class NativeMethods


Not to be too dogmatic, but there is no way these constants aren't already defined somewhere. Is it possible to use them?

luqunl · 2018-07-10T22:54:52Z

src/System.Runtime.InteropServices/src/System.Runtime.InteropServices.csproj

    <Compile Include="System\Runtime\InteropServices\ExporterEventKind.cs" />
    <Compile Include="System\Runtime\InteropServices\RegistrationClassContext.cs" />
    <Compile Include="System\Runtime\InteropServices\RegistrationConnectionType.cs" />
+    <Compile Include="System\Runtime\InteropServices\StandardOleMarshalObject.cs" />


I suppose these types aren't for uap scenario. Could you add a condition for these types?

luqunl · 2018-07-10T23:14:09Z

...ystem.Runtime.InteropServices/src/System/Runtime/InteropServices/StandardOleMarshalObject.cs

+                // we must not wrap pStandardMarshal with an RCW because that would trigger QIs for random IIDs and the marshaler
+                // (aka stub manager object) does not really handle these well and we would risk triggering an AppVerifier break
+                IntPtr vtable = *(IntPtr*)pStandardMarshal.ToPointer();
+                IntPtr method = *((IntPtr*)vtable.ToPointer() + 5); // GetMarshalSizeMax is 5th slot


GetMarshalSizeMax is 5th slot

typo? it should be MarshalInterface is 5th slot

luqunl · 2018-07-10T23:36:26Z

...ystem.Runtime.InteropServices/src/System/Runtime/InteropServices/StandardOleMarshalObject.cs

+                    Marshal.Release(pUnknown);
+                }
+            }
+            throw new InvalidOperationException(string.Format(SR.StandardOleMarshalObjectGetMarshalerFailed, riid.ToString()));


Nit: This doesn't feel correct. These COM methods are marked as [PreserveSig] but in here we are going to throw exception. since it works in desktop, maybe we just keep it as it is.

jkotas · 2018-07-11T19:44:05Z

src/System.Runtime.InteropServices/src/System.Runtime.InteropServices.csproj

    <Compile Include="System\Runtime\InteropServices\ComEventInterfaceAttribute.cs" Condition="'$(TargetGroup)' != 'uapaot'" />
    <Compile Include="System\Runtime\InteropServices\DefaultParameterValueAttribute.cs" Condition="'$(TargetGroup)' != 'uapaot'" />
    <Compile Include="System\Runtime\InteropServices\HandleCollector.cs" Condition="'$(TargetGroup)' != 'uapaot'" />
+    <Compile Include="System\Runtime\InteropServices\IMarshal.cs" Condition="'$(TargetGroup)' != 'uapaot'" />


Why do we need to exclude this for uapaot? CoGetStandardMarshal is allowed for UWP apps in https://docs.microsoft.com/en-us/windows/desktop/api/combaseapi/nf-combaseapi-cogetstandardmarshal.

We have been keeping the netcoreapp and uap surface in sync.

I wasn't aware of this. I did this because of #30931 (comment)
I will revert this

Anipik · 2018-07-11T22:34:51Z

Merging this one and an issue has been added to track the tests (Most Probably tests will be added in coreclr).

* Porting IMarshal * Writing ref File and minor changes * ported from .netfx * formatting and adding aditional files * Minor fixes * Feedback addressed Commit migrated from dotnet/corefx@f70599a

Anipik added 5 commits July 9, 2018 16:01

Porting IMarshal

2d08652

Writing ref File and minor changes

c65edc9

ported from .netfx

2700380

formatting and adding aditional files

6138764

Minor fixes

7f3b8ee

Anipik requested review from danmoseley and ericstj July 9, 2018 23:51

luqunl added the area-System.Runtime.InteropServices label Jul 10, 2018

stephentoub reviewed Jul 10, 2018

View reviewed changes

jkotas reviewed Jul 10, 2018

View reviewed changes

ericstj reviewed Jul 10, 2018

View reviewed changes

jeffschwMSFT requested review from AaronRobinsonMSFT and luqunl July 10, 2018 16:08

AaronRobinsonMSFT reviewed Jul 10, 2018

View reviewed changes

luqunl reviewed Jul 10, 2018

View reviewed changes

jkotas reviewed Jul 11, 2018

View reviewed changes

Feedback addressed

61eb8e1

luqunl approved these changes Jul 11, 2018

View reviewed changes

AaronRobinsonMSFT approved these changes Jul 11, 2018

View reviewed changes

Anipik merged commit f70599a into dotnet:master Jul 11, 2018

Anipik deleted the standardOle branch July 11, 2018 22:34

karelz added this to the 3.0 milestone Jul 19, 2018

AaronRobinsonMSFT mentioned this pull request Jan 31, 2020

Consider testing for IMarshal dotnet/runtime#26753

Closed

		@@ -0,0 +1,33 @@
		// Licensed to the .NET Foundation under one or more agreements.

Porting StandardOleMarshalObject and IMarshal #30931

Porting StandardOleMarshalObject and IMarshal #30931

Uh oh!

Conversation

Anipik commented Jul 9, 2018

Uh oh!

stephentoub Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luqunl Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anipik commented Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

stephentoub Jul 10, 2018 •

edited

Loading

luqunl Jul 10, 2018 •

edited

Loading

jkotas Jul 11, 2018 •

edited

Loading

Anipik commented Jul 11, 2018 •

edited

Loading