-
Notifications
You must be signed in to change notification settings - Fork 998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Broken custom IDataObject implementations #4555
Comments
Doesn't in-process usage of COM query for a private interface to determine its a managed object and let .NET take over from there? In other words you shouldn't be operating on a COM wrapper in the first place if its a managed object, so ComVisible shouldn't matter? I'll try to create a repro and debug to determine whats going on.
|
It's really tricky. We reuse the same data object implementation for drag&drop and clipboard. In the case of drag&drop the object identities somehow kick in. That same behavior does not happen when going through clipboard and using the We never do cross-thread or cross-application calls with this. I don't even think it's reasonably valid to access clipboard from non-UI thread. Our use-case is that we expose an operation akin to "copy email into clipboard". For the operating system (COM IDataObject) we expose the formats to interpret is as file, text, etc. However for copying within the application and pasting into different mailbox we store few additional properties that are exposed only through |
I get this behavior on Desktop Framework as well, i.e. Clipboard.GetDataObject returns a wrapped COM object, not the original WinForms DataObject. I assume OLE does not pass the DataObject on directly but provides a wrapper for isolation. If you had this working on Desktop Framework maybe something else is going wrong, if you can provide a repro or more details I'm happy to take a look, but from a quick test I could not find a difference between Desktop and Core. |
The difference is started happening between .NET Core 3.1 and .NET 5 with the removal of the |
This demonstrates the issue we originally hit: The app multi-targets .NET Core 3.1 and .NET 5. In 3.1 it will pass fine through all the asserts, in 5.0 it will crash. However, it relies on directly calling the |
Thanks, I need some time to look closer at this, there are multiple issues happening, especially since the last assert never worked on Desktop Framework in the first place. Restoring ComVisible is definitely not the right call (beyond TLB issues), it may have looked to you like "it just worked" but that probably was just accidental alignment of multiple bugs. It will break if you start using the interface slightly differently than you are doing (for example note the If you want a quick workaround you could define your own COM visible interface on your data object (in addition to the interfaces you already implement) and cast to that, since you are already doing custom interop anyways. This way you keep relying on the "accidental alignment of bugs" but can keep working like you did before. I'll get back to this once I have a more complete analysis of what happens, and hopefully a suggestion how to improve the situation. (I'd like to get at least |
I intentionally left that out in the example to make it more concise. We do implement these methods as well although I doubt we actually call them directly anywhere. I assume there could be some gotchas with these methods.
That's exactly what we did as a workaround. We needed to do minimal change to avoid breaking the dependent code: [ComVisible(true)]
public interface IComVisibleDataObject
{
object GetData(string format, bool autoConvert);
object GetData(string format);
object GetData(Type format);
bool GetDataPresent(string format, bool autoConvert);
bool GetDataPresent(string format);
bool GetDataPresent(Type format);
string[] GetFormats(bool autoConvert);
string[] GetFormats();
void SetData(string format, bool autoConvert, object data);
void SetData(string format, object data);
void SetData(Type format, object data);
void SetData(object data);
}
class ComVisibleDataObjectWrapper : System.Windows.Forms.IDataObject
{
IComVisibleDataObject innerDataObject;
public ComVisibleDataObjectWrapper(IComVisibleDataObject innerDataObject)
{
this.innerDataObject = innerDataObject;
}
public object GetData(string format, bool autoConvert) => innerDataObject.GetData(format, autoConvert);
public object GetData(string format) => innerDataObject.GetData(format);
public object GetData(Type format) => innerDataObject.GetData(format);
public bool GetDataPresent(string format, bool autoConvert) => innerDataObject.GetDataPresent(format, autoConvert);
public bool GetDataPresent(string format) => innerDataObject.GetDataPresent(format);
public bool GetDataPresent(Type format) => innerDataObject.GetDataPresent(format);
public string[] GetFormats(bool autoConvert) => innerDataObject.GetFormats(autoConvert);
public string[] GetFormats() => innerDataObject.GetFormats();
public void SetData(string format, bool autoConvert, object data) => innerDataObject.SetData(format, autoConvert, data);
public void SetData(string format, object data) => innerDataObject.SetData(format, data);
public void SetData(Type format, object data) => innerDataObject.SetData(format, data);
public void SetData(object data) => innerDataObject.SetData(data);
}
/// <summary>
/// Class implementing drag/drop and clipboard support for virtual files.
/// Also offers an alternate interface to the IDataObject interface.
/// </summary>
public class BaseDataObject :
IComVisibleDataObject,
System.Windows.Forms.IDataObject,
System.Runtime.InteropServices.ComTypes.IDataObject,
IAsyncOperation
{
...
public static System.Windows.Forms.IDataObject GetClipboardDataObject()
{
try
{
System.Runtime.InteropServices.ComTypes.IDataObject dataObject = null;
Win32.OleGetClipboard(ref dataObject);
if (dataObject is IComVisibleDataObject comVisibleDataObject /*&& !Marshal.IsComObject(dataObject)*/)
return new ComVisibleDataObjectWrapper(comVisibleDataObject);
return Clipboard.GetDataObject();
}
catch
{
return Clipboard.GetDataObject();
}
}
}
Thanks! We know we depend on underspecified part of the code but this is something that dates back to the .NET Framework days. Part of the custom data object implementation likely even comes from some CodeProject article or StackOverflow answer so it's likely to affect other people than us. I don't expect it to be very common construct though. The underlying reason for even going to this great length is that we need to expose virtual files where content is generated during the paste operation and not earlier. The standard WinForms |
The sample you provided has a faulty implementation of both the managed IDataObject and the native IDataObject - both are fixable and when done details about the bugs in the sample
if (direction == DATADIR.DATADIR_GET)
{
var formatList = new List<FORMATETC>
{
new FORMATETC
{
cfFormat = (short)DataFormats.GetFormat("Foo").Id,
dwAspect = DVASPECT.DVASPECT_CONTENT,
lindex = -1,
ptd = IntPtr.Zero,
tymed = TYMED.TYMED_HGLOBAL,
}
};
if (SHCreateStdEnumFmtEtc((uint)formatList.Count, formatList.ToArray(), out var enumerator) == S_OK)
return enumerator;
}
Of course I can't tell if that solves your actual problem since it just fixes the sample, there may be other issues preventing you from using WinForms to consume your custom data object. However the initially quoted snippet is not faulty, the second condition will prevent direct usage of managed IDataObject so WinForms itself will never run into a problem from it not being ComVisible. As far as the second assert is concerned, you are actively going out of your way to not use any WinForms code besides the declaration of the interface. If the entire implementation is already coding around WinForms instead of using it, I believe its ok to expect you to also use a custom interface for that external communication. In other words the "workaround" we identified above is most likely also the correct implementation for what you are doing. (Note that the interface you copy/pasted is probably broken even if it currently works for you, see below, and in general the approach you are taking is probably not the best solution, there are better ways to establish a channel through a data object by putting the communication interface into the data object instead of implementing it on the data object.) So if there is no bug with custom IDataObject implementations that leaves us with the breaking change of removing ComVisible on the managed IDataObject. This was an intentional and important change to make sure Desktop Framework and .NET Core can coexist on the same machine without getting in each others way. Besides issues with type libraries discussed in the PR/issue for the change, also note that having
The last point means source compatibility and runtime compatibility at the same time are not achievable, so I believe keeping managed IDataObject not ComVisible is the "best" solution. It allows Desktop Framework applications to keep using the interface globally as they used to, and .NET Core applications can use the interface in-process to feed custom data objects. Anyone already going out of their way to work around the common usage patterns probably has to adapt their code. PS: I tried to keep this response as short as possible, so if you want me to go into details or want to update the sample, you are welcome to keep the discussion going, the above is just my current opinion on the matter |
Firstly, thanks for the detailed write up!
That's intentional even if it trips the WinForms logic. In the actual code we implement logic for
It's something that I tried to explain earlier. We have a set of formats that we want to work for COM and between processes (text, RTF, HTML, file descriptors). That's what is implemented in the COM I am not necessarily saying that's a good way to accomplish the goal of having in-process formats and out-of-process formats (a subset of the in-process ones). It's simply how it was implemented in our application for ±10 years and it broke with the removal of the
Probably a late typo in the editor just before I posted it. Sorry. As for the rest, I understand the reasoning behind the removal of the I am not sure whether there is anything actionable for this issue at the moment. Maybe a documentation mentioning the change about the |
I definitely understand your intentions of why you were doing it, and if you want a low effort solution for maintaining the previous behavior then the custom interface is definitely ok. Just saying that for an actively developed application its possible to implement this in a better way.
We use "private" data formats for this, which I believe is the intended way to pass this kind of information. Using a GUID or a sufficiently custom data format name does the trick, as long as you don't need to pass sensitive data. Also makes it easier if you want to allow limited interaction of multiple instances of your application. On Desktop Framework things were easily done by using the remoting infrastructure (inherit from I'll see if I can find some time to write some examples how we are doing things and what alternatives exist, maybe it gives you some ideas for alternate implementations around your problem.
That actually was recently added: dotnet/docs#22423
I'd definitely welcome a discussion about this. Maybe together with updating the clipboard formats, the .NET Core team driving the removal of BinaryFormatter means another major breaking change is incoming in one of the next releases. |
/cc: @AaronRobinsonMSFT |
FWIW, we're working on making the clipboard API more resilient to address issues like this. |
@filipnavara I want to follow up on your original issue, as promised First here's what we are doing: just store a example for putting a COM reference into a data objectSome notes about the code below:
public static class DataObjectHelper
{
private static Guid IUnknownIID = new Guid("00000000-0000-0000-C000-000000000046");
[DllImport("ole32", ExactSpelling = true, CharSet = CharSet.Unicode, PreserveSig = false)]
public static extern System.Runtime.InteropServices.ComTypes.IBindCtx CreateBindCtx(int reserved = 0);
[DllImport("ole32", ExactSpelling = true, CharSet = CharSet.Unicode, PreserveSig = false)]
public static extern System.Runtime.InteropServices.ComTypes.IMoniker CreateObjrefMoniker([MarshalAs(UnmanagedType.IUnknown)] object punk);
[DllImport("ole32", ExactSpelling = true, CharSet = CharSet.Unicode, PreserveSig = false)]
public static extern System.Runtime.InteropServices.ComTypes.IMoniker MkParseDisplayName(System.Runtime.InteropServices.ComTypes.IBindCtx pbc, [MarshalAs(UnmanagedType.BStr)] string szUserName, out uint pchEaten);
public static void SetObjectReference(this IDataObject obj, string format, object reference)
{
if (reference == null)
throw new ArgumentNullException(nameof(reference));
System.Runtime.InteropServices.ComTypes.IMoniker moniker = null;
System.Runtime.InteropServices.ComTypes.IBindCtx context = null;
try
{
moniker = CreateObjrefMoniker(reference);
context = CreateBindCtx();
moniker.GetDisplayName(context, null, out var id);
obj.SetData(format, new MemoryStream(Encoding.UTF8.GetBytes(id)));
}
finally
{
if (context != null)
Marshal.ReleaseComObject(context);
if (moniker != null)
Marshal.ReleaseComObject(moniker);
}
}
public static object GetObjectReference(this IDataObject obj, string format)
{
var data = obj?.GetData(format) as MemoryStream;
if (data is null)
return null;
return GetObjectReference(Encoding.UTF8.GetString(data.ToArray()));
}
private static object GetObjectReference(string monikerStr)
{
if (string.IsNullOrEmpty(monikerStr))
return null;
System.Runtime.InteropServices.ComTypes.IMoniker moniker = null;
System.Runtime.InteropServices.ComTypes.IBindCtx context = null;
try
{
context = CreateBindCtx();
moniker = MkParseDisplayName(context, monikerStr, out _);
moniker.BindToObject(context, null, ref IUnknownIID, out var obj);
return obj;
}
finally
{
if (moniker != null)
Marshal.ReleaseComObject(moniker);
if (context != null)
Marshal.ReleaseComObject(context);
}
}
} Having given that example of how we do it currently, there is a certain simplicity in your broken solution, especially performance-wise, so I have spent more time to research why it was working for you and if theres a way to make it work reliable. more technical details of what happensSo lets break down what you were doing again:
Note that the second step still returns a COM object and not a managed object, regardless of which framework I tried it on, which indicates that you do not have a reference to your original object, but to "something else". So I researched what
So the result of that research is that The good news is that you can rely on the behavior of
helper code[ComImport]
[Guid("59FFC7AA-BE08-4279-AC5E-C734D55429FD")] // random private GUID
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
interface ICustomDataObjectMarker { }
public static class ClipboardHelper
{
[DllImport("ole32", ExactSpelling = true, CharSet = CharSet.Unicode)]
private static extern int OleGetClipboard(out IntPtr data);
public static IDataObject TryGetCustomDataObject()
{
IntPtr pWrappedDataObject = IntPtr.Zero;
IntPtr pInnerDataObject = IntPtr.Zero;
Guid knownInnerInterface = typeof(ICustomDataObjectMarker).GUID;
try
{
// returns a wrapper object which violates COM rules so we use IntPtr to handle it
if (OleGetClipboard(out pWrappedDataObject) < 0)
return null;
// manually do a QI to an interface not implemented by the wrapper, but implemented by your object
if (Marshal.QueryInterface(pWrappedDataObject, ref knownInnerInterface, out pInnerDataObject) < 0)
return null;
// then let the .NET runtime take over, it unpacks the object and lets you cast to managed interfaces
return Marshal.GetObjectForIUnknown(pInnerDataObject) as IDataObject;
}
finally
{
if (pInnerDataObject != IntPtr.Zero)
Marshal.Release(pInnerDataObject);
if (pWrappedDataObject != IntPtr.Zero)
Marshal.Release(pWrappedDataObject);
}
}
} |
Thanks for the incredibly detailed response. I especially like the clever solution at the end. I guess, my final question is, can the code mentioned in the original post ever succeed? Given the removal of |
Maybe previous versions of Windows didn't return a wrapper object, or previous versions of .NET didn't get confused by it violating COM rules, but in the current state of affairs, yes I believe this is dead and cannot enter that branch. I think thats a good point though, it would be possible to use an internal marker interface as outlined above to bring this optimization back to life, for the stock While it would be technically possible I wouldn't want to use the WinForms IDataObject itself as marker interface, making it ComVisible again, because having it ComVisible would either break runtime compatibility or require breaking source compatibility as explained earlier. @RussKie @JeremyKuhne what do you think? |
JFYI they did, at least all the way to Windows XP. I am too lazy to verify it any further since older versions are unsupported anyway. |
@kant2002 since you're doing work with COM wrappers, you may be interested in this discussion. |
Haha. Thanks for looping @kant2002 in. I wanted to post a link to this issue to the Twitter thread but I got distracted and forgot about it 😅 |
He seemed like the best candidate for nerd sniping :) |
Do I properly understand that initial request is to have fact below working.
And maybe for same-process calls, |
Yes, but from what I gathered at other threads you aren't going to be able to support it, since it means user-defined custom COM objects and IDispatch support are required. It basically means full support for COM interop instead of handrolled inlined internal implementations you're currently doing. Also this thread is a very advanced usecase, usually people don't implement their own COM objects, getting general clipboard and drag'n'drop working with all the WinForms-internal COM objects would probably be much more important in a first iteration. |
@weltkante Where
What you mean user-defined custom COM objects? Regular CCW (COM callable wrappers), or something more, like exposing .NET object as COM object to external processes? Regarding |
Its the default when you make an object
As far as this use case is concerned, exposing a .NET object to COM as CCW and unpacking it to the same managed object in the same process. The CCW is on a user-defined class and there also are user-defined interfaces (in addition to implementing the OLE IDataObject and WinForms IDataObject interfaces). If thats all possible to make working thats great, but having user-defined COM interop has previously been mentioned as an issue for the new ComWrappers model.
Thanks, I see, maybe I got the wrong impression then. That example (WebBrowser scripting) is certainly one of the cases which came up with requiring IDispatch support on arbitrary (ComVisible) managed objects. Don't know how high priority it is to keep the classic WebBrowser control working when there are new ones, but offtopic to DataObject/Clipboard/Drag'n'drop. |
Just to throw the wrench into works, the strategic target we'd want to get to is to re-write the current implementation of the |
Thats fine and matches what AOT needs, but without AOT you can do an incremental implementation and fall back to asking the builtin marshaler for a CCW when getting passed a user-defined object - the problem is that (as far as I understand) AOT doesn't like that very much and prefers everything being custom CCWs/RCWs which is tough when you have to support user-defined ComVisible objects and interfaces (you basically have to reimplement the interop mappings as far as I understand, not impossible but certainly a lot of work nobody wants) of course WinForms can declare those scenarios unsupported under AOT if it has to or you can take a breaking change to the API and no longer allow custom data objects in a future .NET version (so anyone needing them would have to do Windows interop directly using their own custom CCWs instead of going through WinForms). That would probably break some advanced use cases like this issue though, which rely on being able to unwrap a CCW of a custom DataObject back to a managed object so WinForms can use the managed IDataObject implementation instead of the COM interop. |
I think .NET 6 will solve custom IDataObject support if we retreive objects using My understanding that if |
yes, unwrapping is possible, but the question is who creates the CCW of a custom DataObject? It needs to respond to interface queries to things other than IDataObject, including user-defined interfaces. I don't know the new API enough to know if the WinForms ComWrappers can unwrap an RCW obtained from a user-defined ComWrapper CCW. The alternative would be the WinForms ComWrapper to be solely responsible for clipboard CCWs and having to dynamically generate interface tables like the old marshaling layer did. |
I think if we talk about implementations like that
I do not think about cases like in Avalonia. That's make things a bit complicated for ComWrappers 😞 and especially for NativeAOT I have to think a bit more. |
This is the general direction we'll likely be taking the clipboard in the future: https://github.com/RussKie/ClipboardRedux |
Is #6976 having any effect on this issue? |
No, the #6976 does not have effect on the issue, but #7087 is. |
Related to #6269 |
.NET Core Version: 5.0
Have you experienced this same bug with .NET Framework?: No
Problem description:
PR #3388 removed the
ComVisible
attribute fromSystem.Windows.Forms.IDataObject
. That largely breaks the marshaling custom implementation ofIDataObject
through clipboard within the same application. The concern about marshaling of individual calls is not applicable in that case because the runtime could resolve theIDataObject
to the original .NET object implementation.It likely broke this case in
Clipboard.GetDataObject
and now all returned objects are wrapped in the defaultDataObject
which doesn't support all the custom types:winforms/src/System.Windows.Forms/src/System/Windows/Forms/Clipboard.cs
Lines 170 to 173 in 2fd2a58
Expected behavior:
Minimal repro:
TBD
The text was updated successfully, but these errors were encountered: