Finish migrating System.Drawing.Common to ComWrappers

[eerhardt]

This resolves all ILLink warnings in System.Drawing.Common, making it work in a trimmed application.

cc @kant2002

[ghost]

Tagging subscribers to this area: @safern, @tarekgh
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This resolves all ILLink warnings in System.Drawing.Common, making it work in a trimmed application.

cc @kant2002

Author:	eerhardt
Assignees:	-
Labels:	area-System.Drawing
Milestone:	-

[eerhardt]

Thanks for the feedback. I believe I have addressed it all with the latest changes. PTAL.

[eerhardt]

Libraries Test Run release coreclr Linux x64 Debug and Libraries Test Run release coreclr windows x86 Debug failures are all Http Functional tests, which are unrelated to this change.

Build tvOSSimulator x64 Release AllSubsets_Mono failed due to "AppleAppBuilderTask" task failed unexpectedly., which is also unrelated.

I believe this change is ready - any further feedback?

[safern]

do we need to return the buffer in this case?

[eerhardt]

I've been told by @GrabYourPitchforks that when an error occurs, it is usually best to not return buffers to the pool, since they could have corrupt/incomplete data in them.

[jkotas]

It depends on how common you expect the errors can be. We do not bother to return the buffer to the pool for very rare errors, like out-of-memory exceptions.

The error here may not be that rare and it is not perf critical path, so it would be reasonable to return the buffer in finally here. This is similar to Stream.CopyTo where we do return in finally as well.

[eerhardt]

Note - the existing, "built-in COM" code doesn't return the buffer if an exception happens (which my understanding is that a COMException will occur since IStream defines Write as returning void, so the marshalling code will check the HR and throw):

runtime/src/libraries/System.Drawing.Common/src/System/Drawing/Internal/GPStream.cs

Lines 63 to 89 in be80f67

	byte[] buffer = ArrayPool<byte>.Shared.Rent(4096);
	ulong remaining = cb;
	ulong totalWritten = 0;
	ulong totalRead = 0;
	fixed (byte* b = buffer)
	{
	while (remaining > 0)
	{
	uint read = remaining < (ulong)buffer.Length ? (uint)remaining : (uint)buffer.Length;
	Read(b, read, &read);
	remaining -= read;
	totalRead += read;
	if (read == 0)
	{
	break;
	}
	uint written;
	pstm.Write(b, read, &written);
	totalWritten += written;
	}
	}
	ArrayPool<byte>.Shared.Return(buffer);

@jkotas - do you think it is that common for an error to occur here?

[jkotas]

Probably not that common.

It is user supplied stream, so you can certainly constructs cases where the errors are common.

[eerhardt]

I guess my preference is to leave it then, since that is the existing behavior and we haven't heard complaints.

[jkotas]

LGTM

[safern]

LGTM