Skip to content

Only Unwrap If Given IDataObject #12738

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.

Sign up for GitHub

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

Merged
lonitra merged 1 commit into from
Jan 10, 2025
Merged

Only Unwrap If Given IDataObject #12738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion src/System.Private.Windows.Core/src/Windows/Win32/System/Com/ComHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ internal static bool SupportsInterface<T>(object? @object) where T : unmanaged,
/// <summary>
/// Attempts to unwrap a ComWrapper CCW as a particular managed object.
/// </summary>
private static bool TryUnwrapComWrapperCCW<TWrapper>(
public static bool TryUnwrapComWrapperCCW<TWrapper>(
IUnknown* unknown,
[NotNullWhen(true)] out TWrapper? @interface) where TWrapper : class
{
Expand Down
60 changes: 15 additions & 45 deletions src/System.Windows.Forms/src/System/Windows/Forms/OLE/Clipboard.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,13 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes,
ArgumentOutOfRangeException.ThrowIfNegative(retryTimes);
ArgumentOutOfRangeException.ThrowIfNegative(retryDelay);

// Always wrap the data in our DataObject since we know how to retrieve our DataObject from the proxy OleGetClipboard returns.
DataObject wrappedData = data is DataObject { IsWrappedForClipboard: true } alreadyWrapped
? alreadyWrapped
: new DataObject(data) { IsWrappedForClipboard = true };
using var dataObject = ComHelpers.GetComScope<Com.IDataObject>(wrappedData);
// Always wrap the data if not already a DataObject. Mark whether the data is an IDataObject so we unwrap it properly on retrieval.
DataObject dataObject = data as DataObject ?? new DataObject(data) { IsOriginalNotIDataObject = data is not IDataObject };
using var iDataObject = ComHelpers.GetComScope<Com.IDataObject>(dataObject);

HRESULT hr;
int retry = retryTimes;
while ((hr = PInvoke.OleSetClipboard(dataObject)).Failed)
while ((hr = PInvoke.OleSetClipboard(iDataObject)).Failed)
{
if (--retry < 0)
{
Expand Down Expand Up @@ -111,52 +109,24 @@ public static unsafe void SetDataObject(object data, bool copy, int retryTimes,

// OleGetClipboard always returns a proxy. The proxy forwards all IDataObject method calls to the real data object,
// without giving out the real data object. If the data placed on the clipboard is not one of our CCWs or the clipboard
// has been flushed, marshal will create a wrapper around the proxy for us to use. However, if the data placed on
// has been flushed, a wrapper around the proxy for us to use will be given. However, if the data placed on
// the clipboard is one of our own and the clipboard has not been flushed, we need to retrieve the real data object
// pointer in order to retrieve the original managed object via ComWrappers. To do this, we must query for an
// interface that is not known to the proxy e.g. IComCallableWrapper. If we are able to query for IComCallableWrapper
// it means that the real data object is one of our CCWs and we've retrieved it successfully,
// pointer in order to retrieve the original managed object via ComWrappers if an IDataObject was set on the clipboard.
// To do this, we must query for an interface that is not known to the proxy e.g. IComCallableWrapper.
// If we are able to query for IComCallableWrapper it means that the real data object is one of our CCWs and we've retrieved it successfully,
// otherwise it is not ours and we will use the wrapped proxy.
IUnknown* target = default;
var realDataObject = proxyDataObject.TryQuery<IComCallableWrapper>(out hr);
if (hr.Succeeded)
{
target = realDataObject.AsUnknown;
}
else
{
target = proxyDataObject.AsUnknown;
}

if (!ComHelpers.TryGetObjectForIUnknown(target, out object? managedDataObject))
{
target->Release();
return null;
}

if (managedDataObject is not Com.IDataObject.Interface dataObject)
{
// We always wrap data set on the Clipboard in a DataObject, so if we do not have
// a IDataObject.Interface this means built-in com support is turned off and
// we have a proxy where there is no way to retrieve the original data object
// pointer from it likely because either the clipboard was flushed or the data on the
// clipboard is from another process. We need to mimic built-in com behavior and wrap the proxy ourselves.
// DataObject will ref count proxyDataObject properly to take ownership.
return new DataObject(proxyDataObject.Value);
}

if (dataObject is DataObject { IsWrappedForClipboard: true } wrappedData)
if (hr.Succeeded
&& ComHelpers.TryUnwrapComWrapperCCW(realDataObject.AsUnknown, out DataObject? dataObject)
&& !dataObject.IsOriginalNotIDataObject)
{
// There is a DataObject on the clipboard that we placed there. If the real data object
// implements IDataObject, we want to unwrap it and return it. Otherwise return
// the DataObject as is.
return wrappedData.TryUnwrapInnerIDataObject();
// An IDataObject was given to us to place on the clipboard. We want to unwrap and return it instead of a proxy.
return dataObject.TryUnwrapInnerIDataObject();
}

// We did not place the data on the clipboard. Fall back to old behavior.
return dataObject is IDataObject ido && !Marshal.IsComObject(dataObject)
? ido
: new DataObject(dataObject);
// Original data given wasn't an IDataObject, give the proxy value back.
return new DataObject(proxyDataObject.Value);
}

/// <summary>
Expand Down
6 changes: 3 additions & 3 deletions src/System.Windows.Forms/src/System/Windows/Forms/OLE/DataObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ public DataObject(object data)
internal DataObject(string format, bool autoConvert, object data) : this() => SetData(format, autoConvert, data);

/// <summary>
/// Flags that the original data was wrapped for clipboard purposes.
/// Flags that the original data was not a user passed <see cref="IDataObject"/>.
/// </summary>
internal bool IsWrappedForClipboard { get; init; }
internal bool IsOriginalNotIDataObject { get; init; }

/// <summary>
/// Returns the inner data that the <see cref="DataObject"/> was created with if the original data implemented
Expand All @@ -95,7 +95,7 @@ public DataObject(object data)
/// </summary>
internal IDataObject TryUnwrapInnerIDataObject()
{
Debug.Assert(IsWrappedForClipboard, "This method should only be used for clipboard purposes.");
Debug.Assert(!IsOriginalNotIDataObject, "This method should only be used for clipboard purposes.");
return _innerData.OriginalIDataObject is { } original ? original : this;
}

Expand Down
66 changes: 66 additions & 0 deletions src/System.Windows.Forms/tests/ComDisabledTests/ClipboardComTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@

#nullable enable

using System.Runtime.InteropServices;
using static System.Windows.Forms.TestUtilities.DataObjectTestHelpers;
using ComTypes = System.Runtime.InteropServices.ComTypes;

namespace System.Windows.Forms.Tests;

Expand Down Expand Up @@ -50,4 +52,68 @@ public void Clipboard_SetDataObject_WithJson_ReturnsExpected(bool copy)
returnedDataObject.TryGetData("testData", out SimpleTestData deserialized).Should().BeTrue();
deserialized.Should().BeEquivalentTo(testData);
}

[WinFormsFact]
public void Clipboard_GetSet_IDataObject_RoundTrip_ReturnsExpected()
{
CustomDataObject realDataObject = new();
Clipboard.SetDataObject(realDataObject);

IDataObject clipboardDataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
clipboardDataObject.Should().BeSameAs(realDataObject);
clipboardDataObject.GetDataPresent("Foo").Should().BeTrue();
clipboardDataObject.GetData("Foo").Should().Be("Bar");
}

[WinFormsFact]
public void Clipboard_SetDataObject_DerivedDataObject_ReturnsExpected()
{
DerivedDataObject derived = new();
Clipboard.SetDataObject(derived);
Clipboard.GetDataObject().Should().BeSameAs(derived);
}

private class DerivedDataObject : DataObject { }

private class CustomDataObject : IDataObject, ComTypes.IDataObject
{
[DllImport("shell32.dll")]
public static extern int SHCreateStdEnumFmtEtc(uint cfmt, ComTypes.FORMATETC[] afmt, out ComTypes.IEnumFORMATETC ppenumFormatEtc);

int ComTypes.IDataObject.DAdvise(ref ComTypes.FORMATETC pFormatetc, ComTypes.ADVF advf, ComTypes.IAdviseSink adviseSink, out int connection) => throw new NotImplementedException();
void ComTypes.IDataObject.DUnadvise(int connection) => throw new NotImplementedException();
int ComTypes.IDataObject.EnumDAdvise(out ComTypes.IEnumSTATDATA enumAdvise) => throw new NotImplementedException();
ComTypes.IEnumFORMATETC ComTypes.IDataObject.EnumFormatEtc(ComTypes.DATADIR direction)
{
if (direction == ComTypes.DATADIR.DATADIR_GET)
{
// Create enumerator and return it
ComTypes.IEnumFORMATETC enumerator;
if (SHCreateStdEnumFmtEtc(0, [], out enumerator) == 0)
{
return enumerator;
}
}

throw new NotImplementedException();
}

int ComTypes.IDataObject.GetCanonicalFormatEtc(ref ComTypes.FORMATETC formatIn, out ComTypes.FORMATETC formatOut) => throw new NotImplementedException();
object IDataObject.GetData(string format, bool autoConvert) => format == "Foo" ? "Bar" : null!;
object IDataObject.GetData(string format) => format == "Foo" ? "Bar" : null!;
object IDataObject.GetData(Type format) => null!;
void ComTypes.IDataObject.GetData(ref ComTypes.FORMATETC format, out ComTypes.STGMEDIUM medium) => throw new NotImplementedException();
void ComTypes.IDataObject.GetDataHere(ref ComTypes.FORMATETC format, ref ComTypes.STGMEDIUM medium) => throw new NotImplementedException();
bool IDataObject.GetDataPresent(string format, bool autoConvert) => format == "Foo";
bool IDataObject.GetDataPresent(string format) => format == "Foo";
bool IDataObject.GetDataPresent(Type format) => false;
string[] IDataObject.GetFormats(bool autoConvert) => ["Foo"];
string[] IDataObject.GetFormats() => ["Foo"];
int ComTypes.IDataObject.QueryGetData(ref ComTypes.FORMATETC format) => throw new NotImplementedException();
void IDataObject.SetData(string format, bool autoConvert, object? data) => throw new NotImplementedException();
void IDataObject.SetData(string format, object? data) => throw new NotImplementedException();
void IDataObject.SetData(Type format, object? data) => throw new NotImplementedException();
void IDataObject.SetData(object? data) => throw new NotImplementedException();
void ComTypes.IDataObject.SetData(ref ComTypes.FORMATETC formatIn, ref ComTypes.STGMEDIUM medium, bool release) => throw new NotImplementedException();
}
}
120 changes: 108 additions & 12 deletions src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/ClipboardTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,19 +574,11 @@ public unsafe void Clipboard_GetClipboard_ReturnsProxy()
}

[WinFormsFact]
public void Clipboard_Set_DoesNotWrapTwice()
public void Clipboard_SetDataObject_DerivedDataObject_ReturnsExpected()
{
string realDataObject = string.Empty;
Clipboard.SetDataObject(realDataObject);

IDataObject? clipboardDataObject = Clipboard.GetDataObject();
var dataObject = clipboardDataObject.Should().BeOfType<DataObject>().Subject;
dataObject.IsWrappedForClipboard.Should().BeTrue();

Clipboard.SetDataObject(clipboardDataObject!);
IDataObject? clipboardDataObject2 = Clipboard.GetDataObject();
clipboardDataObject2.Should().NotBeNull();
clipboardDataObject2.Should().BeSameAs(clipboardDataObject);
DerivedDataObject derived = new();
Clipboard.SetDataObject(derived);
Clipboard.GetDataObject().Should().BeSameAs(derived);
}

[WinFormsFact]
Expand Down Expand Up @@ -664,6 +656,16 @@ public unsafe void Clipboard_RawClipboard_SetClipboardData_ReturnsExpected()

DataObject dataObject = Clipboard.GetDataObject().Should().BeOfType<DataObject>().Subject;
dataObject.GetData(DataFormats.Text).Should().Be(testString);

Clipboard.ContainsText().Should().BeTrue();
Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();

Clipboard.GetText().Should().Be(testString);
Clipboard.GetText(TextDataFormat.Text).Should().Be(testString);
Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(testString);

Clipboard.GetData("System.String").Should().BeNull();
}

[WinFormsFact]
Expand Down Expand Up @@ -981,6 +983,16 @@ public void Clipboard_SetDataObject_WithJson_ReturnsExpected(bool copy)
ITypedDataObject returnedDataObject = Clipboard.GetDataObject().Should().BeAssignableTo<ITypedDataObject>().Subject;
returnedDataObject.TryGetData("testDataFormat", out SimpleTestData deserialized).Should().BeTrue();
deserialized.Should().BeEquivalentTo(testData);
// We don't expose JsonData<T> in legacy API
var legacyResult = Clipboard.GetData("testDataFormat");
Comment thread
Tanya-Solyanik marked this conversation as resolved.
if (copy)
{
legacyResult.Should().BeOfType<MemoryStream>();
}
else
{
legacyResult.Should().BeNull();
}
}

[WinFormsTheory]
Expand Down Expand Up @@ -1170,4 +1182,88 @@ public void Clipboard_SetDataAsJson_NullData_Throws()
Action clipboardSet = () => Clipboard.SetDataAsJson<string>("format", null!);
clipboardSet.Should().Throw<ArgumentNullException>();
}

[WinFormsFact]
public void Clipboard_SetData_Text_Format_AllUpper()
{
// The fact that casing on input matters is likely incorrect, but behavior has been this way.
Clipboard.SetData("TEXT", "Hello, World!");
Clipboard.ContainsText().Should().BeTrue();
Clipboard.ContainsData("TEXT").Should().BeTrue();
Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();

IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
string[] formats = dataObject.GetFormats();
formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);

formats = dataObject.GetFormats(autoConvert: false);
formats.Should().BeEquivalentTo(["Text"]);

// CLIPBRD_E_BAD_DATA returned when trying to get clipboard data.
Clipboard.GetText().Should().BeEmpty();
Clipboard.GetText(TextDataFormat.Text).Should().BeEmpty();
Clipboard.GetText(TextDataFormat.UnicodeText).Should().BeEmpty();

Clipboard.GetData("System.String").Should().BeNull();
Clipboard.GetData("TEXT").Should().BeNull();
}

[WinFormsFact]
public void Clipboard_SetData_Text_Format_CanonicalCase()
{
string expected = "Hello, World!";
Clipboard.SetData("Text", expected);
Clipboard.ContainsText().Should().BeTrue();
Clipboard.ContainsData("TEXT").Should().BeTrue();
Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();

IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
string[] formats = dataObject.GetFormats();
formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);

formats = dataObject.GetFormats(autoConvert: false);
formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);

Clipboard.GetText().Should().Be(expected);
Clipboard.GetText(TextDataFormat.Text).Should().Be(expected);
Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(expected);

Clipboard.GetData("System.String").Should().Be(expected);

// Case sensitivity matters so we end up reading stream/object from HGLOBAL instead of string.
MemoryStream stream = Clipboard.GetData("TEXT").Should().BeOfType<MemoryStream>().Subject;
byte[] array = stream.ToArray();
array.Should().BeEquivalentTo("Hello, World!\0"u8.ToArray());
}

[WinFormsFact]
public void Clipboard_SetDataObject_Text()
{
string expected = "Hello, World!";
Clipboard.SetDataObject(expected);
Clipboard.ContainsText().Should().BeTrue();
Clipboard.ContainsData("TEXT").Should().BeTrue();
Clipboard.ContainsData(DataFormats.Text).Should().BeTrue();
Clipboard.ContainsData(DataFormats.UnicodeText).Should().BeTrue();

IDataObject dataObject = Clipboard.GetDataObject().Should().BeAssignableTo<IDataObject>().Subject;
string[] formats = dataObject.GetFormats();
formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);

formats = dataObject.GetFormats(autoConvert: false);
formats.Should().BeEquivalentTo(["System.String", "UnicodeText", "Text"]);

Clipboard.GetText().Should().Be(expected);
Clipboard.GetText(TextDataFormat.Text).Should().Be(expected);
Clipboard.GetText(TextDataFormat.UnicodeText).Should().Be(expected);

Clipboard.GetData("System.String").Should().Be(expected);

// Case sensitivity matters so we end up reading stream/object from HGLOBAL instead of string.
MemoryStream stream = Clipboard.GetData("TEXT").Should().BeOfType<MemoryStream>().Subject;
byte[] array = stream.ToArray();
array.Should().BeEquivalentTo("Hello, World!\0"u8.ToArray());
}
}
4 changes: 0 additions & 4 deletions src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/DataObjectTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3032,10 +3032,6 @@ public void DataObject_SetDataAsJson_NonRestrictedFormat_JsonSerialized(string f

// We don't expose JsonData<T> in public legacy API
data.GetData(format).Should().BeNull();

// For the clipboard, we don't expose JsonData<T> either for in proc scenarios.
Clipboard.SetDataObject(data, copy: false);
Comment thread
JeremyKuhne marked this conversation as resolved.
Clipboard.GetData(format).Should().BeNull();
}

[WinFormsFact]
Expand Down