Convert file dialog COM usage to CsWin32

[JeremyKuhne]

Directly uses COM pointers when accessing the native dialog.

This also rewrites the CCW for the dialog options to use the CsWin32 generated interface and moves the statics into the CsWin32 COM struct (which is partial).

Adds better helpers to initialize vtables and find the CCW for a given object.

Removes our DECIMAL and BSTR definitions.

Adds an enum helper for setting/clearing flags based on a boolean value.

Microsoft Reviewers: Open in CodeFlow

[elachlan]

CC: @kant2002

[JeremyKuhne]

FYI: @AArnott, @AaronRobinsonMSFT, @jkoritzinsky, @jacdavis, @elinor-fung, @vitek-karas, @LakshanF, @tannergooding

We're getting enough in place from CsWin32 to start basing our ComWrappers on it. Putting our statics in the partial structs generated by CsWin32, IFileDialogEvents is the example here. The implementation is pretty simple- hopefully we can move more to the code generators. We'll continue to convert things to build confidence in our patterns.

[JeremyKuhne]

This is our ComWrappers "CCW" implementation. Pretty boilerplate stuff now.

[lonitra]

Should we check if ppenum is null before this?

[JeremyKuhne]

We can add that when we convert this one over.

[lonitra]

🥳

[JeremyKuhne]

Thanks to @lonitra for sitting through a good chunk of this with me.

[JeremyKuhne]

If there are any other comments please leave them. I'll be keeping an eye on this. Getting it in as we have further changes backed behind it. :)

[AaronRobinsonMSFT]

This represents a substantial increase in unused metadata if the Vtbl is simply being projected into managed by yet another type.

[JeremyKuhne]

This is only done for projecting to native from managed. In that case we need to keep all of our callbacks so I'm not sure what value it adds in this case?

[AaronRobinsonMSFT]

Do we need to check for format here too? It is dereferenced below.

[JeremyKuhne]

Yeah, when we convert this one we're going to make sure we validate all of these out dereferences.

[AaronRobinsonMSFT]

Check format is null?

[AaronRobinsonMSFT]

I'll stop commenting.

[AaronRobinsonMSFT]

Does this return an HRESULT or is this using the built-in exception throwing?

[JeremyKuhne]

We'll be porting this one over to the new format shortly.

[AaronRobinsonMSFT]

This will never return null.

[JeremyKuhne]

What does it return when something has been deref'ed to 0?

[AaronRobinsonMSFT]

Calling a function on a zero ref CCW is a bug and undefined.

[AaronRobinsonMSFT]

I think the more common pattern would be naming the enclosing class COMDLG32 and then specifying the file name using nameof(COMDLG32). I'd also consider using LibraryImport for these definitions.

[JeremyKuhne]

I can't use LibraryImport in many cases as we reference types from CsWin32. This one is a temporary hack as the Windows metadata doesn't project a platform agnostic type (the issue is linked above it). It will be moved to CsWin32 once the issue is resolved.

[AArnott]

You shouldn't need to declare this method. If you just declare the struct (which is the arch-specific API), then CsWin32 should be willing to generate the method that depends on it.

[AaronRobinsonMSFT]

The [In][Out] have no functional value here. If they are being used to express indent, that seems fine, but I would avoid them and rely on a comment instead.

[JeremyKuhne]

Noted. This is another temporary because of the Win32 metadata issue linked.

[AaronRobinsonMSFT]

I'm not sure this is a good idea. A delegate allocation is occurring for each invocation. Based on the pointer usage and all the other unsafe C# going on here, this is out of place.

[JeremyKuhne]

I thought there were improvements made here regarding caching these? In any case, the hope is to move this to source generation (in CsWin32) so we might be able to entirely avoid it. I knew this wasn't as efficient but decided that it is more important (for our usages) that we're easily made consistent and the code is easy to maintain.

[AArnott]

The compiler cannot cache a delegate that captures local variables. This one captures pfd.

[RussKie]

A bit of related info can be found here: https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/ in the paragraph that starts with "Let’s start with IDE0200".

[jacdavis]

Marshal.Release((nint)(void*)dropTargetPtr);

this leaks if RegisterDragDrop fails. Put in finally?

[JeremyKuhne]

This one should only fail if there was something truly catastrophic (AV or OOM).

[jacdavis]

/ 2;

This assumption is only correct if the BSTR is utf16. It is ALWAYS utf16 on 32 bit windows, but if we were ever to do a cross plat version of winforms this would be a bug (most posix os's use utf8 for the in-memory representation). In native code SysStringLen abstracts out this dependency and is a fast invocation. Would it be better to pinvoke that with a BSTR*? Note that in other cross plat implementations of com code bases, we've relied on PAL conversion of platform dependencies and run into lots of issues with dependencies like this.

[JeremyKuhne]

The runtime does this directly as well: https://github.com/dotnet/runtime/blob/c6c8bb8b73ff4f7a686ca5a350fbb0093b3b708b/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.cs#L1263

The generated code also does it, but we can get that changed. If we do we'd probably want to get CsWin32 to add the [SuppressGCTransition] attribute to make it actually speedy.

Fwiw, the odds of doing an x-plat WinForms are astronomically low so we take Win32 as a given.

@AaronRobinsonMSFT, any thoughts on this from the Marshal perspective? Would exposing a GetBSTRLength() method on Marshal be plausible?

[jacdavis]

Fair enough, but I mention it because this has been a very costly issue for me in the past. Using the length prefix ties to what is essentially an implementation detail. In native code, invoking SysStringLen is only a few instructions longer where the abstraction exists and avoids the problem. For native, avoiding the pinvoke may be reason enough to accept the risk here.

[AArnott]

Using the length prefix ties to what is essentially an implementation detail

BSTR's length prefix is not only documented, but hard-coded in several runtimes. It couldn't possibly change, nor would I feel bad about depending on it given it's documented. Why would you hesitate? Or did I misunderstand what the impl detail was that you were talking about, @jacdavis ?

The pointer arithmetic is complex, but if it works, fine. I wonder though why the hesitation to just reuse Marshal.PtrToStringBSTR?
Also, what is [SuppressGCTransition] and why would its use be important here? Our own method would just be calling the Marshal method, which itself is managed.

[jacdavis]

It is only documented as being the length of the string in bytes. If the internal representation of a character changes as it can do on posix based systems that use UTF8 (1-4 byte variable length chars), then the conversion from bytes to characters changes becomes much more complex. I believe we finally standardized on utf16 for bstrs in the PAL even cross plat, but this means we constantly have to convert to and from utf8 when calling apis. I have no issue with leaving this as-is if we think we'll never hit this. However, I will say that I never expected to be porting the debugger cross plat either where I have hit this.

[jacdavis]

All are reasons why I believe we standardized on utf16 for bstrs in the PAL. However, it becomes very problematic as soon as you start passing to os apis etc... I'm fine with leaving it as-is. The pinvoke cost is probably too high to justify the change here.

[RussKie]

@jacdavis what does PAL stand for?

[AaronRobinsonMSFT]

@AaronRobinsonMSFT, any thoughts on this from the Marshal perspective? Would exposing a GetBSTRLength() method on Marshal be plausible?

We could do it. I am dubious it is worth the effort though. Feel free to submit an api-proposal. It is likely this has already come up before though. I think the current approach is likely fine. My only minor quibble would be to use sizeof(char).

@jacdavis what does PAL stand for?

"Platform Abstraction Layer" or "Platform Adaptation Layer". The .NET runtime has one to help paper over platform differences. Unfortunately ours is sort of an Win32 emulation layer instead of a nicely abstracted PAL.

[AArnott]

FWIW CsWin32 will use sizeof(char) in its version of this, which is:

unsafe int Length => this.Value is null ? 0 : checked((int)(*(((uint*)this.Value) - 1) / sizeof(char)));

[AaronRobinsonMSFT]

Another data point that might be helpful is that in .NET Core/.NET 5+, the following marshalling scenarios have been marked obsolete: UnmanagedType.AnsiBStr and UnmanagedType.TBStr. We've not removed them yet, but since they've been marked this way for so long with perhaps one or two questions, I think we are close to a point where they could be removed entirely. For the new LibraryImport source generator, they aren't supported in any form.

[AArnott]

tip: using Assert.Equal(0, dllName.Length) is better than using True == because when it fails, the exception will tell you more about what the actual value was. In fact if you use Assert.Equal(string.Empty, dllName) here, you'd get the actual string in a failure case, which would be optimal.

[RussKie]

[nit] Please keep these lexographically sorted.

[RussKie]

A bit of related info can be found here: https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/ in the paragraph that starts with "Let’s start with IDE0200".