Stability instrumentation: add a Vectored Exception Handler

components/browser_watcher/stability_debugging_win_unittest.cc

Index: components/browser_watcher/stability_debugging_win_unittest.cc
diff --git a/components/browser_watcher/stability_debugging_win_unittest.cc b/components/browser_watcher/stability_debugging_win_unittest.cc
index 785ab22be786ae76ef070fc2edefc51a53ea74d0..345072122aa7d2cc68a5062bb274399b7aef19bc 100644
--- a/components/browser_watcher/stability_debugging_win_unittest.cc
+++ b/components/browser_watcher/stability_debugging_win_unittest.cc
@@ -4,84 +4,88 @@
 
 #include "components/browser_watcher/stability_debugging.h"
 
-#include "base/files/file_enumerator.h"
+#include <windows.h>
+
+#include "base/command_line.h"
+#include "base/debug/activity_tracker.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/process/process.h"
 #include "base/test/multiprocess_test.h"
-#include "components/browser_watcher/stability_paths.h"
+#include "base/test/test_timeouts.h"
+#include "components/browser_watcher/stability_report.pb.h"
+#include "components/browser_watcher/stability_report_extractor.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/multiprocess_func_list.h"
 
 namespace browser_watcher {
 
-class StabilityDebuggingWinMultiProcTest : public base::MultiProcessTest {};
+using base::debug::GlobalActivityTracker;
 
-MULTIPROCESS_TEST_MAIN(DummyProcess) {
-  return 0;
-}
+const int kMemorySize = 1 << 20;  // 1MiB
+const char* kDebugFileSwitch = "stability_debugging_test_debug_file_switch";
 
-TEST_F(StabilityDebuggingWinMultiProcTest, GetStabilityFileForProcessTest) {
-  const base::FilePath empty_path;
-
-  // Get the path for the current process.
-  base::FilePath stability_path;
-  ASSERT_TRUE(GetStabilityFileForProcess(base::Process::Current(), empty_path,
-                                         &stability_path));
-
-  // Ensure requesting a second time produces the same.
-  base::FilePath stability_path_two;
-  ASSERT_TRUE(GetStabilityFileForProcess(base::Process::Current(), empty_path,
-                                         &stability_path_two));
-  EXPECT_EQ(stability_path, stability_path_two);
-
-  // Ensure a different process has a different stability path.
-  base::SpawnChildResult spawn_result = SpawnChild("DummyProcess");
-  base::FilePath stability_path_other;
-  ASSERT_TRUE(GetStabilityFileForProcess(spawn_result.process, empty_path,
-                                         &stability_path_other));
-  EXPECT_NE(stability_path, stability_path_other);
-}
+class StabilityDebuggingTest : public base::MultiProcessTest {
+ public:
+  StabilityDebuggingTest() {}
+  ~StabilityDebuggingTest() override = default;
+
+  void SetUp() override {
+    testing::Test::SetUp();
 
-TEST(StabilityDebuggingWinTest,
-     GetStabilityFilePatternMatchesGetStabilityFileForProcessResult) {
-  // GetStabilityFileForProcess file names must match GetStabilityFilePattern
-  // according to
-  // FileEnumerator's algorithm. We test this by writing out some files and
-  // validating what is matched.
-
-  base::ScopedTempDir temp_dir;
-  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-  base::FilePath user_data_dir = temp_dir.GetPath();
-
-  // Create the stability directory.
-  base::FilePath stability_dir = GetStabilityDir(user_data_dir);
-  ASSERT_TRUE(base::CreateDirectory(stability_dir));
-
-  // Write a stability file.
-  base::FilePath stability_file;
-  ASSERT_TRUE(GetStabilityFileForProcess(base::Process::Current(),
-                                         user_data_dir, &stability_file));
-  {
-    base::ScopedFILE file(base::OpenFile(stability_file, "w"));
-    ASSERT_TRUE(file.get());
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    debug_file_path_ = temp_dir_.GetPath().AppendASCII("debug.pma");
   }
 
-  // Write a file that shouldn't match.
-  base::FilePath non_matching_file =
-      stability_dir.AppendASCII("non_matching.foo");
-  {
-    base::ScopedFILE file(base::OpenFile(non_matching_file, "w"));
-    ASSERT_TRUE(file.get());
+  const base::FilePath& debug_file_path() { return debug_file_path_; }
+
+ protected:
+  base::CommandLine MakeCmdLine(const std::string& procname) override {
+    base::CommandLine cmd = base::MultiProcessTest::MakeCmdLine(procname);
+    cmd.AppendSwitchPath(kDebugFileSwitch, debug_file_path_);
+    return cmd;
   }
 
-  // Validate only the stability file matches.
-  base::FileEnumerator enumerator(stability_dir, false /* recursive */,
-                                  base::FileEnumerator::FILES,
-                                  GetStabilityFilePattern());
-  ASSERT_EQ(stability_file, enumerator.Next());
-  ASSERT_TRUE(enumerator.Next().empty());
+  base::ScopedTempDir temp_dir_;
+  base::FilePath debug_file_path_;
+};
+
+// Creates a global tracker, registers a vectored exception handler, then
+// crashes.
+MULTIPROCESS_TEST_MAIN(CrashingProcess) {
+  base::FilePath debug_path =
+      base::CommandLine::ForCurrentProcess()->GetSwitchValuePath(
+          kDebugFileSwitch);
+  GlobalActivityTracker::CreateWithFile(debug_path, kMemorySize, 0ULL, "", 3);
+  RegisterStabilityVEH();
+
+  // Crash!
+  char* invalid = nullptr;

[Sigurður Ásgeirsson, 2017/06/09 13:55:15]

I'm not sure the compiler is obliged to honor your intent here. Also it's
probably nice to validate that a caught exception can occur without terminating
the process (which could happen in case of an error in VEH).
So, maybe better to RaiseException with a distinct exception code from within a
__try, then return a different exit code?

[manzagop (departed), 2017/06/09 21:27:35]

Switched to a single process test, as that's simpler if the exception is
recovered from (otherwise we'd need to synchronize the processes to avoid crumb
file cleanup on exit).

+  *invalid = 42;
+
+  return 0;
+}
+
+TEST_F(StabilityDebuggingTest, CrashingTest) {
+  base::SpawnChildResult spawn_result = SpawnChild("CrashingProcess");
+  int exit_code;
+  EXPECT_TRUE(spawn_result.process.WaitForExitWithTimeout(
+      TestTimeouts::action_max_timeout(), &exit_code));
+
+  // Collect the report.
+  StabilityReport report;
+  ASSERT_EQ(SUCCESS, Extract(debug_file_path(), &report));
+
+  // Validate expectations: 1 process with 1 thread that has an exception.

[Sigurður Ásgeirsson, 2017/06/09 13:55:15]

My bet is that this is going to flake a bit, as you'll have all and sundry
piling into the test process and suffering exceptions now and then. More robust
is to have "at least one" test here, for a distinct exception code.

[manzagop (departed), 2017/06/09 21:27:35]

Sgtm! Actually, I'm in the same thread it's easy to look for a specific thread. 

Done.

+  ASSERT_EQ(1, report.process_states_size());
+  const ProcessState& process_state = report.process_states(0);
+  EXPECT_EQ(spawn_result.process.Pid(), process_state.process_id());
+  ASSERT_EQ(1, process_state.threads_size());
+  const ThreadState& thread_state = process_state.threads(0);
+  ASSERT_TRUE(thread_state.has_exception());
+  const Exception& exception = thread_state.exception();
+  EXPECT_EQ(EXCEPTION_ACCESS_VIOLATION, exception.code());
 }
 
 }  // namespace browser_watcher