From 17e228e11914eefd7697e6f0a7cbc0eedea58a15 Mon Sep 17 00:00:00 2001 From: erikwright Date: Fri, 31 Jul 2015 12:01:25 -0700 Subject: [PATCH] Synthesize an exception when reporting a hung process. 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} --- DEPS | 2 +- .../app/chrome_watcher_client_unittest_win.cc | 6 +++++ chrome/app/chrome_watcher_client_win.cc | 9 ++++---- chrome/app/chrome_watcher_client_win.h | 15 +++++++----- ...hrome_watcher_command_line_unittest_win.cc | 10 ++++---- chrome/app/chrome_watcher_command_line_win.cc | 23 +++++++++++++++---- chrome/app/chrome_watcher_command_line_win.h | 16 ++++++++----- chrome/app/client_util.cc | 5 +++- chrome/chrome_watcher/chrome_watcher_main.cc | 20 ++++++++++++---- .../chrome_watcher/chrome_watcher_main_api.h | 15 ++++++------ 10 files changed, 82 insertions(+), 39 deletions(-) diff --git a/DEPS b/DEPS index a6477d491d6d16..8aeb1506ede3b3 100644 --- a/DEPS +++ b/DEPS @@ -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', diff --git a/chrome/app/chrome_watcher_client_unittest_win.cc b/chrome/app/chrome_watcher_client_unittest_win.cc index 04db75cb477997..20d1d32ad1a246 100644 --- a/chrome/app/chrome_watcher_client_unittest_win.cc +++ b/chrome/app/chrome_watcher_client_unittest_win.cc @@ -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, @@ -174,6 +175,11 @@ class ChromeWatcherClientThread : public base::SimpleThread { ret.AppendSwitchASCII( kParentHandle, base::UintToString(reinterpret_cast(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; diff --git a/chrome/app/chrome_watcher_client_win.cc b/chrome/app/chrome_watcher_client_win.cc index 89f0709dd4908b..23e595eac86920 100644 --- a/chrome/app/chrome_watcher_client_win.cc +++ b/chrome/app/chrome_watcher_client_win.cc @@ -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 diff --git a/chrome/app/chrome_watcher_client_win.h b/chrome/app/chrome_watcher_client_win.h index aaa8d140d4a4e2..24c137ccb0ee0d 100644 --- a/chrome/app/chrome_watcher_client_win.h +++ b/chrome/app/chrome_watcher_client_win.h @@ -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 + // 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 CommandLineGenerator; // Constructs an instance that launches its watcher process using the command diff --git a/chrome/app/chrome_watcher_command_line_unittest_win.cc b/chrome/app/chrome_watcher_command_line_unittest_win.cc index b31620380c8f67..b7c6be34463f52 100644 --- a/chrome/app/chrome_watcher_command_line_unittest_win.cc +++ b/chrome/app/chrome_watcher_command_line_unittest_win.cc @@ -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, ¤t_result, - &event_result)); + ASSERT_TRUE(InterpretChromeWatcherCommandLine( + cmd_line, ¤t_result, ¤t_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()); } diff --git a/chrome/app/chrome_watcher_command_line_win.cc b/chrome/app/chrome_watcher_command_line_win.cc index 1eaf1a668f0d47..1e576261e5c6dd 100644 --- a/chrome/app/chrome_watcher_command_line_win.cc +++ b/chrome/app/chrome_watcher_command_line_win.cc @@ -17,24 +17,30 @@ 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(handle))); + switch_name, base::UintToString(reinterpret_cast(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(switch_uint); + return switch_uint; +} + +HANDLE ReadHandleFromSwitch(const base::CommandLine& command_line, + const std::string& switch_name) { + return reinterpret_cast(ReadUintSwitch(command_line, switch_name)); } } // namespace @@ -42,9 +48,12 @@ HANDLE ReadHandleFromSwitch(const base::CommandLine& command_line, 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); @@ -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); @@ -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 @@ -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; } diff --git a/chrome/app/chrome_watcher_command_line_win.h b/chrome/app/chrome_watcher_command_line_win.h index 4b4596a7bb9544..d4d4a97dba0b3e 100644 --- a/chrome/app/chrome_watcher_command_line_win.h +++ b/chrome/app/chrome_watcher_command_line_win.h @@ -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_ diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc index 0e05815e8c4dbd..b01d86e25dd19f 100644 --- a/chrome/app/client_util.cc +++ b/chrome/app/client_util.cc @@ -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; } @@ -218,7 +220,8 @@ int MainDllLoader::Launch(HINSTANCE instance) { reinterpret_cast( ::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()); } diff --git a/chrome/chrome_watcher/chrome_watcher_main.cc b/chrome/chrome_watcher/chrome_watcher_main.cc index e51de7cc61b4da..7d9c6db655eafc 100644 --- a/chrome/chrome_watcher/chrome_watcher_main.cc +++ b/chrome/chrome_watcher/chrome_watcher_main.cc @@ -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. @@ -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) { @@ -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, @@ -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 diff --git a/chrome/chrome_watcher/chrome_watcher_main_api.h b/chrome/chrome_watcher/chrome_watcher_main_api.h index d070a577770d3c..d2f129b4a93a4f 100644 --- a/chrome/chrome_watcher/chrome_watcher_main_api.h +++ b/chrome/chrome_watcher/chrome_watcher_main_api.h @@ -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,