Merge pull request #23262 from dotnet/post-dev15.5-contrib

Mop up merge of post-dev15.5-contrib to master

src/Compilers/Core/Portable/InternalUtilities/FatalError.cs

@@ -143,22 +143,6 @@ public static bool Report(Exception exception)
         [DebuggerHidden]
         public static bool ReportWithoutCrash(Exception exception)
         {
-            // There have been cases where a new, unthrown exception has been passed to this method.
-            // In these cases the exception won't have a stack trace, which isn't very helpful. We
-            // throw and catch the exception here as that will result in a stack trace that is
-            // better than nothing.
-            if (exception.StackTrace == null)
-            {
-                try
-                {
-                    throw exception;
-                }
-                catch
-                {
-                    // Empty; we just need the exception to have a stack trace.
-                }
-            }
-
             Report(exception, s_nonFatalHandler);
             return true;
         }

src/VisualStudio/CSharp/Test/Watson/WatsonTests.cs

@@ -22,7 +22,7 @@ public void TestRegularException()
             catch (Exception exception)
             {
                 var mockFault = new MockFault();
-                mockFault.SetExtraParameters(exception);
+                mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
                 // there should be no extra bucket info
                 // for regular exception
@@ -48,7 +48,7 @@ public void TestRegularWithInnerexception()
             catch (Exception exception)
             {
                 var mockFault = new MockFault();
-                mockFault.SetExtraParameters(exception);
+                mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
                 Assert.Equal(exception.InnerException.GetParameterString(), mockFault.Map[8]);
             }
@@ -60,7 +60,7 @@ public void TestRemoteInvocationException()
             var mockFault = new MockFault();
 
             var exception = new RemoteInvocationException("test", "remoteCallstack", "remoteErrorCode");
-            mockFault.SetExtraParameters(exception);
+            mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
             Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
         }
@@ -71,7 +71,7 @@ public void TestRemoteInvocationExceptionNull()
             var mockFault = new MockFault();
 
             var exception = new RemoteInvocationException(message: null, remoteStack: null, remoteCode: null);
-            mockFault.SetExtraParameters(exception);
+            mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
             Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
         }
@@ -87,7 +87,7 @@ public void TestAggregateException()
             catch (Exception exception)
             {
                 var mockFault = new MockFault();
-                mockFault.SetExtraParameters(exception);
+                mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
                 // there should be no extra bucket info
                 // for regular exception
@@ -113,7 +113,7 @@ public void TestAggregateWithInnerexception()
             catch (Exception exception)
             {
                 var mockFault = new MockFault();
-                mockFault.SetExtraParameters(exception);
+                mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
                 Assert.Equal(exception.GetParameterString(), mockFault.Map[8]);
             }
@@ -151,7 +151,7 @@ public void TestAggregateWithMultipleInnerexceptions()
             catch (AggregateException exception)
             {
                 var mockFault = new MockFault();
-                mockFault.SetExtraParameters(exception);
+                mockFault.SetExtraParameters(exception, emptyCallstack: false);
 
                 var flatten = exception.Flatten();
                 Assert.Equal(flatten.CalculateHash(), mockFault.Map[7]);
@@ -159,6 +159,17 @@ public void TestAggregateWithMultipleInnerexceptions()
             }
         }
 
+        [Fact]
+        public void TestEmptyCallstack()
+        {
+            var mockFault = new MockFault();
+
+            var exception = new Exception("not thrown");
+            mockFault.SetExtraParameters(exception, emptyCallstack: true);
+
+            Assert.NotNull(mockFault.Map[9]);
+        }
+
         public class MockFault : IFaultUtility
         {
             public readonly Dictionary<int, string> Map = new Dictionary<int, string>();

src/VisualStudio/Core/Def/Implementation/Remote/RemoteHostClientServiceFactory.RemoteHostClientService.cs

@@ -267,7 +267,7 @@ private void OnStatusChanged(object sender, bool started)
 
                     // s_lastRemoteClientTask info should be saved in the dump
                     // report NFW when connection is closed unless it is proper shutdown
-                    FatalError.ReportWithoutCrash(new Exception("Connection to remote host closed"));
+                    WatsonReporter.Report(new Exception("Connection to remote host closed"));
 
                     // use info bar to show warning to users
                     var infoBarUIs = new List<InfoBarUI>();

src/VisualStudio/Core/Def/Implementation/Watson/WatsonExtensions.cs

@@ -10,16 +10,24 @@ namespace Microsoft.CodeAnalysis.ErrorReporting
     internal static class WatsonExtensions
     {
         // NFW API let caller to customize watson report to make them better bucketed by
-        // putting custom string in reserved slots. normally those 2 slots will be empty.
+        // putting custom string in reserved slots. normally those 3 slots will be empty.
         private const int Reserved1 = 8;
         private const int Reserved2 = 7;
+        private const int Reserved3 = 9;
 
         /// <summary>
         /// This sets extra watson bucket parameters to make bucketting better
         /// in non fatal watson report
         /// </summary>
-        public static void SetExtraParameters(this IFaultUtility fault, Exception exception)
+        public static void SetExtraParameters(this IFaultUtility fault, Exception exception, bool emptyCallstack)
         {
+            if (emptyCallstack)
+            {
+                // if exception we got started with empty callstack, put runtime
+                // callstack in one of reserved slot for better bucketting
+                fault.SetBucketParameter(Reserved3, Environment.StackTrace);
+            }
+
             switch (exception)
             {
                 case RemoteInvocationException remote:
@@ -80,5 +88,66 @@ public static string GetParameterString(this Exception exception)
                     return $"{exception.GetType().ToString()} {(exception.StackTrace ?? exception.ToString())}";
             }
         }
+
+        /// <summary>
+        /// hold onto last issue we reported. we use hash
+        /// since exception callstack could be quite big
+        /// </summary>
+        private static int s_lastExceptionReported;
+
+#if DEBUG
+        /// <summary>
+        /// in debug, we also hold onto reported string to make debugging easier
+        /// </summary>
+        private static string s_lastExceptionReportedDebug;
+#endif
+
+        public static bool ShouldReport(this Exception exception)
+        {
+            // this is a poor man's check whether we are called for same issues repeatedly
+            // one of problem of NFW compared to FW is that since we don't crash at an issue, same issue
+            // might happen repeatedly. especially in short amount of time. reporting all those issues
+            // are meaningless so we do cheap check to see we just reported same issue and
+            // bail out.
+            // I think this should be actually done by PostFault itself and I talked to them about it.
+            // but until they do something, we will do very simple throuttle ourselves.
+            var currentExceptionString = exception.GetParameterString();
+            var currentException = currentExceptionString.GetHashCode();
+            if (s_lastExceptionReported == currentException)
+            {
+                return false;
+            }
+
+#if DEBUG
+            s_lastExceptionReportedDebug = currentExceptionString;
+#endif
+
+            s_lastExceptionReported = currentException;
+
+            return true;
+        }
+
+        public static bool SetCallstackIfEmpty(this Exception exception)
+        {
+            // There have been cases where a new, unthrown exception has been passed to this method.
+            // In these cases the exception won't have a stack trace, which isn't very helpful. We
+            // throw and catch the exception here as that will result in a stack trace that is
+            // better than nothing.
+            if (exception.StackTrace != null)
+            {
+                return false;
+            }
+
+            try
+            {
+                throw exception;
+            }
+            catch
+            {
+                // Empty; we just need the exception to have a stack trace.
+            }
+
+            return true;
+        }
     }
 }

src/VisualStudio/Core/Def/Implementation/Watson/WatsonReporter.cs

@@ -21,19 +21,6 @@ internal static class WatsonDisabled
 
     internal static class WatsonReporter
     {
-        /// <summary>
-        /// hold onto last issue we reported. we use hash
-        /// since exception callstack could be quite big
-        /// </summary>
-        private static int s_lastExceptionReported;
-
-#if DEBUG
-        /// <summary>
-        /// in debug, we also hold onto reported string to make debugging easier
-        /// </summary>
-        private static string s_lastExceptionReportedDebug;
-#endif
-
         /// <summary>
         /// The default callback to pass to <see cref="TelemetrySessionExtensions.PostFault(TelemetrySession, string, string, Exception, Func{IFaultUtility, int})"/>.
         /// Returning "0" signals that we should send data to Watson; any other value will cancel the Watson report.
@@ -69,31 +56,14 @@ public static void Report(string description, Exception exception)
         /// CAB.</param>
         public static void Report(string description, Exception exception, Func<IFaultUtility, int> callback)
         {
-            if (!WatsonDisabled.s_reportWatson)
-            {
-                return;
-            }
+            var emptyCallstack = exception.SetCallstackIfEmpty();
 
-            // this is a poor man's check whether we are called for same issues repeatedly
-            // one of problem of NFW compared to FW is that since we don't crash at an issue, same issue
-            // might happen repeatedly. especially in short amount of time. reporting all those issues
-            // are meaningless so we do cheap check to see we just reported same issue and
-            // bail out.
-            // I think this should be actually done by PostFault itself and I talked to them about it.
-            // but until they do something, we will do very simple throuttle ourselves.
-            var currentExceptionString = exception.GetParameterString();
-            var currentException = currentExceptionString.GetHashCode();
-            if (s_lastExceptionReported == currentException)
+            if (!WatsonDisabled.s_reportWatson ||
+                !exception.ShouldReport())
             {
                 return;
             }
 
-#if DEBUG
-            s_lastExceptionReportedDebug = currentExceptionString;
-#endif
-
-            s_lastExceptionReported = currentException;
-
             TelemetryService.DefaultSession.PostFault(
                 eventName: FunctionId.NonFatalWatson.GetEventName(),
                 description: description,
@@ -104,7 +74,7 @@ public static void Report(string description, Exception exception, Func<IFaultUt
                     arg.AddProcessDump(System.Diagnostics.Process.GetCurrentProcess().Id);
 
                     // add extra bucket parameters to bucket better in NFW
-                    arg.SetExtraParameters(exception);
+                    arg.SetExtraParameters(exception, emptyCallstack);
 
                     return callback(arg);
                 });

src/Workspaces/Remote/ServiceHub/Telemetry/WatsonReporter.cs

@@ -59,6 +59,8 @@ public static void Report(string description, Exception exception)
         /// CAB.</param>
         public static void Report(string description, Exception exception, Func<IFaultUtility, int> callback)
         {
+            var emptyCallstack = exception.SetCallstackIfEmpty();
+
             // if given exception is non recoverable exception,
             // crash instead of NFW
             if (IsNonRecoverableException(exception))
@@ -66,6 +68,11 @@ public static void Report(string description, Exception exception, Func<IFaultUt
                 CodeAnalysis.FailFast.OnFatalException(exception);
             }
 
+            if (!exception.ShouldReport())
+            {
+                return;
+            }
+
             SessionOpt?.PostFault(
                 eventName: FunctionId.NonFatalWatson.GetEventName(),
                 description: description,
@@ -76,7 +83,7 @@ public static void Report(string description, Exception exception, Func<IFaultUt
                     arg.AddProcessDump(System.Diagnostics.Process.GetCurrentProcess().Id);
 
                     // add extra bucket parameters to bucket better in NFW
-                    arg.SetExtraParameters(exception);
+                    arg.SetExtraParameters(exception, emptyCallstack);
 
                     return callback(arg);
                 });