From 603746c33f4a964e0ac50e9795d97ba6a0db4483 Mon Sep 17 00:00:00 2001
From: Rolf Bjarne Kvinge <rolf@xamarin.com>
Date: Thu, 11 Mar 2021 07:36:45 +0100
Subject: [PATCH] [CoreText] Clean up CTRunDelegate[Operations] implementation.
 (#10830)

* Make CTRunDelegateCallbacks a struct instead of class, since it's being
  passed as a parameter to a P/Invoke. Passing an instance of a class to a
  P/Invoke is just weird.

* Make CTRunDelegate subclass NativeObject. This allows us to remove a big
  chunk of boiler-plate code.

* Make CTRunDelegateOperations.handle a private field. Handle fields should be
  private.

* Put the CTRunDelegateCallbacks struct with the delegates passed to native
  code on the CTRunDelegateOperations instance instead of the CTRunDelegate
  instance, since the callbacks are tied to the CTRunDelegateOperations instance.

  Fixes this fatal error on CoreCLR, because otherwise the Deallocate callback could be called after the CTRunDelegate instance was freed, in which case the Deallocate delegate could also be freed:

  > A callback was made on a garbage collected delegate of type 'Xamarin.Mac!CoreText.CTRunDelegateDeallocateCallback::Invoke'.
---
 src/CoreText/CTRunDelegate.cs | 84 +++++++++++++----------------------
 1 file changed, 32 insertions(+), 52 deletions(-)

diff --git a/src/CoreText/CTRunDelegate.cs b/src/CoreText/CTRunDelegate.cs
index 7b562a32ec..c72d5fea03 100644
--- a/src/CoreText/CTRunDelegate.cs
+++ b/src/CoreText/CTRunDelegate.cs
@@ -39,7 +39,7 @@ namespace CoreText {
 	delegate nfloat CTRunDelegateGetCallback (IntPtr refCon);
 
 	[StructLayout (LayoutKind.Sequential)]
-	class CTRunDelegateCallbacks {
+	struct CTRunDelegateCallbacks {
 		public /* CFIndex */ nint version;
 		public CTRunDelegateDeallocateCallback dealloc;
 		public CTRunDelegateGetCallback getAscent;
@@ -49,8 +49,14 @@ namespace CoreText {
 #endregion
 
 	public class CTRunDelegateOperations : IDisposable {
+		// This instance is kept alive using a GCHandle until the Deallocate callback has been called,
+		// which is called when the corresponding CTRunDelegate is freed (retainCount reaches 0).
+		// This even means that the GCHandle is not freed if Dispose is called manually.
+		GCHandle handle;
 
-		internal GCHandle handle;
+		public IntPtr Handle {
+			get { return GCHandle.ToIntPtr (handle); }
+		}
 
 		protected CTRunDelegateOperations ()
 		{
@@ -104,16 +110,19 @@ namespace CoreText {
 		}
 #endif
 
+		CTRunDelegateCallbacks? callbacks; // prevent GC since they are called from native code
 		internal CTRunDelegateCallbacks GetCallbacks ()
 		{
-			var callbacks = new CTRunDelegateCallbacks () {
-				version = 1, // kCTRunDelegateVersion1
-				dealloc = Deallocate,
-				getAscent = GetAscent,
-				getDescent = GetDescent,
-				getWidth = GetWidth,
-			};
-			return callbacks;
+			if (!callbacks.HasValue) {
+				callbacks = new CTRunDelegateCallbacks () {
+					version = 1, // kCTRunDelegateVersion1
+					dealloc = Deallocate,
+					getAscent = GetAscent,
+					getDescent = GetDescent,
+					getWidth = GetWidth,
+				};
+			}
+			return callbacks.Value;
 		}
 
 		[MonoPInvokeCallback (typeof (CTRunDelegateDeallocateCallback))]
@@ -165,57 +174,28 @@ namespace CoreText {
 		}
 	}
 
-	public class CTRunDelegate : INativeObject, IDisposable {
-		internal IntPtr handle;
-
+	public class CTRunDelegate : NativeObject, IDisposable {
 		internal CTRunDelegate (IntPtr handle, bool owns)
+			: base (handle, owns)
 		{
-			if (handle == IntPtr.Zero)
-				throw new ArgumentNullException ("handle");
-
-			this.handle = handle;
-			if (!owns)
-				CFObject.CFRetain (handle);
-		}
-		
-		public IntPtr Handle {
-			get {return handle;}
-		}
-
-		~CTRunDelegate ()
-		{
-			Dispose (false);
-		}
-		
-		public void Dispose ()
-		{
-			Dispose (true);
-			GC.SuppressFinalize (this);
-		}
-
-		protected virtual void Dispose (bool disposing)
-		{
-			if (handle != IntPtr.Zero){
-				CFObject.CFRelease (handle);
-				handle = IntPtr.Zero;
-			}
 		}
 
 #region RunDelegate Creation
 		[DllImport (Constants.CoreTextLibrary)]
-		static extern IntPtr CTRunDelegateCreate (CTRunDelegateCallbacks callbacks, IntPtr refCon);
+		static extern IntPtr CTRunDelegateCreate (ref CTRunDelegateCallbacks callbacks, IntPtr refCon);
 
-		CTRunDelegateCallbacks callbacks; // prevent GC since they are called from native code
-
-		public CTRunDelegate (CTRunDelegateOperations operations)
+		static IntPtr Create (CTRunDelegateOperations operations)
 		{
 			if (operations == null)
-				throw ConstructorError.ArgumentNull (this, "operations");
+				throw new ArgumentNullException (nameof (operations));
 
-			callbacks = operations.GetCallbacks ();
-			handle = CTRunDelegateCreate (callbacks, GCHandle.ToIntPtr (operations.handle));
-			if (handle == IntPtr.Zero)
-				throw ConstructorError.Unknown (this);
+			CTRunDelegateCallbacks callbacks = operations.GetCallbacks ();
+			return CTRunDelegateCreate (ref callbacks, operations.Handle);
+		}
+
+		public CTRunDelegate (CTRunDelegateOperations operations)
+			: base (Create (operations), true)
+		{
 		}
 #endregion
 
@@ -225,7 +205,7 @@ namespace CoreText {
 
 		public CTRunDelegateOperations Operations {
 			get {
-				return CTRunDelegateOperations.GetOperations (CTRunDelegateGetRefCon (handle));
+				return CTRunDelegateOperations.GetOperations (CTRunDelegateGetRefCon (Handle));
 			}
 		}
 #endregion