Blame fixes

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/BlameCollector.cs

@@ -214,29 +214,32 @@ private void CollectDumpAndAbortTesthost()
                 EqtTrace.Verbose("Inactivity timer is already disposed.");
             }
 
+            if (this.collectProcessDumpOnTrigger)
+            {
+                // Detach procdump from the testhost process to prevent testhost process from crashing
+                // if/when we try to kill the existing proc dump process.
+                // And also prevent collecting dump on exit of the process.
+                this.processDumpUtility.DetachFromTargetProcess(this.testHostProcessId);
+            }
+
+            var hangDumpSuccess = false;
             try
             {
                 Action<string> logWarning = m => this.logger.LogWarning(this.context.SessionDataCollectionContext, m);
                 var dumpDirectory = this.GetDumpDirectory();
                 this.processDumpUtility.StartHangBasedProcessDump(this.testHostProcessId, dumpDirectory, this.processFullDumpEnabled, this.targetFramework, logWarning);
+                hangDumpSuccess = true;
             }
             catch (Exception ex)
             {
                 this.logger.LogError(this.context.SessionDataCollectionContext, $"Blame: Creating hang dump failed with error.", ex);
             }
 
-            if (this.collectProcessDumpOnTrigger)
-            {
-                // Detach procdump from the testhost process to prevent testhost process from crashing
-                // if/when we try to kill the existing proc dump process.
-                this.processDumpUtility.DetachFromTargetProcess(this.testHostProcessId);
-            }
-
             if (this.uploadDumpFiles)
             {
                 try
                 {
-                    var dumpFiles = this.processDumpUtility.GetDumpFiles();
+                    var dumpFiles = this.processDumpUtility.GetDumpFiles(true, /* if we killed it by hang dumper, we already have our dump, otherwise it might have crashed, and we want all dumps */ !hangDumpSuccess);
                     foreach (var dumpFile in dumpFiles)
                     {
                         try
@@ -451,8 +454,9 @@ private void SessionEndedHandler(object sender, SessionEndEventArgs args)
             {
                 // If the last test crashes, it will not invoke a test case end and therefore
                 // In case of crash testStartCount will be greater than testEndCount and we need to write the sequence
-                // And send the attachment
-                if (this.testStartCount > this.testEndCount)
+                // And send the attachment. This won't indicate failure if there are 0 tests in the assembly, or when it fails in setup.
+                var processCrashedWhenRunningTests = this.testStartCount > this.testEndCount;
+                if (processCrashedWhenRunningTests)
                 {
                     var filepath = Path.Combine(this.GetTempDirectory(), Constants.AttachmentFileName + "_" + this.attachmentGuid);
 
@@ -472,7 +476,7 @@ private void SessionEndedHandler(object sender, SessionEndEventArgs args)
                 {
                     try
                     {
-                        var dumpFiles = this.processDumpUtility.GetDumpFiles(warnOnNoDumpFiles: this.collectDumpAlways);
+                        var dumpFiles = this.processDumpUtility.GetDumpFiles(warnOnNoDumpFiles: this.collectDumpAlways, processCrashedWhenRunningTests);
                         foreach (var dumpFile in dumpFiles)
                         {
                             if (!string.IsNullOrEmpty(dumpFile))

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/CrashDumperFactory.cs

@@ -6,6 +6,7 @@ namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
     using System;
     using System.Runtime.InteropServices;
     using Microsoft.VisualStudio.TestPlatform.ObjectModel;
+    using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers;
     using NuGet.Frameworks;
 
     internal class CrashDumperFactory : ICrashDumperFactory
@@ -34,7 +35,7 @@ public ICrashDumper Create(string targetFramework)
                 if (!isNet50OrNewer)
                 {
                     EqtTrace.Info($"CrashDumperFactory: This is Windows on {targetFramework} which is not net5.0 or newer, returning ProcDumpCrashDumper that uses ProcDump utility.");
-                    return new ProcDumpCrashDumper();
+                    return new ProcDumpDumper();
                 }
 
                 // On net5.0 we don't have the capability to crash dump on exit, which is useful in rare cases
@@ -48,17 +49,17 @@ public ICrashDumper Create(string targetFramework)
                 if (forceUsingProcdump)
                 {
                     EqtTrace.Info($"CrashDumperFactory: This is Windows on {targetFramework}. Forcing the use of ProcDumpCrashDumper that uses ProcDump utility, via VSTEST_DUMP_FORCEPROCDUMP={procdumpOverride}.");
-                    return new ProcDumpCrashDumper();
+                    return new ProcDumpDumper();
                 }
 
                 EqtTrace.Info($"CrashDumperFactory: This is Windows on {targetFramework}, returning the .NETClient dumper which uses env variables to collect crashdumps of testhost and any child process.");
-                return new NetClientCrashDumper();
+                return new NetClientCrashDumper(new FileHelper());
             }
 
             if (isNet50OrNewer)
             {
                 EqtTrace.Info($"CrashDumperFactory: This is {RuntimeInformation.OSDescription} on {targetFramework} .NETClient dumper which uses env variables to collect crashdumps of testhost and any child process.");
-                return new NetClientCrashDumper();
+                return new NetClientCrashDumper(new FileHelper());
             }
 
             throw new PlatformNotSupportedException($"Unsupported operating system: {RuntimeInformation.OSDescription}, and framework: {targetFramework}.");

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/HangDumperFactory.cs

@@ -20,6 +20,9 @@ public IHangDumper Create(string targetFramework)
             }
 
             EqtTrace.Info($"HangDumperFactory: Creating dumper for {RuntimeInformation.OSDescription} with target framework {targetFramework}.");
+            var procdumpOverride = Environment.GetEnvironmentVariable("VSTEST_DUMP_FORCEPROCDUMP")?.Trim();
+            var netdumpOverride = Environment.GetEnvironmentVariable("VSTEST_DUMP_FORCENETDUMP")?.Trim();
+            EqtTrace.Verbose($"HangDumperFactory: Overrides for dumpers: VSTEST_DUMP_FORCEPROCDUMP={procdumpOverride};VSTEST_DUMP_FORCENETDUMP={netdumpOverride}");
 
             var tfm = NuGetFramework.Parse(targetFramework);
 
@@ -31,6 +34,30 @@ public IHangDumper Create(string targetFramework)
 
             if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
             {
+                // On some system the interop dumper will thrown AccessViolationException, add an option to force procdump.
+                var forceUsingProcdump = !string.IsNullOrWhiteSpace(procdumpOverride) && procdumpOverride != "0";
+                if (forceUsingProcdump)
+                {
+                    EqtTrace.Info($"HangDumperFactory: This is Windows on  Forcing the use of ProcDumpHangDumper that uses ProcDump utility, via VSTEST_DUMP_FORCEPROCDUMP={procdumpOverride}.");
+                    return new ProcDumpDumper();
+                }
+
+                // On some system the interop dumper will thrown AccessViolationException, add an option to force procdump.
+                var forceUsingNetdump = !string.IsNullOrWhiteSpace(netdumpOverride) && netdumpOverride != "0";
+                if (forceUsingNetdump)
+                {
+                    var isLessThan50 = tfm.Framework == ".NETCoreApp" && tfm.Version < Version.Parse("5.0.0.0");
+                    if (!isLessThan50)
+                    {
+                        EqtTrace.Info($"HangDumperFactory: This is Windows on {tfm.Framework} {tfm.Version}, VSTEST_DUMP_FORCENETDUMP={netdumpOverride} is active, forcing use of .NetClientHangDumper");
+                        return new NetClientHangDumper();
+                    }
+                    else
+                    {
+                        EqtTrace.Info($"HangDumperFactory: This is Windows on {tfm.Framework} {tfm.Version}, VSTEST_DUMP_FORCENETDUMP={netdumpOverride} is active, but only applies to .NET 5.0 and newer. Falling back to default hang dumper.");
+                    }
+                }
+
                 EqtTrace.Info($"HangDumperFactory: This is Windows, returning the default WindowsHangDumper that P/Invokes MiniDumpWriteDump.");
                 return new WindowsHangDumper(this.LogWarning);
             }

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ICrashDumper.cs

@@ -3,12 +3,16 @@
 
 namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
 {
+    using System.Collections.Generic;
+
     public interface ICrashDumper
     {
         void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType, bool collectAlways);
 
         void WaitForDumpToFinish();
 
         void DetachFromTargetProcess(int processId);
+
+        IEnumerable<string> GetDumpFiles(bool processCrashed);
     }
 }

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcDumpArgsBuilder.cs

@@ -22,11 +22,8 @@ public interface IProcDumpArgsBuilder
         /// <param name="isFullDump">
         /// Is full dump enabled
         /// </param>
-        /// <param name="collectAlways">
-        /// Collects the dump on process exit even when there is no exception
-        /// </param>
         /// <returns>Arguments</returns>
-        string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump, bool collectAlways);
+        string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump);
 
         /// <summary>
         /// Arguments for procdump.exe for getting a dump in case of a testhost hang

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/Interfaces/IProcessDumpUtility.cs

@@ -12,10 +12,11 @@ public interface IProcessDumpUtility
         /// Get generated dump files
         /// </summary>
         /// <param name="warnOnNoDumpFiles">Writes warning when no dump file is found.</param>
+        /// <param name="processCrashed">Process might have crashed. If true it crashed for sure. If false we don't know.</param>
         /// <returns>
         /// Path of dump file
         /// </returns>
-        IEnumerable<string> GetDumpFiles(bool warnOnNoDumpFiles = true);
+        IEnumerable<string> GetDumpFiles(bool warnOnNoDumpFiles, bool processCrashed);
 
         /// <summary>
         /// Launch proc dump process

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/NetClientCrashDumper.cs

@@ -3,19 +3,40 @@
 
 namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
 {
+    using System;
+    using System.Collections.Generic;
+    using System.IO;
+    using Microsoft.VisualStudio.TestPlatform.Utilities.Helpers.Interfaces;
+
     internal class NetClientCrashDumper : ICrashDumper
     {
+        private string outputDirectory;
+        private IFileHelper fileHelper;
+
+        public NetClientCrashDumper(IFileHelper fileHelper)
+        {
+            this.fileHelper = fileHelper;
+        }
+
         public void AttachToTargetProcess(int processId, string outputDirectory, DumpTypeOption dumpType, bool collectAlways)
         {
             // we don't need to do anything directly here, we setup the env variables
             // in the dumper configuration, including the path
+            this.outputDirectory = outputDirectory;
         }
 
         public void DetachFromTargetProcess(int processId)
         {
             // here we might consider renaming the files to have timestamp
         }
 
+        public IEnumerable<string> GetDumpFiles(bool processCrashed)
+        {
+            return this.fileHelper.DirectoryExists(this.outputDirectory)
+               ? this.fileHelper.GetFiles(this.outputDirectory, "*_crashdump*.dmp", SearchOption.AllDirectories)
+               : Array.Empty<string>();
+        }
+
         public void WaitForDumpToFinish()
         {
         }

src/Microsoft.TestPlatform.Extensions.BlameDataCollector/ProcDumpArgsBuilder.cs

@@ -3,21 +3,34 @@
 
 namespace Microsoft.TestPlatform.Extensions.BlameDataCollector
 {
+    using System;
     using System.Collections.Generic;
     using System.Text;
 
     public class ProcDumpArgsBuilder : IProcDumpArgsBuilder
     {
         /// <inheritdoc />
-        public string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump, bool collectAlways)
+        public string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnumerable<string> procDumpExceptionsList, bool isFullDump)
         {
             // -accepteula: Auto accept end-user license agreement
+            //
             // -e: Write a dump when the process encounters an unhandled exception. Include the 1 to create dump on first chance exceptions.
+            // We use -e 1 to make sure we are able to catch StackOverflow and AccessViolationException exceptions.
             // -g: Run as a native debugger in a managed process (no interop).
+            // We use -g to be able to intercept  StackOverflow and AccessViolationException.
             // -t: Write a dump when the process terminates.
+            // Collect the dump all the time, even if CollectAlways is not enabled to produce dumps for Environment.FailFast. We will later ignore the last
+            // dump file for testhost when we know that test host did not crash.
             // -ma: Full dump argument.
             // -f: Filter the exceptions.
-            StringBuilder procDumpArgument = new StringBuilder($"-accepteula -e 1 -g {(collectAlways ? "-t " : string.Empty)}");
+            // Filter the first chance exceptions only to those that are most likely to kill the whole process.
+
+            // Fully override parameters to procdump
+            var procdumpArgumentsFromEnv = Environment.GetEnvironmentVariable("VSTEST_DUMP_PROCDUMPARGUMENTS")?.Trim();
+
+            // Useful additional arguments are -n 100, to collect all dumps that you can, or -o to overwrite dump, or -f EXCEPTION_NAME to add exception to filter list
+            var procdumpAdditonalArgumentsFromEnv = Environment.GetEnvironmentVariable("VSTEST_DUMP_PROCDUMPADDITIONALARGUMENTS")?.Trim();
+            StringBuilder procDumpArgument = new StringBuilder($"-accepteula -e 1 -g -t {procdumpAdditonalArgumentsFromEnv}");
             if (isFullDump)
             {
                 procDumpArgument.Append("-ma ");
@@ -29,7 +42,11 @@ public string BuildTriggerBasedProcDumpArgs(int processId, string filename, IEnu
             }
 
             procDumpArgument.Append($"{processId} {filename}.dmp");
-            var argument = procDumpArgument.ToString();
+            var argument = string.IsNullOrWhiteSpace(procdumpArgumentsFromEnv) ? procDumpArgument.ToString() : procdumpArgumentsFromEnv;
+            if (!argument.ToUpperInvariant().Contains("-accepteula".ToUpperInvariant()))
+            {
+                argument = $"-accepteula {argument}";
+            }
 
             return argument;
         }