microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2.07k stars 86 forks source link

IShellWindows.FindWindowSW definition throws NotSupportedException unless `int*` is changed to `out int` #860

Closed jnm2 closed 1 year ago

jnm2 commented 1 year ago

I tracked this down to the int* phwnd parameter. It works just fine if you edit the generated declaration to declare the parameter as out int. For example, https://github.com/microsoft/nodejstools/blob/main/Nodejs/Product/Nodejs/SharedProject/SystemUtilities.cs#L26 uses an out int definition.

Documentation: https://learn.microsoft.com/en-us/windows/win32/api/exdisp/nf-exdisp-ishellwindows-findwindowsw#parameters

image

Repro steps

  1. NativeMethods.txt content:

    ShellWindows
    IShellWindows
    CSIDL_DESKTOP
    IServiceProvider
    ShellWindowTypeConstants
    ShellWindowFindWindowOptions
  2. NativeMethods.json content (if present):

  3. Any of your own code that should be shared?

var shellWindows = (IShellWindows)new ShellWindows();

int hwnd;
var serviceProvider = (IServiceProvider)shellWindows.FindWindowSW(
    PInvoke.CSIDL_DESKTOP,
    pvarLocRoot: null,
    (int)ShellWindowTypeConstants.SWC_DESKTOP,
    &hwnd,
    (int)ShellWindowFindWindowOptions.SWFO_NEEDDISPATCH);

Context

AArnott commented 1 year ago

This is a very interesting problem. I guess the object that implements IShellWindows isn't in the same process, and the proxy that remotes the call can't handle pointers. Although a friendly overload offers an out int parameter declaration, that friendly overload ultimately calls the method with the pointer declaration, so it still fails. I can repro the problem, and the working solution after hand-editing as you describe.

I don't know how CsWin32 can predict at generation time when an interface will be implemented by a remoted object. Short of that, the only remedy I can imagine is to always emit non-optional parameters that are pointers to structs as in, ref or out parameters. That has the downside that in-proc interface uses will no longer have the ability to use pointers if they have them. I'm not sure how important that is.

@jeremykuhne do you have any insights?

JeremyKuhne commented 1 year ago

@AaronRobinsonMSFT I presume this is COM interop explicitly expecting an out int. If so, why does it expect it, and is there a way around it?

AaronRobinsonMSFT commented 1 year ago

@AaronRobinsonMSFT I presume this is COM interop explicitly expecting an out int. If so, why does it expect it, and is there a way around it?

Nope. The out keyword means nothing in practice from an interop perspective. All it does it force marshaller to pin parameters and/or copy memory around. Basically whenever out/in/ref is written in C#, it means pointer and potentially copy - nothing more.

I'm not sure what is going on here either, it needs to be debugged, but I can say there is an issue with that signature and possible that is the root issue. The FindWindowSW takes 6 parameters not 5. See the referenced doc above and below is the definition in ExDisp.h.

virtual /* [hidden][helpstring] */ HRESULT STDMETHODCALLTYPE FindWindowSW(
    /* [in] */ __RPC__in VARIANT *pvarLoc,
    /* [in] */ __RPC__in VARIANT *pvarLocRoot,
    /* [in] */ int swClass,
    /* [out] */ __RPC__out long *phwnd,
    /* [in] */ int swfwOptions,
    /* [retval][out] */ __RPC__deref_out_opt IDispatch **ppdispOut) = 0;
AaronRobinsonMSFT commented 1 year ago

Oh, the out is being converted to a "return". Ignore the parameter count comment.

AaronRobinsonMSFT commented 1 year ago

@AArnott Can you share the generated definition so I can take a look?

AArnott commented 1 year ago

Sure, here you go @aaronrobinsonmsft (trimmed to just the relevant method):

[Guid("85CB6900-4D95-11CF-960C-0080C7F4EE85"),InterfaceType(ComInterfaceType.InterfaceIsIDispatch),ComImport()]
[global::System.CodeDom.Compiler.GeneratedCode("Microsoft.Windows.CsWin32", "0.2.188-beta+852e176a3b")]
internal interface IShellWindows
{
   [return: MarshalAs(UnmanagedType.IDispatch)]
   unsafe object FindWindowSW([MarshalAs(UnmanagedType.Struct)] in object pvarLoc, [MarshalAs(UnmanagedType.Struct)] in object pvarLocRoot, int swClass, int* phwnd, int swfwOptions);
}

If you want a full compilable repro, here you go: CsWin32Sandbox.zip I even copied the generated code into a user file so you can try switching int* to out int to see how that fixes the problem.

By the way, the exception I get is not NotSupportedException, but rather:

System.Runtime.Remoting.RemotingException
  HResult=0x8013150B
  Message=Pointer types cannot be passed in a remote call.
  Source=mscorlib
  StackTrace:
   at System.Runtime.Remoting.Messaging.Message.InternalGetArgs()
   at System.RuntimeType.ForwardCallToInvokeMember(String memberName, BindingFlags flags, Object target, Int32[] aWrapperTypes, MessageData& msgData)
   at Windows.Win32.UI.Shell.IShellWindows.FindWindowSW(Object& pvarLoc, Object& pvarLocRoot, Int32 swClass, Int32* phwnd, Int32 swfwOptions)
   at Program.<Main>$(String[] args) in C:\Users\andarno\source\repos\CsWin32Sandbox\CsWin32Sandbox\Program.cs:line 9
JeremyKuhne commented 1 year ago

@AaronRobinsonMSFT I was guessing that this was something relating to casting the COM class object to the interface definition. Like COM interop was generating what it thought the sigs should be based off of the registered type library and not being happy with it not matching the interface definition.

Total guess however. I'm unclear on the TLB dependencies that COM interop has. :)

AaronRobinsonMSFT commented 1 year ago

System.Runtime.Remoting.RemotingException HResult=0x8013150B

It seems the OP is running on .NET 6, and I assume the repro you've provided is on .NET Framework. I also didn't realize this could be a remoting scenario, that does introduce other issues depending on the interface definition in COM. I'm interested in why this is failing this way on .NET 6 though, it is rather annoying. From a remoting perspective I would've expected [Out] to be the goto solution here rather than needing the out keyword and by-ref, although perhaps the by-ref is a distraction.

I'll try to take a look tomorrow. Can someone try with a pointer and the [Out] attribute?

AArnott commented 1 year ago

@AaronRobinsonMSFT The repro sln I sent you multitargets net48 and net7.0-windows. I see that NotSupportedException is actually the exception type thrown by .NET 7.

Adding [Out] to the parameter declaration does not fix it, on either runtime.

AaronRobinsonMSFT commented 1 year ago

I am missing some history here. I am able to reproduce this issue and it is indeed an intentional block and I can understand why for .NET Framework. Let me do some research and see what I can find. This is by-design for .NET Framework due to how Remoting was implemented. For .NET Core, it is possible this is no longer a requirement since we no longer have Remoting. However, I need to root around and see if I can understand "why" this decision was made. COM supports this so I'm not sure why .NET has blocked it.

AArnott commented 1 year ago

In the meantime, I wonder if we should push on the metadata to represent this method with out int instead of int* if at some fundamental level, out int is more correct. We need something to help Cswin32 generate the right declaration here (yes, we need .NET Framework to work). Should we always change COM interface methods with [Out] some* to out Some, or is this a special case that the metadata should cover?

AaronRobinsonMSFT commented 1 year ago

I have dug around history and I see the issue and it is annoying, but truthfully the current behavior is correct. First things first, this is by-design in both .NET Core and .NET Framework and won't change.

The details on why are subtle but the gist is an int* in C# is ambiguous from the marshaller's perspective. Is this a pointer value? a pointer to an int to reference? a pointer to a pinned int array? The marshaller just doesn't know and so a raw pointer type is invalid because of all the things it could be. One can see glimpses of this in the VARIANT type, which is how parameters are defined in this scenario - notice that VT_PTR isn't permitted, but VT_BYREF is. In this case, the built-in marshaller for this sort of remote invocation is strictly following the rules for TLBs and raw pointers aren't permitted.

Note that if the interface is used for local COM interop this is marshalled as desired because we aren't using the dispatch based marshaller. The out of proc nature is the problem here and is expressed as a RemotingException on .NET Framework, but a generic NotSupportedException on .NET Core. More than willing to provide a better message if someone wants to file an issue.

@AArnott The solution here is to express the intent using the C# keywords as precisely as possible. Using out Some is the preferred approach, [Out] should be rarely needed.

AArnott commented 1 year ago

It sounds like this is sometimes an issue with IDispatch interfaces. So CsWin32 could switch from [Out] Struct* to out Struct for only the IDispatch-derived interfaces to fix this problem, right? It also sounds like it's only a problem when the COM interface is remoted, but as CsWin32 cannot know whether a COM interface will be used in a remoting scenario, it sounds like it should just always use out Struct for dispatch interfaces. Does that sound reasonable?

AaronRobinsonMSFT commented 1 year ago

It sounds like this is sometimes an issue with IDispatch interfaces.

Not really. TLBs can also contain non-IDispatch based interfaces. This is more fundamental. If an interface is going to be used in an out of proc context, we don't support using pointer types - at least not that I can see. The best approach here is to understand the intent of the pointer to properly express that in the signature.

but as CsWin32 cannot know whether a COM interface will be used in a remoting scenario, it sounds like it should just always use out Struct

Right. But remember this isn't just limited to IDispatch types.

AArnott commented 1 year ago

Ok, so it sounds like CsWin32 should attempt to translate all pointers in COM interface method signatures to in out or ref modifier non-pointer parameters. That seems to put us more in the direction of relying on .NET marshaling (where I thought we were trying to get away from that since .NET 8 w/ trimming eliminates the marshaler, or something like that, but oh well.