[Foundation] Fix NSObjectData leak.

src/Foundation/NSObject2.cs

@@ -19,6 +19,9 @@
 // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
 //
+#define LOGGING
+#define CONSISTENCY_CHECKS
+
 using System.Diagnostics.CodeAnalysis;
 using System.Reflection;
 using System.Collections.Generic;
@@ -76,6 +79,11 @@ struct NSObjectData {
 	}
 
 	class NSObjectDataHandle : CriticalHandle {
+#if LOGGING
+		static int counter;
+		public int ID = Interlocked.Increment (ref counter);
+#endif
+		bool invalidated;
 		public NSObjectDataHandle ()
 			: base (IntPtr.Zero)
 		{
@@ -84,18 +92,46 @@ public NSObjectDataHandle ()
 			}
 		}
 
+		public NSObjectDataHandle (IntPtr handle)
+			: base (handle)
+		{
+#if LOGGING
+			Runtime.NSLog ($"NSObjectDataHandle (0x{handle:x}): wrapped existing pointer");
+#endif
+		}
+
+		public void Invalidate ()
+		{
+			invalidated = true;
+#if LOGGING
+			Runtime.NSLog ($"NSObjectDataHandle.Invalidate (): invalidated 0x{handle:x}");
+#endif
+		}
+
 		public unsafe NSObjectData* Data {
 			get => (NSObjectData*) handle;
 		}
 
 		public override bool IsInvalid {
-			get => handle != IntPtr.Zero;
+			get => handle == IntPtr.Zero;
 		}
 
 		protected override bool ReleaseHandle ()
 		{
-			unsafe {
-				NativeMemory.Free ((void*) handle);
+			if (handle != IntPtr.Zero) {
+				if (invalidated) {
+					// nothing to do here.
+#if LOGGING
+					Runtime.NSLog ($"NSObjectDataHandle.ReleaseHandle (): NOT released 0x{handle:x}; not owned");
+#endif
+				} else {
+					unsafe {
+						NativeMemory.Free ((void*) handle);
+					}
+#if LOGGING
+					Runtime.NSLog ($"NSObjectDataHandle.ReleaseHandle (): released 0x{handle:x}");
+#endif
+				}
 			}
 			handle = IntPtr.Zero;
 			return true;
@@ -149,13 +185,31 @@ unsafe NativeHandle handle {
 
 		internal unsafe NSObjectData* GetData ()
 		{
-			return AllocateData ().Data;
+			var rv = AllocateData ().Data;
+
+			if (rv is null) {
+#if LOGGING
+				Runtime.NSLog ($"{GetType ().Name}.GetData (): id={objectId} no handle\n{Environment.StackTrace}");
+				throw new ObjectDisposedException ($"{GetType ().Name}.GetData (): id={objectId} No data???");
+#else
+				// Throwing an exception here is better than returning a null pointer, because that will crash the process when the pointer is dereferenced
+				// (and none of the callers can do anything useful with a null pointer anyway).
+				throw new ObjectDisposedException ($"This object (of type {GetType ().Name}) does not have a data pointer anymore, possibly because of a race condition. Please file a bug at https://github.com/dotnet/macios/issues.");
+#endif
+			}
+
+			return rv;
 		}
 
+#if LOGGING
+		static int objectIdCounter;
+		int objectId;
+#endif
 		unsafe NSObjectDataHandle AllocateData ()
 		{
-			if (data_handle is not null)
-				return data_handle;
+			var dh = data_handle;
+			if (dh is not null)
+				return dh;
 
 			var data = new NSObjectDataHandle ();
 			var previousValue = Interlocked.CompareExchange (ref data_handle, data, null);
@@ -165,6 +219,10 @@ unsafe NSObjectDataHandle AllocateData ()
 				return previousValue;
 			}
 
+#if LOGGING
+			objectId = Interlocked.Increment (ref objectIdCounter);
+			Runtime.NSLog ($"{GetType ().Name}.AllocateData () id={objectId} allocated 0x{((IntPtr) data.Data):x}");
+#endif
 			if (!Runtime.IsCoreCLR) // This condition (and the assignment to __handle_for_mono if applicable) is trimmed away by the linker.
 				__data_for_mono = data.Data;
 
@@ -998,10 +1056,67 @@ protected virtual void Dispose (bool disposing)
 					ReleaseManagedRef ();
 				} else {
 					NSObject_Disposer.Add (this);
+					RecreateDataHandle ();
 				}
 			}
 		}
 
+#nullable enable
+		void RecreateDataHandle ()
+		{
+			// OK, this code is _weird_.
+			// We need to delay the deletion of the native memory pointed to by data_handle until
+			// after this instance has been collected. A CriticalHandle seems to fit this purpose like glove, until
+			// you realize that a CriticalHandle is only kept alive until the parent object _becomes finalizable_,
+			// not _is collected_, which is very different - in other words, resurrected objects don't keep CriticalHandles
+			// they contain alive. This is a problem because every single managed NSObject instance is resurrected, and we
+			// need the native memory to stay alive after resurrection.
+			//
+			// So this solution depends on a few bits:
+			// * At this point, this instance may have become finalizable, but the native memory shouldn't have been freed yet.
+			// * The original NSObjectDataHandle (aka CriticalHandle) will be collected in this/upcoming GC cycle, and can't
+			//   be trusted to keep the native memory alive anymore.
+			// * So we just create a new one, pointing to the same native memory, and replace the original NSObjectDataHandle (aka
+			//   CriticalHandle) with it
+			// * This works, because since this instance has become / will become resurrected, it's not finalizable anymore,
+			//   and it will keep the new NSObjectDataHandle instance (and the native memory it points to) alive.
+			// * Now if this instance is deemed finalizable, and then resurrected *again*, bad things will likely happen. This
+			//   is a bit more unlikely though, because we don't re-register the finalizer for execution, so unless somebody
+			//   else does that, it's quite unlikely this instance will become resurrected a second time.
+			var previous_data = data_handle;
+			if (previous_data is null) {
+				var msg = $"This object (of type {GetType ().Name}) does not have an existing data pointer, possibly because of a race condition. Please file a bug at https://github.com/dotnet/macios/issues.";
+#if CONSISTENCY_CHECKS
+				throw new InvalidOperationException (msg);
+#else
+				Runtime.NSLog (msg);
+				return;
+#endif
+			}
+
+			unsafe {
+				data_handle = new NSObjectDataHandle ((IntPtr) previous_data.Data);
+			}
+
+			if (previous_data.IsInvalid) {
+				var msg = $"This object (of type {GetType ().Name}) does not have valid data pointer, possibly because of a race condition. Please file a bug at https://github.com/dotnet/macios/issues.";
+#if CONSISTENCY_CHECKS
+				throw new InvalidOperationException (msg);
+#else
+				Runtime.NSLog (msg);
+				return;
+#endif
+			}
+
+#if LOGGING
+			Runtime.NSLog ($"{GetType ().Name}: id={objectId} previous value invalidated and disposed. Current flags: {flags}");
+#endif
+			previous_data.Invalidate ();
+			// Don't dispose previous_data, because another thread might be referencing it, and trying to access its pointer - which is still valid.
+			// The GC will dispose of previous_data when its not accessible anymore.
+		}
+#nullable disable
+
 		[Register ("__NSObject_Disposer")]
 		[Preserve (AllMembers = true)]
 		internal class NSObject_Disposer : NSObject {

tests/xharness/Jenkins/Reports/HtmlReportWriter.cs

@@ -424,11 +424,13 @@ public void Write (IList<ITestTask> allTasks, StreamWriter writer)
 						}
 
 						if (logs.Count () > 0) {
-							foreach (var log in logs) {
-								if (!(log is IFileBackedLog fileLog)) {
-									continue;
-								}
-
+							var query = logs.
+								OfType<IFileBackedLog> ().
+								OrderBy (v => v.Description).
+								ThenBy (v => v.FullPath);
+							var hasListedErrors = false;
+							foreach (var fileLog in query) {
+								var log = fileLog;
 								log.Flush ();
 								var exists = File.Exists (fileLog.FullPath);
 								string log_type = GetMimeMapping (fileLog.FullPath);
@@ -495,11 +497,12 @@ public void Write (IList<ITestTask> allTasks, StreamWriter writer)
 												fails = data_tuple.Item2;
 											}
 										}
-										if (fails.Count > 0) {
+										if (!hasListedErrors && fails.Count > 0) {
 											writer.WriteLine ("<div style='padding-left: 15px;'>");
 											foreach (var fail in fails)
 												writer.WriteLine ("{0} <br />", fail.AsHtml ());
 											writer.WriteLine ("</div>");
+											hasListedErrors = true;
 										}
 										if (!string.IsNullOrEmpty (summary))
 											writer.WriteLine ("<span style='padding-left: 15px;'>{0}</span><br />", summary);
@@ -532,29 +535,32 @@ public void Write (IList<ITestTask> allTasks, StreamWriter writer)
 												errors = (HashSet<string>) data.Item2;
 											}
 										}
-										if (errors.Count > 0) {
+										if (!hasListedErrors && errors.Count > 0) {
 											writer.WriteLine ("<div style='padding-left: 15px;'>");
 											foreach (var error in errors)
 												writer.WriteLine ("{0} <br />", error.AsHtml ());
 											writer.WriteLine ("</div>");
+											hasListedErrors = true;
 										}
 									} catch (Exception ex) {
 										writer.WriteLine ("<span style='padding-left: 15px;'>Could not parse log file: {0}: {1}</span><br />", ex.Message?.AsHtml (), ex.StackTrace?.AsHtml ());
 									}
 								} else if (log.Description == LogType.NUnitResult.ToString () || log.Description == LogType.XmlLog.ToString ()) {
 									try {
-										if (File.Exists (fileLog.FullPath) && new FileInfo (fileLog.FullPath).Length > 0) {
+										if (!hasListedErrors && File.Exists (fileLog.FullPath) && new FileInfo (fileLog.FullPath).Length > 0) {
 											if (resultParser.IsValidXml (fileLog.FullPath, out var jargon)) {
 												resultParser.GenerateTestReport (writer, fileLog.FullPath, jargon);
+												hasListedErrors = true;
 											}
 										}
 									} catch (Exception ex) {
 										writer.WriteLine ($"<span style='padding-left: 15px;'>Could not parse {log.Description}: {ex.Message?.AsHtml ()} : {ex.StackTrace?.AsHtml ()}</span><br />");
 									}
 								} else if (log.Description == LogType.TrxLog.ToString ()) {
 									try {
-										if (resultParser.IsValidXml (fileLog.FullPath, out var jargon)) {
+										if (!hasListedErrors && resultParser.IsValidXml (fileLog.FullPath, out var jargon)) {
 											resultParser.GenerateTestReport (writer, fileLog.FullPath, jargon);
+											hasListedErrors = true;
 										}
 									} catch (Exception ex) {
 										writer.WriteLine ($"<span style='padding-left: 15px;'>Could not parse {log.Description}: {ex.Message?.AsHtml ()}</span><br />");