Skip to content

Commit

Permalink
Synthesize an exception when reporting a hung process.
Browse files Browse the repository at this point in the history
This should cause the Crash backend to activate its magic signature bucketing logic, which will assist in triaging these reports.

BUG=
TBR=siggi
NOTRY=true

Review URL: https://codereview.chromium.org/1257403002

Cr-Commit-Position: refs/heads/master@{#341380}
  • Loading branch information
erikwright authored and Commit bot committed Jul 31, 2015
1 parent e9ea838 commit 17e228e
Show file tree
Hide file tree
Showing 10 changed files with 82 additions and 39 deletions.
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,7 @@ hooks = [
'action': ['python',
'src/build/get_syzygy_binaries.py',
'--output-dir=src/third_party/kasko',
'--revision=d609198a0d51019028b45a41d957808986f04dd3',
'--revision=56f13b37f044639b4c28cb75f327ca5e3db8758e',
'--resource=kasko.zip',
'--resource=kasko_symbols.zip',
'--overwrite',
Expand Down
6 changes: 6 additions & 0 deletions chrome/app/chrome_watcher_client_unittest_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ class ChromeWatcherClientThread : public base::SimpleThread {
private:
// Returns a command line to launch back into ChromeWatcherClientTestProcess.
base::CommandLine GenerateCommandLine(HANDLE parent_handle,
DWORD main_thread_id,
HANDLE on_initialized_event) {
base::CommandLine ret = base::GetMultiProcessTestChildBaseCommandLine();
ret.AppendSwitchASCII(switches::kTestChildProcess,
Expand All @@ -174,6 +175,11 @@ class ChromeWatcherClientThread : public base::SimpleThread {
ret.AppendSwitchASCII(
kParentHandle,
base::UintToString(reinterpret_cast<unsigned int>(parent_handle)));

// Our child does not actually need the main thread ID, but we verify here
// that the correct ID is being passed from the client.
EXPECT_EQ(::GetCurrentThreadId(), main_thread_id);

ret.AppendSwitchASCII(kNamedEventSuffix,
base::UTF16ToASCII(NamedEventSuffix()));
return ret;
Expand Down
9 changes: 5 additions & 4 deletions chrome/app/chrome_watcher_client_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,17 @@
namespace {

// Because we can only bind parameters from the left, |parent_process| must be
// the last parameter of the method that we bind int a
// the last parameter of the method that we bind into a
// BrowserWatcherClient::CommandLineGenerator. The ChromeWatcherClient API is
// more intuitive if the ChromeWatcherClient::CommandLineGenerator takes
// |parent_process| as its second (of three) parameters, so we use this
// intermediate function to swap the order.
// |parent_process| as its second parameter, so we use this intermediate
// function to swap the order.
base::CommandLine InvokeCommandLineGenerator(
const ChromeWatcherClient::CommandLineGenerator& command_line_generator,
HANDLE on_initialized_event,
HANDLE parent_process) {
return command_line_generator.Run(parent_process, on_initialized_event);
return command_line_generator.Run(parent_process, ::GetCurrentThreadId(),
on_initialized_event);
}

} // namespace
Expand Down
15 changes: 9 additions & 6 deletions chrome/app/chrome_watcher_client_win.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,15 @@
class ChromeWatcherClient {
public:
// A CommandLineGenerator generates command lines that will launch a separate
// process and pass the supplied HANDLE values to WatcherMain in that process.
// The first HANDLE is the process that the watcher process should watch. The
// second HANDLE is an event that should be signaled when the watcher process
// is fully initialized. The process will be launched such that the HANDLEs
// are inherited by the new process.
typedef base::Callback<base::CommandLine(HANDLE, HANDLE)>
// process and pass the supplied values to WatcherMain in that process.
// |parent_process| is the process that the watcher process should watch;
// |main_thread_id| is the parent process' main thread ID.
// |on_initialized_event| should be signaled when the watcher process is fully
// initialized. The process will be launched such that the HANDLEs are
// inherited by the new process.
typedef base::Callback<base::CommandLine(HANDLE parent_process,
DWORD main_thread_id,
HANDLE on_initialized_event)>
CommandLineGenerator;

// Constructs an instance that launches its watcher process using the command
Expand Down
10 changes: 6 additions & 4 deletions chrome/app/chrome_watcher_command_line_unittest_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,16 @@ TEST(ChromeWatcherCommandLineTest, BasicTest) {

HANDLE event = ::CreateEvent(nullptr, FALSE, FALSE, nullptr);
ASSERT_NE(nullptr, event);

DWORD current_thread_id = ::GetCurrentThreadId();
base::CommandLine cmd_line = GenerateChromeWatcherCommandLine(
base::FilePath(L"example.exe"), current, event);
base::FilePath(L"example.exe"), current, current_thread_id, event);

base::win::ScopedHandle current_result;
DWORD current_thread_id_result = 0;
base::win::ScopedHandle event_result;
ASSERT_TRUE(InterpretChromeWatcherCommandLine(cmd_line, &current_result,
&event_result));
ASSERT_TRUE(InterpretChromeWatcherCommandLine(
cmd_line, &current_result, &current_thread_id_result, &event_result));
ASSERT_EQ(current, current_result.Get());
ASSERT_EQ(current_thread_id, current_thread_id_result);
ASSERT_EQ(event, event_result.Get());
}
23 changes: 18 additions & 5 deletions chrome/app/chrome_watcher_command_line_win.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,34 +17,43 @@ namespace {

const char kOnIninitializedEventHandleSwitch[] = "on-initialized-event-handle";
const char kParentHandleSwitch[] = "parent-handle";
const char kMainThreadIdSwitch[] = "main-thread-id";

void AppendHandleSwitch(const std::string& switch_name,
HANDLE handle,
base::CommandLine* command_line) {
command_line->AppendSwitchASCII(
switch_name, base::UintToString(reinterpret_cast<unsigned int>(handle)));
switch_name, base::UintToString(reinterpret_cast<uint32_t>(handle)));
}

HANDLE ReadHandleFromSwitch(const base::CommandLine& command_line,
uint32_t ReadUintSwitch(const base::CommandLine& command_line,
const std::string& switch_name) {
std::string switch_string = command_line.GetSwitchValueASCII(switch_name);
unsigned int switch_uint = 0;
if (switch_string.empty() ||
!base::StringToUint(switch_string, &switch_uint)) {
DLOG(ERROR) << "Missing or invalid " << switch_name << " argument.";
return nullptr;
return 0;
}
return reinterpret_cast<HANDLE>(switch_uint);
return switch_uint;
}

HANDLE ReadHandleFromSwitch(const base::CommandLine& command_line,
const std::string& switch_name) {
return reinterpret_cast<HANDLE>(ReadUintSwitch(command_line, switch_name));
}

} // namespace

base::CommandLine GenerateChromeWatcherCommandLine(
const base::FilePath& chrome_exe,
HANDLE parent_process,
DWORD main_thread_id,
HANDLE on_initialized_event) {
base::CommandLine command_line(chrome_exe);
command_line.AppendSwitchASCII(switches::kProcessType, "watcher");
command_line.AppendSwitchASCII(kMainThreadIdSwitch,
base::UintToString(main_thread_id));
AppendHandleSwitch(kOnIninitializedEventHandleSwitch, on_initialized_event,
&command_line);
AppendHandleSwitch(kParentHandleSwitch, parent_process, &command_line);
Expand All @@ -54,6 +63,7 @@ base::CommandLine GenerateChromeWatcherCommandLine(
bool InterpretChromeWatcherCommandLine(
const base::CommandLine& command_line,
base::win::ScopedHandle* parent_process,
DWORD* main_thread_id,
base::win::ScopedHandle* on_initialized_event) {
DCHECK(on_initialized_event);
DCHECK(parent_process);
Expand All @@ -66,6 +76,7 @@ bool InterpretChromeWatcherCommandLine(
ReadHandleFromSwitch(command_line, kParentHandleSwitch);
HANDLE on_initialized_event_handle =
ReadHandleFromSwitch(command_line, kOnIninitializedEventHandleSwitch);
*main_thread_id = ReadUintSwitch(command_line, kMainThreadIdSwitch);

if (parent_handle) {
// Initial test of the handle, a zero PID indicates invalid handle, or not
Expand Down Expand Up @@ -93,10 +104,12 @@ bool InterpretChromeWatcherCommandLine(
}
}

if (!on_initialized_event->IsValid() || !parent_process->IsValid()) {
if (!*main_thread_id || !on_initialized_event->IsValid() ||
!parent_process->IsValid()) {
// If one was valid and not the other, free the valid one.
on_initialized_event->Close();
parent_process->Close();
*main_thread_id = 0;
return false;
}

Expand Down
16 changes: 10 additions & 6 deletions chrome/app/chrome_watcher_command_line_win.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,21 +15,25 @@ class FilePath;
} // namespace base

// Generates a CommandLine that will launch |chrome_exe| in Chrome Watcher mode
// to observe |parent_process|. The watcher process will signal
// |on_initialized_event| when its initialization is complete.
// to observe |parent_process|, whose main thread is identified by
// |main_thread_id|. The watcher process will signal |on_initialized_event| when
// its initialization is complete.
base::CommandLine GenerateChromeWatcherCommandLine(
const base::FilePath& chrome_exe,
HANDLE parent_process,
DWORD main_thread_id,
HANDLE on_initialized_event);

// Interprets the Command Line used to launch a Chrome Watcher process and
// extracts the parent process and initialization event HANDLEs. Verifies that
// the handles are usable in this process before returning them. Returns true if
// both handles are successfully parsed and false otherwise. If only one of the
// handles can be parsed, it will be closed.
// extracts the parent process and initialization event HANDLEs and the parent
// process main thread ID. Verifies that the handles are usable in this process
// before returning them. Returns true if all parameters are successfully parsed
// and false otherwise. In case of partial failure, any successfully parsed
// HANDLEs will be closed.
bool InterpretChromeWatcherCommandLine(
const base::CommandLine& command_line,
base::win::ScopedHandle* parent_process,
DWORD* main_thread_id,
base::win::ScopedHandle* on_initialized_event);

#endif // CHROME_APP_CHROME_WATCHER_COMMAND_LINE_WIN_H_
5 changes: 4 additions & 1 deletion chrome/app/client_util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,9 @@ int MainDllLoader::Launch(HINSTANCE instance) {

base::win::ScopedHandle parent_process;
base::win::ScopedHandle on_initialized_event;
DWORD main_thread_id = 0;
if (!InterpretChromeWatcherCommandLine(cmd_line, &parent_process,
&main_thread_id,
&on_initialized_event)) {
return chrome::RESULT_CODE_UNSUPPORTED_PARAM;
}
Expand Down Expand Up @@ -218,7 +220,8 @@ int MainDllLoader::Launch(HINSTANCE instance) {
reinterpret_cast<ChromeWatcherMainFunction>(
::GetProcAddress(watcher_dll, kChromeWatcherDLLEntrypoint));
return watcher_main(chrome::kBrowserExitCodesRegistryPath,
parent_process.Take(), on_initialized_event.Take(),
parent_process.Take(), main_thread_id,
on_initialized_event.Take(),
watcher_data_directory.value().c_str(),
message_window_name.c_str(), channel_name.c_str());
}
Expand Down
20 changes: 15 additions & 5 deletions chrome/chrome_watcher/chrome_watcher_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,8 @@ void OnWindowEvent(
}

#ifdef KASKO
void DumpHungBrowserProcess(const base::string16& channel,
void DumpHungBrowserProcess(DWORD main_thread_id,
const base::string16& channel,
const base::Process& process) {
// TODO(erikwright): Rather than recreating these crash keys here, it would be
// ideal to read them directly from the browser process.
Expand Down Expand Up @@ -274,10 +275,17 @@ void DumpHungBrowserProcess(const base::string16& channel,
}
key_buffers.push_back(nullptr);
value_buffers.push_back(nullptr);

// Synthesize an exception for the main thread.
CONTEXT thread_context = {};
EXCEPTION_RECORD exception_record = {};
exception_record.ExceptionCode = EXCEPTION_ARRAY_BOUNDS_EXCEEDED;
EXCEPTION_POINTERS exception_pointers = {&exception_record, &thread_context};

// TODO(erikwright): Make the dump-type channel-dependent.
kasko::api::SendReportForProcess(process.Handle(),
kasko::api::LARGER_DUMP_TYPE,
key_buffers.data(), value_buffers.data());
kasko::api::SendReportForProcess(
process.Handle(), main_thread_id, &exception_pointers,
kasko::api::LARGER_DUMP_TYPE, key_buffers.data(), value_buffers.data());
}

void LoggedDeregisterEventSource(HANDLE event_source_handle) {
Expand Down Expand Up @@ -349,6 +357,7 @@ void OnCrashReportUpload(void* context,
// mangling.
extern "C" int WatcherMain(const base::char16* registry_path,
HANDLE process_handle,
DWORD main_thread_id,
HANDLE on_initialized_event_handle,
const base::char16* browser_data_directory,
const base::char16* message_window_name,
Expand Down Expand Up @@ -385,7 +394,8 @@ extern "C" int WatcherMain(const base::char16* registry_path,
#ifdef KASKO_HANG_REPORTS
if (launched_kasko &&
base::StringPiece16(channel_name) == installer::kChromeChannelCanary) {
on_hung_callback = base::Bind(&DumpHungBrowserProcess, channel_name);
on_hung_callback =
base::Bind(&DumpHungBrowserProcess, main_thread_id, channel_name);
}
#endif // KASKO_HANG_REPORTS
#endif // KASKO
Expand Down
15 changes: 8 additions & 7 deletions chrome/chrome_watcher/chrome_watcher_main_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,19 @@ extern const char kChromeWatcherDLLEntrypoint[];
extern const base::FilePath::CharType kPermanentlyFailedReportsSubdir[];

// The type of the watcher DLL's main entry point.
// Watches |parent_process| and records its exit code under |registry_path| in
// HKCU. A window named |message_window_name|, owned by |parent_process|, will
// be monitored for responsiveness. If SyzyASAN is enabled, a Kasko reporter
// process is also instantiated, using |browser_data_directory| to store crash
// reports. |on_initialized_event| will be signaled once the watcher process is
// fully initialized. Takes ownership of |parent_process| and
// |on_initialized_event|.
// Watches |parent_process|, whose main thread ID is |main_thread_id|, and
// records its exit code under |registry_path| in HKCU. A window named
// |message_window_name|, owned by |parent_process|, will be monitored for
// responsiveness. If SyzyASAN is enabled, a Kasko reporter process is also
// instantiated, using |browser_data_directory| to store crash reports.
// |on_initialized_event| will be signaled once the watcher process is fully
// initialized. Takes ownership of |parent_process| and |on_initialized_event|.
// |channel_name| is the current Chrome distribution channel (one of
// installer::kChromeChannelXXX).
typedef int (*ChromeWatcherMainFunction)(
const base::char16* registry_path,
HANDLE parent_process,
DWORD main_thread_id,
HANDLE on_initialized_event,
const base::char16* browser_data_directory,
const base::char16* message_window_name,
Expand Down

0 comments on commit 17e228e

Please sign in to comment.