Handle non-crashing cases for server failure to start [crashpad/crashpad : master]
Scott Graham (Gerrit)
Scott Graham posted comments on Handle non-crashing cases for server failure to start.
Patch Set 1: I figured maybe some of those test things might be a good idea at this point.
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on Handle non-crashing cases for server failure to start.
Patch Set 1: (6 comments) Thanks for the new tests!
Patch Set #1, Line 11:
R=ma...@chromium.orgSince we’ve got tests for this now, you can add TEST=.
File client/crashpad_client_win.cc:
Patch Set #1, Line 652:
LOG(ERROR) << "haven't called StartHandler()";SetHandlerIPCPipe() would be fine too. But instead of listing all of the possibilities in a log message, maybe just say something like “no handler”.
Patch Set #1, Line 659:
// and then sleeping unnecessarily.It’s not so much a sleep in this function as it is a wait for an event that will never be signaled.
Patch Set #1, Line 717:
// We don't to check for handler startup here, as UnhandledExceptionHandler()Don’t [need] to?
File handler/win/fake_handler_that_crashes_at_startup.cc:
Patch Set #1, Line 15:
#include <windows.h>Unused.
File util/win/termination_codes.h:
Patch Set #1, Line 32:
kTerminationCodeStartHandlerNotCalled = 0xffff7003,As with the log message text, the “StartHandler” bit isn’t quite right.
To view, visit this change. To unsubscribe, visit settings.
Scott Graham (Gerrit)
Scott Graham posted comments on Handle non-crashing cases for server failure to start.
Patch Set 3: (5 comments) Thanks
File client/crashpad_client_win.cc:
Patch Set #1, Line 652:
LOG(ERROR) << "haven't connected to handler";
SetHandlerIPCPipe() would be fine too. But instead of listing all of the po
Done
Patch Set #1, Line 659:
// g_non_crash_dump_done event that would never be signalled.
It’s not so much a sleep in this function as it is a wait for an event that
Done
Patch Set #1, Line 717:
// We don't need to check for handler startup here, asDon’t [need] to?
Done
File handler/win/fake_handler_that_crashes_at_startup.cc:
Patch Set #1, Line 15:
// This is used to test a crashpad_handler that launches successfully, but thenUnused.
Done
File util/win/termination_codes.h:
Patch Set #1, Line 32:
kTerminationCodeNotConnectedToHandler = 0xffff7003,
As with the log message text, the “StartHandler” bit isn’t quite right.
Done
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on Handle non-crashing cases for server failure to start.
Patch Set 3: Code-Review+1 (1 comment)
File client/crashpad_client_win.cc:
Patch Set #3, Line 652:
LOG(ERROR) << "haven't connected to handler";Just “not connected”, here and on line 711, easier.
Scott Graham (Gerrit)
Scott Graham posted comments on Handle non-crashing cases for server failure to start.
Patch Set 4: (1 comment)
File client/crashpad_client_win.cc:
Patch Set #3, Line 652:
LOG(ERROR) << "not connected";
Just “not connected”, here and on line 711, easier.
Done
To view, visit this change. To unsubscribe, visit settings.
Scott Graham (Gerrit)
Scott Graham merged Handle non-crashing cases for server failure to start.
Handle non-crashing cases for server failure to start Follow up #4! R=ma...@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 <ma...@chromium.org> --- M client/crashpad_client_win.cc M client/crashpad_client_win_test.cc M handler/handler.gyp A handler/win/fake_handler_that_crashes_at_startup.cc M util/win/termination_codes.h 5 files changed, 138 insertions(+), 6 deletions(-)
Approvals: Mark Mentovai: Looks good to me
diff --git a/client/crashpad_client_win.cc b/client/crashpad_client_win.cc
index 20a66d5..235ef18 100644
--- a/client/crashpad_client_win.cc
+++ b/client/crashpad_client_win.cc
@@ -95,7 +95,7 @@
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 @@
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 @@
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 @@
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 @@
// 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);
}
diff --git a/client/crashpad_client_win_test.cc b/client/crashpad_client_win_test.cc
index cd66320..94a9b0c 100644
--- a/client/crashpad_client_win_test.cc
+++ b/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 @@
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
diff --git a/handler/handler.gyp b/handler/handler.gyp
index cb219fb..d690053 100644
--- a/handler/handler.gyp
+++ b/handler/handler.gyp
@@ -161,6 +161,13 @@
],
},
{
+ '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',
'dependencies': [
diff --git a/handler/win/fake_handler_that_crashes_at_startup.cc b/handler/win/fake_handler_that_crashes_at_startup.cc
new file mode 100644
index 0000000..028190a
--- /dev/null
+++ b/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;
+}
diff --git a/util/win/termination_codes.h b/util/win/termination_codes.h
index 1cb69b1..1bcbf5d 100644
--- a/util/win/termination_codes.h
+++ b/util/win/termination_codes.h
@@ -29,7 +29,7 @@
//! \brief A dump was requested for a client that was never registered with
//! the crash handler.
- kTerminationCodeUseHandlerNotCalled = 0xffff7003,
+ kTerminationCodeNotConnectedToHandler = 0xffff7003,
};
} // namespace crashpad
To view, visit this change. To unsubscribe, visit settings.
Mark Mentovai (Gerrit)
Mark Mentovai posted comments on Handle non-crashing cases for server failure to start.
Patch Set 5: I can think of no further follow-ups this time. Maybe we’re done?
To view, visit this change. To unsubscribe, visit settings.
Scott Graham (Gerrit)
Scott Graham posted comments on Handle non-crashing cases for server failure to start.
Patch Set 5: > Patch Set 5: > > I can think of no further follow-ups this time. Maybe we’re done? Hopefully! I'll roll into Chrome now and do some testing and see how our theory works out in practice.