dotnet / winforms

Windows Forms is a .NET UI framework for building Windows desktop applications.
MIT License
4.41k stars 984 forks source link

IDataObject.GetData with non-serializable type returns null #7864

Closed TheLeftExit closed 2 years ago

TheLeftExit commented 2 years ago

.NET version

.NET 7 RC1

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

Yes, up to and including .NET 6.

Issue description

Hello,

My name is Norton, and I'm writing on behalf of the WinForms team at DevExpress.

Our customers who were testing our controls with the .NET 7 RC1 runtime reported issues when using our controls' drag-drop capabilities. Upon further research, we found what seems to be a breaking change in .NET 7: whenever an instance of a non-serializable class is used with the Control.DoDragDrop method, calling IDataObject.GetData with that class as a parameter afterwards returns null rather than the dragged object.

We could not find any issues/PRs or MSDN articles documenting this change. Could you have a look and let us know if this change is intended?

Steps to reproduce

I attached a sample application. To replicate this behavior, run the project and start dragging anywhere on the form. The Debugger.Break method is called to indicate that the GetData method returned null.

If the type of the dragged object is made serializable (e.g. by modifying the using statement in Form1.cs), this behavior no longer takes place.

DXApplication1.zip

RussKie commented 2 years ago

@kant2002 could it be related to any of your work?

kant2002 commented 2 years ago

This was introduced in the https://github.com/dotnet/winforms/pull/7589 Reason for the issue, is that Ole32.IDropTarget.DragEnter should receive original .NET DataObject because of unwrapping local CCW. Currently this promise was broken when I fix #7518 where remote COM proxy for Ole IDataObject requires agility(?).

I have option to undo work which touch IDataObject but if possible that I add test automation for all related fixes and I manage to fix implementation, I would appreciate possibility to leave things with current implementation.

RussKie commented 2 years ago

I have option to undo work which touch IDataObject but if possible that I add test automation for all related fixes and I manage to fix implementation, I would appreciate possibility to leave things with current implementation.

The main focus is to ensure the compat. If we can't address the issue quickly or within the limited scope we'll have no other option but to revert the orignal changes. The release window is closing very fast.

/cc: @JeremyKuhne @dreddy-work @AaronRobinsonMSFT

TheLeftExit commented 2 years ago

Thanks for the info @kant2002.

We would strongly prefer that the behavior stayed the same as it is in .NET 6. Much of our code relies on the fact that serializability is not a requirement if the dragged object does not cross application boundaries (as per the Control.DoDragDrop doc). Implementing a serialization compatibility layer for all affected classes in our suite would require a lot of effort from us, especially within such a short timeframe.

kant2002 commented 2 years ago

@TheLeftExit I do not even discuss backward compatibility aspect. If I want move-fast-break everything I would stick with JS community. I just thinking about how to preserve refactoring, at the same time without breaking compat. Just want better NativeAOT story for WinForms and checking if I can somehow preserve that.

StefanKoell commented 2 years ago

Hi! I just installed .net 7 rc2 and it seems not be fixed. Did the fix not make it to rc2?

StefanKoell commented 2 years ago

To add to my previous comment: the fix seems to be merged into the rc2 release but the issue itself is not fixed. The result of e.Data.GetData(typeof(someobject)) is still always null when the type is not serializable.

Using 7.0.100-rc.2.22477.23 and also tested the attached sample project in this issue.

noxe commented 2 years ago

@StefanKoell - see https://github.com/dotnet/winforms/pull/7878 -> "no, RC2 is already locked. This is for RTM/GA ."

StefanKoell commented 2 years ago

@noxe that's unfortunate but thanks for letting me know. Would have loved to test this beforehand.

noxe commented 2 years ago

same here :(

Ashley-Li commented 2 years ago

Verified on latest .NET 8.0.100-alpha.1.22513.16 and .NET 7.0 RTM, run the customer's project and start dragging a picture or txt on the form designer. The [Debugger.Break] method is called to indicate that the GetData method returned null. In Addition, tested the .NET 6.0 and it behaves the same way. @TheLeftExit Could you please check my test result(below gif)?If I missed anything, please help me point it out. Thanks. issue7864

StefanKoell commented 2 years ago

Hi @Ashley-Li ! Although I'm not the author of the sample project, I guess the test is written in a way to use the TDraggableObject (see using on top) where you can easily swap out a serializable and non-serializable object for testing. The data object is set in the mouse down event/override. So dragging a file from the desktop will always return null when calling the GetData method with the typeof(TDraggableObject). To test the behavior, simple click and hold on the form and start dragging.

kant2002 commented 2 years ago

@Ashley-Li we have issues in both cases. Drag from desktop and drag of the form, so testing of drag from desktop is also important.

Ashley-Li commented 2 years ago

Verified on latest .NET 8.0.100-alpha.1.22513.16 and .NET 7.0 RTM, run the customer's project and simple click and hold on the form designer and start dragging. It cannot go to the Debugger.Break method. 8 0_7864_verify

Verified on.NET SDK 7.0.100-rc2.2.22477.23, run the customer's project and simple click and hold on the form designer and start dragging. It will go to the Debugger.Break method. 7 0rc2_result

StefanKoell commented 2 years ago

Thank you for confirming @Ashley-Li This looks promising and seems to fix the issue in 7 RTM. Looking forward to the release and hope no other regression brings back the faulty behavior.

Ashley-Li commented 2 years ago

Hi @TheLeftExit, please download the latest .NET 8.0 or .NET 7.0 RTM to verify this issue again.

TheLeftExit commented 2 years ago

Hi @Ashley-Li, tested with the latest .NET 8 build and it's fixed on my side as well. Thanks for checking!

RussKie commented 2 years ago

Great to hear. Thanks again to all involved for getting it fixed.

Amy-Li03 commented 2 years ago

Verified this issue on .NET 7.0.100 GA build, issue was fixed, the IDataObject.GetData method doesn't return null when clicking and holding on the form designer and start dragging.