Provide CCW for IDataObject #6976

kant2002 · 2022-04-06T09:44:25Z

Progress towards #5163

Microsoft Reviewers: Open in CodeFlow

RussKie · 2022-04-06T23:18:56Z

The build is failing

RussKie · 2022-04-06T23:29:47Z

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

RussKie · 2022-04-06T23:22:13Z

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

+            [UnmanagedCallersOnly]
+            private static HRESULT GetData(IntPtr thisPtr, FORMATETC* format, STGMEDIUM_Raw* pMedium)
+            {
+                var inst = ComInterfaceDispatch.GetInstance<IDataObject>((ComInterfaceDispatch*)thisPtr);


[nit]

Suggested change

var inst = ComInterfaceDispatch.GetInstance<IDataObject>((ComInterfaceDispatch*)thisPtr);

var instance = ComInterfaceDispatch.GetInstance<IDataObject>((ComInterfaceDispatch*)thisPtr);

ditto other places

RussKie · 2022-04-06T23:23:56Z

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

+                    STGMEDIUM medium = new();
+                    medium.pUnkForRelease = Marshal.GetObjectForIUnknown(pMedium->pUnkForRelease);
+                    medium.tymed = pMedium->tymed;
+                    medium.unionmember = pMedium->unionmember;
+                    inst.GetDataHere(ref *format, ref medium);
+                    pMedium->pUnkForRelease = medium.pUnkForRelease == null ? IntPtr.Zero : Marshal.GetIUnknownForObject(medium.pUnkForRelease);
+                    pMedium->tymed = medium.tymed;
+                    pMedium->unionmember = medium.unionmember;
+                    return HRESULT.S_OK;


The code would benefit from reformatting and having few empty lines to improve readability

Suggested change

STGMEDIUM medium = new();

medium.pUnkForRelease = Marshal.GetObjectForIUnknown(pMedium->pUnkForRelease);

medium.tymed = pMedium->tymed;

medium.unionmember = pMedium->unionmember;

inst.GetDataHere(ref *format, ref medium);

pMedium->pUnkForRelease = medium.pUnkForRelease == null ? IntPtr.Zero : Marshal.GetIUnknownForObject(medium.pUnkForRelease);

pMedium->tymed = medium.tymed;

pMedium->unionmember = medium.unionmember;

return HRESULT.S_OK;

STGMEDIUM medium = new()

{

pUnkForRelease = Marshal.GetObjectForIUnknown(pMedium->pUnkForRelease);

tymed = pMedium->tymed;

unionmember = pMedium->unionmember;

};

instance.GetDataHere(ref *format, ref medium);

pMedium->pUnkForRelease = medium.pUnkForRelease is null ? IntPtr.Zero : Marshal.GetIUnknownForObject(medium.pUnkForRelease);

pMedium->tymed = medium.tymed;

pMedium->unionmember = medium.unionmember;

return HRESULT.S_OK;

ditto other places

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

JeremyKuhne · 2022-04-06T23:38:15Z

@kant2002 can you please summarize what was broken and why?

kant2002 · 2022-04-07T00:08:24Z

@JeremyKuhne better look at this #6981. Yesterday I was probably in a rush to fix, and did not realize that much smaller fix needed. I think for validation that would be easier fix to get, since if you ask me to add tests it can be a much bigger change, and I do want Preview 4 to be in working conditions.

We will still need these changes.

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

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

alexrp · 2022-04-13T02:37:17Z

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

+                return result;
+            }
+
+            result = RegisterDragDrop(hwnd.Handle, dropTargetPtr);


This might be a silly question (still wrapping my head around ComWrappers for my own project): Where/how does dropTargetPtr actually get released? My understanding is that QueryInterface increments its ref count by 1, but I see no (obvious) matching Release anywhere.

RegisterDeagDrop also call AddRef. So as RevokeDragDrop https://docs.microsoft.com/en-us/windows/win32/api/ole2/nf-ole2-revokedragdrop#remarks

Releasing of this object in this particular case happens on unmanaged part.

Hmm, I think I'm still confused.

QueryInterface -> +1 ref

RegisterDragDrop -> +1 ref

RevokeDragDrop -> -1 ref

Seems like we're missing a decrement in the process? A Release after RegisterDragDrop would make sense to me since RegisterDragDrop will already have incremented the ref count in order to be able to hang onto the object.

I think you are right. and after passing pointer to RegisterDragDrop we should release it.

I apply these suggestion in the #6981 because that's smallest PR which is fix the bug, and here is too much additional stuff.

FWIW, I think there were other uses of GetComPointer / GetOrCreateComInterfaceForObject with the same problem when I was looking through the COM interop code in WinForms. Might want to double-check all uses of these.

Progress towards dotnet#5163

- Trace exceptions - Use helper functions to simplify interop - Use object initializer

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

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

AaronRobinsonMSFT

@RussKie and @JeremyKuhne This LGTM.

willibrandon · 2022-05-06T06:40:33Z

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

+                {
+                    STGMEDIUM medium = new()
+                    {
+                        pUnkForRelease = pMedium->pUnkForRelease == IntPtr.Zero ? null : Marshal.GetObjectForIUnknown(pMedium->pUnkForRelease),


I see a System.ExecutionEngineException getting thrown here after merging these changes.

@willibrandon Can you give me test case ?

Working on a mini test case now and will get it to you asap.

Separate question. Should I test on your branch?

And you can give me vague hints, so I can try repro separately. Do not like when my work broke something.

Sorry for being vague, it was late for me last night and was stuck in meeting today. I have not yet confirmed if I can reproduce the issue on the main branch but I'm taking a look now. It could be something that I'm doing incorrectly so first let me confirm a verifiable scenario exists before you waste time looking into it.

ExecutionEngineException might take me some time to confirm, and it could be an issue on my end, so please bear with me. Leading up to that I see NotImplementedException thrown in WinFormsComWrappers, ComputeVtables.

System.NotImplementedException: 'ComWrappers for type System.Windows.Forms.DataObject+FormatEnumerator not implemented.'

That I can also observe on the main branch while debugging DragDrop_QueryDefaultCursors_Async().

Do we need to add a ComWrappers IEnumFORMATETC implementation?

That should handled by #7087

willibrandon · 2022-05-11T04:03:08Z

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

+
+            internal struct STGMEDIUM_Raw
+            {
+                public IntPtr pUnkForRelease;


It seems the ExecutionEngineException was due to the STGMEDIUM_Raw struct fields being out of order? But I see it is fixed in #7087.

ooh, yeah. You are right. I forget about that problem.

I update #7162

kant2002 requested a review from a team as a code owner April 6, 2022 09:44

ghost assigned kant2002 Apr 6, 2022

RussKie requested a review from JeremyKuhne April 6, 2022 23:29

RussKie reviewed Apr 6, 2022

View reviewed changes

RussKie added the waiting-author-feedback The team requires more information from the author label Apr 6, 2022

ghost removed the waiting-author-feedback The team requires more information from the author label Apr 7, 2022

AaronRobinsonMSFT reviewed Apr 7, 2022

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/Ole32/Interop.DoDragDrop.cs Outdated Show resolved Hide resolved

AaronRobinsonMSFT reviewed Apr 7, 2022

View reviewed changes

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.IDataObjectVtbl.cs Outdated Show resolved Hide resolved

src/System.Windows.Forms.Primitives/src/Interop/WinFormsComWrappers.IDataObjectVtbl.cs Show resolved Hide resolved

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

alexrp reviewed Apr 13, 2022

View reviewed changes

Provide CCW for IDataObject

0ef6b07

Progress towards dotnet#5163

kant2002 force-pushed the kant/issue-6952 branch from 2ff1c87 to 0ef6b07 Compare April 16, 2022 16:57

kant2002 changed the title ~~Properly use QI for all ComWrapper created instances~~ Provide CCW for IDataObject Apr 16, 2022

kant2002 added 5 commits April 17, 2022 08:40

Fix compilation errors

f0f08eb

Apply suggestions from code review

f437ab5

- Trace exceptions - Use helper functions to simplify interop - Use object initializer

Oops

1353b64

Add missing namespace

b7494ae

Last time

5d15316

AaronRobinsonMSFT reviewed Apr 19, 2022

View reviewed changes

Apply PR suggestions

833e826

AaronRobinsonMSFT reviewed Apr 19, 2022

View reviewed changes

JeremyKuhne approved these changes Apr 19, 2022

View reviewed changes

Tanya-Solyanik approved these changes Apr 21, 2022

View reviewed changes

Tanya-Solyanik removed the waiting-review This item is waiting on review by one or more members of team label Apr 21, 2022

Tanya-Solyanik merged commit d24596e into dotnet:main Apr 21, 2022

ghost added this to the 7.0 Preview5 milestone Apr 21, 2022

kant2002 deleted the kant/issue-6952 branch April 22, 2022 01:44

RussKie mentioned this pull request Apr 26, 2022

Broken custom IDataObject implementations #4555

Closed

willibrandon reviewed May 6, 2022

View reviewed changes

kant2002 mentioned this pull request May 10, 2022

Add IEnumFormatEtc ComWrapper #7162

Merged

willibrandon reviewed May 11, 2022

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Jun 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide CCW for IDataObject #6976

Provide CCW for IDataObject #6976

kant2002 commented Apr 6, 2022 •

edited

Loading

RussKie commented Apr 6, 2022

RussKie commented Apr 6, 2022

RussKie Apr 6, 2022

RussKie Apr 6, 2022

JeremyKuhne commented Apr 6, 2022

kant2002 commented Apr 7, 2022

alexrp Apr 13, 2022

kant2002 Apr 13, 2022

alexrp Apr 13, 2022

kant2002 Apr 13, 2022

kant2002 Apr 13, 2022

alexrp Apr 13, 2022

AaronRobinsonMSFT left a comment

willibrandon May 6, 2022

kant2002 May 6, 2022

willibrandon May 6, 2022

kant2002 May 6, 2022

kant2002 May 6, 2022

willibrandon May 6, 2022

willibrandon May 7, 2022 •

edited

Loading

willibrandon May 7, 2022

kant2002 May 7, 2022

willibrandon May 11, 2022

kant2002 May 11, 2022

kant2002 May 11, 2022

	var inst = ComInterfaceDispatch.GetInstance<IDataObject>((ComInterfaceDispatch*)thisPtr);
	var instance = ComInterfaceDispatch.GetInstance<IDataObject>((ComInterfaceDispatch*)thisPtr);

Provide CCW for IDataObject #6976

Provide CCW for IDataObject #6976

Conversation

kant2002 commented Apr 6, 2022 • edited Loading

Microsoft Reviewers: Open in CodeFlow

RussKie commented Apr 6, 2022

RussKie commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeremyKuhne commented Apr 6, 2022

kant2002 commented Apr 7, 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

AaronRobinsonMSFT 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

willibrandon May 7, 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

kant2002 commented Apr 6, 2022 •

edited

Loading

willibrandon May 7, 2022 •

edited

Loading