Skip to content
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

Use ComWrappers for Clipboard #7087

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

internal static partial class Interop
{
internal static partial class Ole32
{
public enum AgileReferenceOptions
{
Default = 0,
DelayedMarshal = 1,
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,31 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Diagnostics.CodeAnalysis;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.ComTypes;

internal static partial class Interop
{
internal static partial class Ole32
{
[DllImport(Libraries.Ole32, ExactSpelling = true)]
private static extern HRESULT OleGetClipboard(out IntPtr data);

[DllImport(Libraries.Ole32, ExactSpelling = true)]
public static extern HRESULT OleGetClipboard(ref IDataObject? data);

public static bool TryOleGetClipboard(out HRESULT result, [NotNullWhen(true)] out IDataObject? data)
{
result = OleGetClipboard(out IntPtr ptr);
if (result == HRESULT.S_OK)
{
data = (IDataObject)WinFormsComWrappers.Instance.GetOrCreateObjectForComInstance(ptr, CreateObjectFlags.Unwrap);
return true;
}

data = null;
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in like this?

Yep.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public static HRESULT OleSetClipboard(IDataObject? pDataObj)
{
if (pDataObj is null)
{
return OleSetClipboard(IntPtr.Zero);
}

return OleSetClipboard(WinFormsComWrappers.Instance.GetComPointer(pDataObj, IID.IDataObject));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime.InteropServices;

internal static partial class Interop
{
internal static partial class Ole32
{
[DllImport(Libraries.Ole32, ExactSpelling = true)]
public static extern HRESULT RoGetAgileReference(
AgileReferenceOptions opts,
ref Guid riid,
IntPtr instance,
out IntPtr agileReference);

public static WinFormsComWrappers.AgileReferenceWrapper RoGetAgileReference(AgileReferenceOptions opts, ref Guid riid, IntPtr instance)
{
RoGetAgileReference(opts, ref riid, instance, out var agileReference);
return new WinFormsComWrappers.AgileReferenceWrapper(agileReference);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime.InteropServices.ComTypes;

internal partial class Interop
{
internal unsafe partial class WinFormsComWrappers
{
internal class AgileDataObjectWrapper : IDataObject
{
private AgileReferenceWrapper _wrappedInstance;

public AgileDataObjectWrapper(AgileReferenceWrapper wrappedInstance)
{
_wrappedInstance = wrappedInstance;
}

private DataObjectWrapper Unwrap()
{
var instance = _wrappedInstance.Resolve(IID.IDataObject);
return new DataObjectWrapper(instance);
}

public void GetData(ref FORMATETC format, out STGMEDIUM medium)
{
using var dataObject = Unwrap();
dataObject.GetData(ref format, out medium);
}

public void GetDataHere(ref FORMATETC format, ref STGMEDIUM medium)
{
using var dataObject = Unwrap();
dataObject.GetDataHere(ref format, ref medium);
}

public int QueryGetData(ref FORMATETC format)
{
using var dataObject = Unwrap();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return dataObject.QueryGetData(ref format);
}

public int GetCanonicalFormatEtc(ref FORMATETC formatIn, out FORMATETC formatOut)
{
using var dataObject = Unwrap();
return dataObject.GetCanonicalFormatEtc(ref formatIn, out formatOut);
}

public void SetData(ref FORMATETC formatIn, ref STGMEDIUM medium, bool release)
{
using var dataObject = Unwrap();
dataObject.SetData(ref formatIn, ref medium, release);
}

public IEnumFORMATETC EnumFormatEtc(DATADIR direction)
{
using var dataObject = Unwrap();
return dataObject.EnumFormatEtc(direction);
}

public int DAdvise(ref FORMATETC pFormatetc, ADVF advf, IAdviseSink adviseSink, out int connection)
{
using var dataObject = Unwrap();
return dataObject.DAdvise(ref pFormatetc, advf, adviseSink, out connection);
}

public void DUnadvise(int connection)
{
using var dataObject = Unwrap();
dataObject.DUnadvise(connection);
}

public int EnumDAdvise(out IEnumSTATDATA? enumAdvise)
{
using var dataObject = Unwrap();
return dataObject.EnumDAdvise(out enumAdvise);
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime.InteropServices;
using System.Windows.Forms;

internal partial class Interop
{
internal unsafe partial class WinFormsComWrappers
{
internal class AgileReferenceWrapper
{
private IntPtr _wrappedInstance;

public AgileReferenceWrapper(IntPtr wrappedInstance)
{
_wrappedInstance = wrappedInstance.OrThrowIfZero();
}

internal IntPtr Instance => _wrappedInstance;

~AgileReferenceWrapper()
{
Marshal.Release(_wrappedInstance);
_wrappedInstance = IntPtr.Zero;
}

public unsafe IntPtr Resolve(Guid iid)
{
IntPtr result = IntPtr.Zero;
((delegate* unmanaged<IntPtr, Guid*, IntPtr*, HRESULT>)(*(*(void***)_wrappedInstance + 3)))
(_wrappedInstance, &iid, &result).ThrowIfFailed();
return result;
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,167 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Runtime.InteropServices;
using System.Runtime.InteropServices.ComTypes;
using System.Windows.Forms;

internal partial class Interop
{
internal unsafe partial class WinFormsComWrappers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the clipboard we check:

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

Perhaps we should be doing the same here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT May 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
internal sealed class DataObjectWrapper : IDataObject, IDisposable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
private IntPtr _wrappedInstance;

public DataObjectWrapper(IntPtr wrappedInstance)
{
_wrappedInstance = wrappedInstance.OrThrowIfZero();
}

internal IntPtr Instance => _wrappedInstance;

~DataObjectWrapper()
{
this.DisposeInternal();
kant2002 marked this conversation as resolved.
Show resolved Hide resolved
}

public void Dispose()
{
DisposeInternal();
GC.SuppressFinalize(this);
}

private void DisposeInternal()
{
Marshal.Release(_wrappedInstance);
_wrappedInstance = IntPtr.Zero;
}

public void GetData(ref FORMATETC format, out STGMEDIUM medium)
{
fixed (FORMATETC* formatPtr = &format)
{
STGMEDIUM_Raw mediumRaw;
((delegate* unmanaged<IntPtr, FORMATETC*, STGMEDIUM_Raw*, HRESULT>)(*(*(void***)_wrappedInstance + 3)))
(_wrappedInstance, formatPtr, &mediumRaw).ThrowIfFailed();
medium = new()
{
pUnkForRelease = mediumRaw.pUnkForRelease == IntPtr.Zero ? null : Marshal.GetObjectForIUnknown(mediumRaw.pUnkForRelease),
RussKie marked this conversation as resolved.
Show resolved Hide resolved
tymed = mediumRaw.tymed,
unionmember = mediumRaw.unionmember,
};
if (mediumRaw.pUnkForRelease != IntPtr.Zero)
kant2002 marked this conversation as resolved.
Show resolved Hide resolved
{
Marshal.Release(mediumRaw.pUnkForRelease);
}
}
}

public void GetDataHere(ref FORMATETC format, ref STGMEDIUM medium)
{
fixed (FORMATETC* formatPtr = &format)
{
STGMEDIUM_Raw mediumRaw = new()
{
pUnkForRelease = medium.pUnkForRelease is null ? IntPtr.Zero : Marshal.GetIUnknownForObject(medium.pUnkForRelease),
RussKie marked this conversation as resolved.
Show resolved Hide resolved
tymed = medium.tymed,
unionmember = medium.unionmember,
};
((delegate* unmanaged<IntPtr, FORMATETC*, STGMEDIUM_Raw*, HRESULT>)(*(*(void***)_wrappedInstance + 4)))
(_wrappedInstance, formatPtr, &mediumRaw).ThrowIfFailed();
// Because we do not know if COM interface implementation change value of mediumRaw.pUnkForRelease during the call,
// unmarshal it again.
medium = new()
{
pUnkForRelease = mediumRaw.pUnkForRelease == IntPtr.Zero ? null : Marshal.GetObjectForIUnknown(mediumRaw.pUnkForRelease),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there was a question here consider a clarifying comment

tymed = mediumRaw.tymed,
unionmember = mediumRaw.unionmember,
};
if (mediumRaw.pUnkForRelease != IntPtr.Zero)
{
Marshal.Release(mediumRaw.pUnkForRelease);
}
}
}

public int QueryGetData(ref FORMATETC format)
{
fixed (FORMATETC* formatPtr = &format)
{
return (int)((delegate* unmanaged<IntPtr, FORMATETC*, HRESULT>)(*(*(void***)_wrappedInstance + 5)))
(_wrappedInstance, formatPtr);
}
}

public int GetCanonicalFormatEtc(ref FORMATETC formatIn, out FORMATETC formatOut)
{
fixed (FORMATETC* formatInPtr = &formatIn)
fixed (FORMATETC* formatOutPtr = &formatOut)
{
return (int)((delegate* unmanaged<IntPtr, FORMATETC*, FORMATETC*, HRESULT>)(*(*(void***)_wrappedInstance + 6)))
(_wrappedInstance, formatInPtr, formatOutPtr);
}
}

public void SetData(ref FORMATETC formatIn, ref STGMEDIUM medium, bool release)
{
fixed (FORMATETC* formatInPtr = &formatIn)
{
STGMEDIUM_Raw mediumRaw = new()
{
pUnkForRelease = medium.pUnkForRelease is null ? IntPtr.Zero : Marshal.GetIUnknownForObject(medium.pUnkForRelease),
tymed = medium.tymed,
unionmember = medium.unionmember,
};
((delegate* unmanaged<IntPtr, FORMATETC*, STGMEDIUM_Raw*, int, HRESULT>)(*(*(void***)_wrappedInstance + 7)))
(_wrappedInstance, formatInPtr, &mediumRaw, release ? 1 : 0).ThrowIfFailed();
medium = new()
{
pUnkForRelease = mediumRaw.pUnkForRelease == IntPtr.Zero ? null : Marshal.GetObjectForIUnknown(mediumRaw.pUnkForRelease),
tymed = mediumRaw.tymed,
unionmember = mediumRaw.unionmember,
};
if (mediumRaw.pUnkForRelease != IntPtr.Zero)
{
Marshal.Release(mediumRaw.pUnkForRelease);
}
}
}

public IEnumFORMATETC EnumFormatEtc(DATADIR direction)
{
IntPtr resultPtr;
((delegate* unmanaged<IntPtr, DATADIR, IntPtr*, HRESULT>)(*(*(void***)_wrappedInstance + 8)))
(_wrappedInstance, direction, &resultPtr).ThrowIfFailed();
return (IEnumFORMATETC)WinFormsComWrappers.Instance.GetOrCreateObjectForComInstance(resultPtr, CreateObjectFlags.Unwrap);
}

public int DAdvise(ref FORMATETC pFormatetc, ADVF advf, IAdviseSink adviseSink, out int connection)
{
fixed (FORMATETC* formatPtr = &pFormatetc)
fixed (int* connectionPtr = &connection)
{
var adviseSinkPtr = WinFormsComWrappers.Instance.GetOrCreateComInterfaceForObject(adviseSink, CreateComInterfaceFlags.None);
return (int)((delegate* unmanaged<IntPtr, FORMATETC*, ADVF, IntPtr, int*, HRESULT>)(*(*(void***)_wrappedInstance + 9)))
(_wrappedInstance, formatPtr, advf, adviseSinkPtr, connectionPtr);
}
}

public void DUnadvise(int connection)
{
((delegate* unmanaged<IntPtr, int, HRESULT>)(*(*(void***)_wrappedInstance + 10)))
(_wrappedInstance, connection).ThrowIfFailed();
}

public int EnumDAdvise(out IEnumSTATDATA? enumAdvise)
{
IntPtr enumAdvisePtr;
var result = ((delegate* unmanaged<IntPtr, IntPtr*, HRESULT>)(*(*(void***)_wrappedInstance + 11)))
(_wrappedInstance, &enumAdvisePtr);
enumAdvise = result.Succeeded() ? null : (IEnumSTATDATA)Marshal.GetObjectForIUnknown(enumAdvisePtr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release is responsibility of RCW finalizer.

return (int)result;
}
}
}
}
Loading