Use ComWrappers for Clipboard #7087

kant2002 · 2022-04-26T15:09:35Z

Create missing RCW and CCW
Testing happens using existing test suite which is quite good.

Microsoft Reviewers: Open in CodeFlow

kant2002 · 2022-04-29T01:22:15Z

@RussKie can I ask you start looking into this masterpiece.

RussKie · 2022-04-29T03:31:56Z

I’ll put aside some time next week.

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.OleGetClipboard.cs

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.OleSetClipboard.cs

src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs

RussKie · 2022-05-06T02:35:01Z

src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs

-                if (dataObject is IDataObject ido && !Marshal.IsComObject(dataObject))
-                {
-                    return ido;
-                }
-


Could you please explain this removal?

This is effectively dead code after ComVisible removal. My current plan is provide for DataObject CCW's which supports 2 interfaces IComDataObject and IWinFormsDataObject which resurrect code in some form.

dataObject is always DataObjectWrapper here which implements only IComDataObject for now. And previously we have _ComObject which react just to IComDataObject, that why @filipnavara create issue in first place.

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.cs

RussKie · 2022-05-06T02:57:11Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.EnumFORMATETCWrapper.cs

+
+            internal IntPtr Instance => _wrappedInstance;
+
+            public void Dispose()


How is this called? I didn't see any direct callers.

I add finalizer after this comment. Also I leave this method for consistency with other wrappers.

I don't think the Dispose pattern shouldn't be used in this case. Consistency isn't the priority here, but rather correctness. See the note under https://docs.microsoft.com/dotnet/standard/native-interop/tutorial-comwrappers#step-4--handle-native-object-wrapper-lifetime-details.

I can attribute my ignorance to the fact that there now documentation when I start 😉 , but thank for keeping me responsible.

And crap. I add finalizer in different place. In the DataObject....

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

RussKie · 2022-05-06T03:59:47Z

Looks good. I've run some local manual tests too.

@AaronRobinsonMSFT @JeremyKuhne I'd appreciate it if you could review as well.

AaronRobinsonMSFT · 2022-05-06T15:53:41Z

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.OleSetClipboard.cs

@@ -10,6 +10,16 @@ internal static partial class Interop
    internal static partial class Ole32
    {
        [DllImport(Libraries.Ole32, ExactSpelling = true)]
-        public static extern HRESULT OleSetClipboard(IDataObject? pDataObj);
+        private static extern HRESULT OleSetClipboard(IntPtr pDataObj);


Entirely up to the WinForms owners, @RussKie and @JeremyKuhne, but this could be declared within the method body below as a static local function.

As in like this?

Suggested change

private static extern HRESULT OleSetClipboard(IntPtr pDataObj);

public static HRESULT OleSetClipboard(IDataObject? pDataObj)

{

if (pDataObj is null)

{

return OleSetClipboard(IntPtr.Zero);

}

return OleSetClipboard(WinFormsComWrappers.Instance.GetComPointer(pDataObj, IID.IDataObject));

[DllImport(Libraries.Ole32, ExactSpelling = true)]

static extern HRESULT OleSetClipboard(IntPtr pDataObj);

}

I think that's what Aaron means. Do you want me apply this change?

As in like this?

Yep.

How will it play if we switch to LibraryImport later on? Will source gen produce code for this?

I guess we'll cross that bridge when we get to it :)

Isn’t #7172 practically done. And it will land definitely faster then this one. Am I miss something?

Judging by the @JeremyKuhne's comments there are perf implications that may need to be addressed first.

My understanding on this, that this comment applied only if custom marshaller would be used with HandleRef or IHandle, but with IntPtr these concerns are not applicable. So that PR looks like it can be merged as is right now.

How will it play if we switch to LibraryImport later on? Will source gen produce code for this?

If we're just switching to LibraryImport it won't necessitate changing anything here.

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.IEnumFORMATETCVtbl.cs

AaronRobinsonMSFT · 2022-05-06T23:26:13Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

+
+internal partial class Interop
+{
+    internal unsafe partial class WinFormsComWrappers


I don't see anything here around apartment marshalling. If I recall, the entire clipboard stack is required to be on the STA. This doesn't enforce that or even bother to check, which means we can get into data corruption or random crashes. This type only ever going to be used on the STA and if so, how is that is that enforced?

In the clipboard we check:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs

Lines 40 to 43 in 93edc2d

if (Application.OleRequired() != ApartmentState.STA)

{

throw new ThreadStateException(SR.ThreadMustBeSTA);

}

Perhaps we should be doing the same here as well.

I'm all for multilayered checks, but I wonder, how .NET already handling that? Also My understnading was that I should use IAgileReference like it is here https://github.com/RussKie/ClipboardRedux

All .NET objects are free-threaded so COM servers implemented in .NET can run on any apartment. For built-in RCWs, this is incredibly complex and handled in the RCW implementation - see UnmarshalIUnknownForCurrContext for an example of the marshalling across apartments.

Yes, IAgileReference is the best way to do this on Windows 8+. However, it doesn't work on Windows 7, which means we need to have a fallback if Winforms is still required to support that OS. On pre-Windows 8 using the Global Interface Table (GIT) is required.

I believe this is mentioned in the ClipboardRedux example above.

Independent of the answer on Windows 7 support, can we just implement new Windows Forms features the new best way and say that we'll document which features are not supported on Windows 7 going forward if that proves necessary?

Put another way, if we decide to support Windows 7 again -- which I really hope not -- it's not really intended as a first-class citizen.

@AaronRobinsonMSFT and @RussKie can we consider current implementation as "limited support" for Win7? I'm not sure that Clipboard can be considered new feature. Or maybe for Win7 I can revert back to built-in Com Interop, but how all this playout with LibraryImport work. Just throwing ideas.

My take on this is that we add OS checks to use the new functionality on Win8+ and use the original implementations for Win7.

FYI .NET support for Windows 7 and 8.1 will end in January 2023

Seems to be Did not matter. I have to somehow drag to that point and I need old implementation hanging around.

Still looking for approximate list of test cases for clipboard.

RussKie

Few questions.

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

RussKie · 2022-05-09T01:01:58Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

+                        (_wrappedInstance, formatPtr, &mediumRaw).ThrowIfFailed();
+                    medium = new()
+                    {
+                        pUnkForRelease = mediumRaw.pUnkForRelease == IntPtr.Zero ? null : Marshal.GetObjectForIUnknown(mediumRaw.pUnkForRelease),


Could you please help me understand why we need to marshal again, and can't just copy mediumRaw.pUnkForRelease here?

what if somebody replace value, stored in the mediumRaw.pUnkForRelease. We do not know if somebody change value of pUnkForRelease. That's highly unlikely, but who knows.

Since there was a question here consider a clarifying comment

RussKie · 2022-05-09T01:04:02Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

+
+internal partial class Interop
+{
+    internal unsafe partial class WinFormsComWrappers


In the clipboard we check:

winforms/src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs

Lines 40 to 43 in 93edc2d

if (Application.OleRequired() != ApartmentState.STA)

{

throw new ThreadStateException(SR.ThreadMustBeSTA);

}

Perhaps we should be doing the same here as well.

RussKie · 2022-05-09T01:08:37Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

+                IntPtr enumAdvisePtr;
+                var result = ((delegate* unmanaged<IntPtr, IntPtr*, HRESULT>)(*(*(void***)_wrappedInstance + 11)))
+                    (_wrappedInstance, &enumAdvisePtr);
+                enumAdvise = result.Succeeded() ? null : (IEnumSTATDATA)Marshal.GetObjectForIUnknown(enumAdvisePtr);


@AaronRobinsonMSFT @kant2002 could you please help me understand the best practices for Marshal.GetObjectForIUnknown? According to the docs it increments the ref count. Does it mean we need to release enumAdvisePtr before we exit?

Release is responsibility of RCW finalizer.

RussKie · 2022-05-09T01:09:33Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.EnumFORMATETCWrapper.cs

+            ~EnumFORMATETCWrapper()
+            {
+                this.DisposeInternal();
+            }
+
+            private void DisposeInternal()
+            {
+                Marshal.Release(_wrappedInstance);
+                _wrappedInstance = IntPtr.Zero;
+            }


Any reason we can't inline these together?

Technically we can. That was needed because I thought that having Dispose method is valuable, but right now there no reason.

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.IEnumFORMATETCVtbl.cs

Create missing RCW and CCW Testing happens using existing test suite which is quite good.

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>

Add finalizer

AaronRobinsonMSFT · 2022-05-16T15:57:14Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.AgileDataObjectWrapper.cs

+
+            public int QueryGetData(ref FORMATETC format)
+            {
+                using var dataObject = Unwrap();


If this is how apartment support for IDataObject is being provided, then I'm not convinced we should permit it to be IDisposable. That pattern has non-trivial performance impact and is likely to be noticeable in large applications. We should instead defer to the built-in lifetime management provided by the runtime.

AaronRobinsonMSFT · 2022-05-16T15:58:14Z

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.DataObjectWrapper.cs

+{
+    internal unsafe partial class WinFormsComWrappers
+    {
+        internal sealed class DataObjectWrapper : IDataObject, IDisposable


I would expect the IAgileReference to be an implementation detail of DataObjectWrapper.

RussKie · 2022-05-30T03:39:38Z

src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs

@@ -138,13 +138,13 @@ public static void SetDataObject(object data, bool copy, int retryTimes, int ret
        /// </remarks>
        private static IDataObject? GetDataObject(int retryTimes, int retryDelay)
        {
-            IComDataObject? dataObject = null;


Since W7 support is still in scope, we need to retain the original implementation, and conditionally invoke the new implementation. E.g., using OsVersion.IsWindows8OrGreater or doing something like:

private static IDataObject? GetDataObject(int retryTimes, int retryDelay) { // new code } private static IDataObject? GetDataObjectWin7(int retryTimes, int retryDelay) { // original code }

Thanks! does agility an issue for OleSetClipboard in same way as for OleGetClipboard?

JeremyKuhne · 2023-07-05T17:32:36Z

@kant2002 unfortunately resource constraints have pushed this out of .NET 8. We hope to tackle the rest of the COM interop for .NET 9.

I appreciate all of the effort here and if you're interested, I'll keep you looped in. As the COM code has changed significantly, I think it's better to close this one and use it as a reference in a new PR.

Note: One particular difficulty here is that we need to make sure we've fully understood and documented changes in castability. Without built-in COM interop you don't get the "magic" casting of pointers back to their original objects or out to arbitrary implemented [ComImport] interfaces on the types. I haven't had the time to deep-dive into mapping this out, but I suspect we may have to have AppContext switches in some places. I'll capture this in more detail as I get our tracking issues into shape.

kant2002 · 2023-07-05T17:36:31Z

@JeremyKuhne I agree with you on this one. And since I did not push this in time we should rethink how it can be done in .NET 9. I still interested in this area, but cannot allocate cycles for pushing this forward.

kant2002 requested a review from a team as a code owner April 26, 2022 15:09

ghost assigned kant2002 Apr 26, 2022

kant2002 mentioned this pull request Apr 26, 2022

Broken custom IDataObject implementations #4555

Closed

RussKie self-requested a review May 3, 2022 00:08

RussKie added the waiting-review This item is waiting on review by one or more members of team label May 3, 2022

RussKie reviewed May 6, 2022

View reviewed changes

RussKie removed the waiting-review This item is waiting on review by one or more members of team label May 6, 2022

RussKie added 📭 waiting-author-feedback The team requires more information from the author waiting-review This item is waiting on review by one or more members of team labels May 6, 2022

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label May 6, 2022

AaronRobinsonMSFT reviewed May 6, 2022

View reviewed changes

kant2002 mentioned this pull request May 7, 2022

Provide CCW for IDataObject #6976

Merged

RussKie reviewed May 9, 2022

View reviewed changes

kant2002 mentioned this pull request May 10, 2022

Add IEnumFormatEtc ComWrapper #7162

Merged

kant2002 and others added 4 commits May 16, 2022 11:59

Use ComWrappers for Clipboard

2622cc7

Create missing RCW and CCW Testing happens using existing test suite which is quite good.

Apply suggestions from code review

90a2bca

Co-authored-by: Igor Velikorossov <RussKie@users.noreply.github.com>

Fix compilation errors after

a2cf976

Add finalizer

Fix style warning

0c812d2

kant2002 force-pushed the kant/comwrappers-clipboard branch from f7b52b9 to 0c812d2 Compare May 16, 2022 06:00

kant2002 added 2 commits May 16, 2022 20:28

Attempt to implement AgileReference

860ceec

Remove usings

a5b13b2

AaronRobinsonMSFT reviewed May 16, 2022

View reviewed changes

RussKie reviewed May 30, 2022

View reviewed changes

kant2002 added 3 commits June 3, 2022 17:02

Add comment

62a6a5f

Restore old codepath for Win7

b5eb454

Wrap COM implementation in try/catch

3d51ec5

kant2002 mentioned this pull request Jul 31, 2022

Add RCW for IDataObject #7519

Closed

JeremyKuhne added the area-COM label Jul 5, 2023

JeremyKuhne added 📭 waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Jul 5, 2023

ghost removed the 📭 waiting-author-feedback The team requires more information from the author label Jul 5, 2023

JeremyKuhne closed this Jul 5, 2023

ghost locked as resolved and limited conversation to collaborators Aug 4, 2023


		internal IntPtr Instance => _wrappedInstance;

		public void Dispose()

-        private static extern HRESULT OleSetClipboard(IntPtr pDataObj);
+        public static HRESULT OleSetClipboard(IDataObject? pDataObj)
+        {
+            if (pDataObj is null)
+            {
+                return OleSetClipboard(IntPtr.Zero);
+            }
+            return OleSetClipboard(WinFormsComWrappers.Instance.GetComPointer(pDataObj, IID.IDataObject));
+            [DllImport(Libraries.Ole32, ExactSpelling = true)]
+            static extern HRESULT OleSetClipboard(IntPtr pDataObj);
+        }

	if (Application.OleRequired() != ApartmentState.STA)
	{
	throw new ThreadStateException(SR.ThreadMustBeSTA);
	}

Use ComWrappers for Clipboard #7087

Use ComWrappers for Clipboard #7087

Conversation

kant2002 commented Apr 26, 2022 • edited by ghost Loading

Microsoft Reviewers: Open in CodeFlow

kant2002 commented Apr 29, 2022

RussKie commented Apr 29, 2022 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented May 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AaronRobinsonMSFT May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne commented Jul 5, 2023

kant2002 commented Jul 5, 2023

kant2002 commented Apr 26, 2022 •

edited by ghost

Loading

AaronRobinsonMSFT May 9, 2022 •

edited

Loading