diff --git a/src/Foundation/NSObject2.cs b/src/Foundation/NSObject2.cs index 0e5cd8df76f..728f1c3457b 100644 --- a/src/Foundation/NSObject2.cs +++ b/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 { diff --git a/tests/xharness/Jenkins/Reports/HtmlReportWriter.cs b/tests/xharness/Jenkins/Reports/HtmlReportWriter.cs index 8642dd241de..75ea031ac90 100644 --- a/tests/xharness/Jenkins/Reports/HtmlReportWriter.cs +++ b/tests/xharness/Jenkins/Reports/HtmlReportWriter.cs @@ -424,11 +424,13 @@ public void Write (IList allTasks, StreamWriter writer) } if (logs.Count () > 0) { - foreach (var log in logs) { - if (!(log is IFileBackedLog fileLog)) { - continue; - } - + var query = logs. + OfType (). + 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 allTasks, StreamWriter writer) fails = data_tuple.Item2; } } - if (fails.Count > 0) { + if (!hasListedErrors && fails.Count > 0) { writer.WriteLine ("
"); foreach (var fail in fails) writer.WriteLine ("{0}
", fail.AsHtml ()); writer.WriteLine ("
"); + hasListedErrors = true; } if (!string.IsNullOrEmpty (summary)) writer.WriteLine ("{0}
", summary); @@ -532,20 +535,22 @@ public void Write (IList allTasks, StreamWriter writer) errors = (HashSet) data.Item2; } } - if (errors.Count > 0) { + if (!hasListedErrors && errors.Count > 0) { writer.WriteLine ("
"); foreach (var error in errors) writer.WriteLine ("{0}
", error.AsHtml ()); writer.WriteLine ("
"); + hasListedErrors = true; } } catch (Exception ex) { writer.WriteLine ("Could not parse log file: {0}: {1}
", 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) { @@ -553,8 +558,9 @@ public void Write (IList allTasks, StreamWriter writer) } } 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 ($"Could not parse {log.Description}: {ex.Message?.AsHtml ()}
");