Handle non-crashing cases for server failure to start

Follow up #4!

R=mark@chromium.org
BUG=chromium:567850,chromium:656800
TEST=tests added to crashpad_client_test

Change-Id: I2a53f2168988e620ce240750c6c2d544ba95c8b4
Reviewed-on: https://chromium-review.googlesource.com/406741
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/crashpad_client_win.cc

@@ -95,7 +95,7 @@ void SetHandlerStartupState(StartupState state) {
                               static_cast<base::subtle::AtomicWord>(state));
 }
 
-LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
+StartupState BlockUntilHandlerStartedOrFailed() {
   // Wait until we know the handler has either succeeded or failed to start.
   base::subtle::AtomicWord startup_state;
   while (
@@ -104,7 +104,11 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
     Sleep(1);
   }
 
-  if (startup_state == static_cast<int>(StartupState::kFailed)) {
+  return static_cast<StartupState>(startup_state);
+}
+
+LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
+  if (BlockUntilHandlerStartedOrFailed() == StartupState::kFailed) {
     // If we know for certain that the handler has failed to start, then abort
     // here, rather than trying to signal to a handler that will never arrive,
     // and then sleeping unnecessarily.
@@ -112,6 +116,7 @@ LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
     TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump);
     return EXCEPTION_CONTINUE_SEARCH;
   }
+
   // Otherwise, we know the handler startup has succeeded, and we can continue.
 
   // Tracks whether a thread has already entered UnhandledExceptionHandler.
@@ -644,7 +649,15 @@ bool CrashpadClient::WaitForHandlerStart() {
 void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) {
   if (g_signal_non_crash_dump == INVALID_HANDLE_VALUE ||
       g_non_crash_dump_done == INVALID_HANDLE_VALUE) {
-    LOG(ERROR) << "haven't called UseHandler()";
+    LOG(ERROR) << "not connected";
+    return;
+  }
+
+  if (BlockUntilHandlerStartedOrFailed() == StartupState::kFailed) {
+    // If we know for certain that the handler has failed to start, then abort
+    // here, as we would otherwise wait indefinitely for the
+    // g_non_crash_dump_done event that would never be signalled.
+    LOG(ERROR) << "crash server failed to launch, no dump captured";
     return;
   }
 
@@ -695,11 +708,15 @@ void CrashpadClient::DumpWithoutCrash(const CONTEXT& context) {
 // static
 void CrashpadClient::DumpAndCrash(EXCEPTION_POINTERS* exception_pointers) {
   if (g_signal_exception == INVALID_HANDLE_VALUE) {
-    LOG(ERROR) << "haven't called UseHandler(), no dump captured";
-    TerminateProcess(GetCurrentProcess(), kTerminationCodeUseHandlerNotCalled);
+    LOG(ERROR) << "not connected";
+    TerminateProcess(GetCurrentProcess(),
+                     kTerminationCodeNotConnectedToHandler);
     return;
   }
 
+  // We don't need to check for handler startup here, as
+  // UnhandledExceptionHandler() necessarily does that.
+
   UnhandledExceptionHandler(exception_pointers);
 }
 

client/crashpad_client_win_test.cc

@@ -15,10 +15,12 @@
 #include "client/crashpad_client.h"
 
 #include "base/files/file_path.h"
+#include "base/macros.h"
 #include "gtest/gtest.h"
 #include "test/paths.h"
 #include "test/scoped_temp_dir.h"
 #include "test/win/win_multiprocess.h"
+#include "util/win/termination_codes.h"
 
 namespace crashpad {
 namespace test {
@@ -88,6 +90,92 @@ TEST(CrashpadClient, StartWithSameStdoutStderr) {
   WinMultiprocess::Run<StartWithSameStdoutStderr>();
 }
 
+void StartAndUseBrokenHandler(CrashpadClient* client) {
+  ScopedTempDir temp_dir;
+  base::FilePath handler_path = Paths::Executable().DirName().Append(
+      FILE_PATH_LITERAL("fake_handler_that_crashes_at_startup.exe"));
+  ASSERT_TRUE(client->StartHandler(handler_path,
+                                  temp_dir.path(),
+                                  base::FilePath(),
+                                  "",
+                                  std::map<std::string, std::string>(),
+                                  std::vector<std::string>(),
+                                  false,
+                                  true));
+}
+
+class HandlerLaunchFailureCrash : public WinMultiprocess {
+ public:
+  HandlerLaunchFailureCrash() : WinMultiprocess() {}
+
+ private:
+  void WinMultiprocessParent() override {
+    SetExpectedChildExitCode(crashpad::kTerminationCodeCrashNoDump);
+  }
+
+  void WinMultiprocessChild() override {
+    CrashpadClient client;
+    StartAndUseBrokenHandler(&client);
+    __debugbreak();
+    exit(0);
+  }
+};
+
+TEST(CrashpadClient, HandlerLaunchFailureCrash) {
+  WinMultiprocess::Run<HandlerLaunchFailureCrash>();
+}
+
+class HandlerLaunchFailureDumpAndCrash : public WinMultiprocess {
+ public:
+  HandlerLaunchFailureDumpAndCrash() : WinMultiprocess() {}
+
+ private:
+  void WinMultiprocessParent() override {
+    SetExpectedChildExitCode(crashpad::kTerminationCodeCrashNoDump);
+  }
+
+  void WinMultiprocessChild() override {
+    CrashpadClient client;
+    StartAndUseBrokenHandler(&client);
+    // We don't need to fill this out as we're only checking that we're
+    // terminated with the correct failure code.
+    EXCEPTION_POINTERS info = {};
+    client.DumpAndCrash(&info);
+    exit(0);
+  }
+};
+
+TEST(CrashpadClient, HandlerLaunchFailureDumpAndCrash) {
+  WinMultiprocess::Run<HandlerLaunchFailureDumpAndCrash>();
+}
+
+class HandlerLaunchFailureDumpWithoutCrash : public WinMultiprocess {
+ public:
+  HandlerLaunchFailureDumpWithoutCrash() : WinMultiprocess() {}
+
+ private:
+  void WinMultiprocessParent() override {
+    // DumpWithoutCrash() normally blocks indefinitely. There's no return value,
+    // but confirm that it exits cleanly because it'll return right away if the
+    // handler didn't start.
+    SetExpectedChildExitCode(0);
+  }
+
+  void WinMultiprocessChild() override {
+    CrashpadClient client;
+    StartAndUseBrokenHandler(&client);
+    // We don't need to fill this out as we're only checking that we're
+    // terminated with the correct failure code.
+    CONTEXT context = {};
+    client.DumpWithoutCrash(context);
+    exit(0);
+  }
+};
+
+TEST(CrashpadClient, HandlerLaunchFailureDumpWithoutCrash) {
+  WinMultiprocess::Run<HandlerLaunchFailureDumpWithoutCrash>();
+}
+
 }  // namespace
 }  // namespace test
 }  // namespace crashpad

handler/handler.gyp

@@ -160,6 +160,13 @@
             'win/crash_other_program.cc',
           ],
         },
+        {
+          'target_name': 'fake_handler_that_crashes_at_startup',
+          'type': 'executable',
+          'sources': [
+            'win/fake_handler_that_crashes_at_startup.cc',
+          ],
+        },
         {
           'target_name': 'hanging_program',
           'type': 'executable',

handler/win/fake_handler_that_crashes_at_startup.cc

@@ -0,0 +1,20 @@
+// Copyright 2016 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// This is used to test a crashpad_handler that launches successfully, but then
+// crashes before setting up.
+int wmain() {
+  __debugbreak();
+  return 0;
+}

util/win/termination_codes.h

@@ -29,7 +29,7 @@ enum TerminationCodes : unsigned int {
 
   //! \brief A dump was requested for a client that was never registered with
   //!     the crash handler.
-  kTerminationCodeUseHandlerNotCalled = 0xffff7003,
+  kTerminationCodeNotConnectedToHandler = 0xffff7003,
 };
 
 }  // namespace crashpad