Merge pull request #7733 from mono/exception-handling-better

Handle exceptions coming in from glib and objc in a better way

3 files changed, 62 insertions, 30 deletions

diff --git a/main/src/addins/MacPlatform/MacPlatform.cs b/main/src/addins/MacPlatform/MacPlatform.cs
index f860333842..0b9a6d51d5 100644
--- a/main/src/addins/MacPlatform/MacPlatform.cs
+++ b/main/src/addins/MacPlatform/MacPlatform.cs
@@ -249,28 +249,26 @@ namespace MonoDevelop.MacIntegration
 
 			var nsException = ObjCRuntime.Runtime.GetNSObject<NSException> (exceptionPtr);
 			try {
-				throw new MarshalledObjCException (nsException, Environment.StackTrace);
+				throw new MarshalledObjCException (nsException);
 			} catch (MarshalledObjCException e) {
+				LoggingService.LogInternalError ("Unhandled ObjC Exception", e);
 				// Is there a way to figure out if it's going to crash us? Maybe check MarshalObjectiveCExceptionMode and MarshalManagedExceptionMode?
-				LoggingService.LogInternalError ("Unhandled ObjC exception", e);
 			}
 
+			// Invoke the default xamarin.mac one, so if it bubbles up an exception, the caller receives it.
 			oldHandler?.Invoke (exceptionPtr);
 		}
 
 		sealed class MarshalledObjCException : ObjCException
 		{
-			public MarshalledObjCException (NSException exception, string stacktrace) : base (exception)
+			public MarshalledObjCException (NSException exception) : base (exception)
 			{
-				StackTrace = stacktrace;
-			}
-
-			public override string StackTrace { get; }
+				const BindingFlags flags = BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.SetField;
 
-			public override string ToString ()
-			{
-				// Matches normal exception format:
-				return GetType () + ": " + Message + Environment.NewLine + StackTrace;
+				var trace = new [] { new StackTrace (true), };
+				//// Otherwise exception stacktrace is not gathered.
+				typeof (Exception)
+					.InvokeMember ("captured_traces", flags, null, this, new object [] { trace });
 			}
 		}
 
diff --git a/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide/GLibLogging.cs b/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide/GLibLogging.cs
index 5f6ef79aff..21a4593f6d 100644
--- a/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide/GLibLogging.cs
+++ b/main/src/core/MonoDevelop.Ide/MonoDevelop.Ide/GLibLogging.cs
@@ -33,6 +33,7 @@ using System.Runtime.InteropServices;
 using System.Collections;

 using System.Runtime.ExceptionServices;
 using System.Runtime.CompilerServices;
+using System.Diagnostics;
 
 namespace MonoDevelop.Ide.Gui
 {
@@ -255,7 +256,7 @@ namespace MonoDevelop.Ide.Gui
 				LoggingService.LogError (message, e);
 			}
 
-			string msg = string.Format ("{0}-{1}: {2}", logDomain, logLevel, message);
+			string msg = string.Format ("{0}-{1}: {2}\n{3}", logDomain, logLevel, message, GetStacktraceIfNeeded (logLevel));
 
 			switch (logLevel) {
 			case LogLevelFlags.Debug:
@@ -271,13 +272,12 @@ namespace MonoDevelop.Ide.Gui
 			case LogLevelFlags.Critical:
 			default:
 				try {
-					// Otherwise exception info is not gathered.
-					throw new CriticalGtkException (msg, Environment.StackTrace);
+					throw new CriticalGtkException (msg);
 				} catch (CriticalGtkException e) {
 					if (logLevel.HasFlag (LogLevelFlags.FlagFatal))
-						LoggingService.LogFatalError ("Fatal GLib error", e);
+						LoggingService.LogFatalError ($"Fatal {logDomain} error", e);
 					else
-						LoggingService.LogInternalError ("Critical GLib error", e);
+						LoggingService.LogInternalError ($"Critical {logDomain} error", e);
 				}
 				break;
 			}
@@ -287,19 +287,42 @@ namespace MonoDevelop.Ide.Gui
 				LoggingService.LogError ("Disabling glib logging for the rest of the session");
 		}
 
+		static bool IsMono = Type.GetType ("Mono.Runtime") != null;
+
+		static string GetStacktraceIfNeeded (LogLevelFlags flags)
+		{
+			// If it's an Error or a Critical message, we're going to add the stacktrace via the logged exception property,
+			// we don't need to append it to the message in the log. But we are only doing so on Mono, nowhere else.
+			if (IsMono && flags.HasFlag (LogLevelFlags.Error | LogLevelFlags.Critical))
+				return string.Empty;
+
+			return "Stack trace: \n" + new StackTrace (1, true);
+		}
+
 		sealed class CriticalGtkException : Exception
 		{
-			public CriticalGtkException (string message, string stacktrace) : base (message)
+			readonly StackTrace trace = new StackTrace (1, true);
+
+			public CriticalGtkException (string message) : base (message)
 			{
-				StackTrace = stacktrace;
+				const System.Reflection.BindingFlags flags =
+					 System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.SetField;
+
+				// HACK: We need to somehow inject our stacktrace, and this is the only way we can
+				// This is not to transform every glib error into a managed exception
+				if (IsMono) {
+					typeof (Exception)
+						.GetField ("captured_traces", flags)
+						?.SetValue (this, new StackTrace [] { trace });
+				}
 			}
 
-			public override string StackTrace { get; }
+			public override string StackTrace => trace.ToString ();
 
 			public override string ToString ()
 			{
 				// Matches normal exception format:
-				return GetType() + ": " + Message + Environment.NewLine + StackTrace;
+				return GetType () + ": " + Message + Environment.NewLine + trace;
 			}
 		}
 	}
diff --git a/main/tests/Ide.Tests/MonoDevelop.Ide.Gui/GLibLoggingTests.cs b/main/tests/Ide.Tests/MonoDevelop.Ide.Gui/GLibLoggingTests.cs
index 61ac439b6a..b75242e373 100644
--- a/main/tests/Ide.Tests/MonoDevelop.Ide.Gui/GLibLoggingTests.cs
+++ b/main/tests/Ide.Tests/MonoDevelop.Ide.Gui/GLibLoggingTests.cs
@@ -27,6 +27,7 @@ using System;
 using System.Collections.Generic;
 using System.Linq;
 using MonoDevelop.Core;
+using MonoDevelop.Core.Logging;
 using MonoDevelop.Core.LogReporting;
 using NUnit.Framework;
 using UnitTests;
@@ -65,22 +66,33 @@ namespace MonoDevelop.Ide.Gui
 		}
 
 		[Test]
-		public void CriticalErrorsExceptionsHaveFullStacktracesInLog ()
+		public void GLibLoggingHaveFullStacktracesInLog ()
 		{
 			var old = GLibLogging.Enabled;
-			var logger = new CapturingLogger {
-				EnabledLevel = Core.Logging.EnabledLoggingLevel.Error,
-			};
 
 			try {
-				LoggingService.AddLogger (logger);
 				GLibLogging.Enabled = true;
 
-				GLib.Log.Write ("Gtk", GLib.LogLevelFlags.Critical, "{0}", "critical should be captured");
-				var (_, message) = logger.LogMessages.Single (x => x.Level == Core.Logging.LogLevel.Error);
-				AssertGLibStackTrace (message);
+				var flagsToCheck = new [] {
+					(GLib.LogLevelFlags.Critical, LogLevel.Error),
+					(GLib.LogLevelFlags.Debug, LogLevel.Debug),
+					(GLib.LogLevelFlags.Info, LogLevel.Info),
+					//(GLib.LogLevelFlags.Error, LogLevel.Fatal),
+				};
+
+				foreach (var (glibLevel, coreLevel) in flagsToCheck) {
+					var logger = new CapturingLogger ();
+
+					LoggingService.AddLogger (logger);
+					try {
+						GLib.Log.Write ("Gtk", glibLevel, "{0}: should be captured", glibLevel);
+						var (_, message) = logger.LogMessages.Single (x => x.Level == coreLevel);
+						AssertGLibStackTrace (message);
+					} finally {
+						LoggingService.RemoveLogger (logger.Name);
+					}
+				}
 			} finally {
-				LoggingService.RemoveLogger (logger.Name);
 				GLibLogging.Enabled = old;
 			}
 		}
@@ -91,7 +103,6 @@ namespace MonoDevelop.Ide.Gui
 			Assert.That (stacktrace, Contains.Substring ("at GLib.Log.g_logv"));
 			Assert.That (stacktrace, Contains.Substring ("at MonoDevelop.Ide.Gui.GLibLogging.LoggerMethod"));
 			Assert.That (stacktrace, Contains.Substring ("at MonoDevelop.Ide.Gui.GLibLoggingTests"));
-
 		}
 	}
 }