From dbbe24a0e71c80f534fdc5d507cc4eeb4431f567 Mon Sep 17 00:00:00 2001 From: rtenneti Date: Fri, 21 Nov 2014 16:38:38 -0800 Subject: [PATCH] ThreadWatcher - disable optimizations for ThreadWatcher block of functions so the compiler doesn't merge them all together. rvargas and I want to experiment if this would help us to get better callstacks in crash dumps. From wfh@: out of curiosity any idea on why we didn't get the full call stack in dump file? R=rvargas@chromium.org BUG=399438 Review URL: https://codereview.chromium.org/741273002 Cr-Commit-Position: refs/heads/master@{#305329} --- chrome/browser/metrics/thread_watcher.cc | 103 +-------------------- .../browser/metrics/thread_watcher_report_hang.cc | 91 ++++++++++++++++++ .../browser/metrics/thread_watcher_report_hang.h | 26 ++++++ chrome/chrome_browser.gypi | 2 + 4 files changed, 124 insertions(+), 98 deletions(-) create mode 100644 chrome/browser/metrics/thread_watcher_report_hang.cc create mode 100644 chrome/browser/metrics/thread_watcher_report_hang.h diff --git a/chrome/browser/metrics/thread_watcher.cc b/chrome/browser/metrics/thread_watcher.cc index 9f6b7baf2b3d..96e9ea981637 100644 --- a/chrome/browser/metrics/thread_watcher.cc +++ b/chrome/browser/metrics/thread_watcher.cc @@ -8,8 +8,6 @@ #include "base/bind.h" #include "base/compiler_specific.h" -#include "base/debug/alias.h" -#include "base/debug/debugger.h" #include "base/debug/dump_without_crashing.h" #include "base/lazy_instance.h" #include "base/metrics/field_trial.h" @@ -20,6 +18,7 @@ #include "base/threading/thread_restrictions.h" #include "build/build_config.h" #include "chrome/browser/chrome_notification_types.h" +#include "chrome/browser/metrics/thread_watcher_report_hang.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_version_info.h" #include "chrome/common/logging_chrome.h" @@ -31,98 +30,6 @@ using content::BrowserThread; -namespace { - -// The following are unique function names for forcing the crash when a thread -// is unresponsive. This makes it possible to tell from the callstack alone what -// thread was unresponsive. -// -// We disable optimizations for this block of functions so the compiler doesn't -// merge them all together. -MSVC_DISABLE_OPTIMIZE() -MSVC_PUSH_DISABLE_WARNING(4748) - -void ReportThreadHang() { -#if defined(NDEBUG) - base::debug::DumpWithoutCrashing(); -#else - base::debug::BreakDebugger(); -#endif -} - -#if !defined(OS_ANDROID) || !defined(NDEBUG) -// TODO(rtenneti): Enabled crashing, after getting data. -NOINLINE void StartupHang() { - ReportThreadHang(); -} -#endif // OS_ANDROID - -NOINLINE void ShutdownHang() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_UI() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_DB() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_FILE() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_FILE_USER_BLOCKING() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_PROCESS_LAUNCHER() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_CACHE() { - ReportThreadHang(); -} - -NOINLINE void ThreadUnresponsive_IO() { - ReportThreadHang(); -} - -void CrashBecauseThreadWasUnresponsive(BrowserThread::ID thread_id) { - base::debug::Alias(&thread_id); - - switch (thread_id) { - case BrowserThread::UI: - return ThreadUnresponsive_UI(); - case BrowserThread::DB: - return ThreadUnresponsive_DB(); - case BrowserThread::FILE: - return ThreadUnresponsive_FILE(); - case BrowserThread::FILE_USER_BLOCKING: - return ThreadUnresponsive_FILE_USER_BLOCKING(); - case BrowserThread::PROCESS_LAUNCHER: - return ThreadUnresponsive_PROCESS_LAUNCHER(); - case BrowserThread::CACHE: - return ThreadUnresponsive_CACHE(); - case BrowserThread::IO: - return ThreadUnresponsive_IO(); - case BrowserThread::ID_COUNT: - CHECK(false); // This shouldn't actually be reached! - break; - - // Omission of the default hander is intentional -- that way the compiler - // should warn if our switch becomes outdated. - } - - CHECK(false) << "Unknown thread was unresponsive."; // Shouldn't be reached. -} - -MSVC_POP_WARNING() -MSVC_ENABLE_OPTIMIZE(); - -} // namespace - // ThreadWatcher methods and members. ThreadWatcher::ThreadWatcher(const WatchingParams& params) : thread_id_(params.thread_id), @@ -396,7 +303,7 @@ void ThreadWatcher::GotNoResponse() { static bool crashed_once = false; if (!crashed_once) { crashed_once = true; - CrashBecauseThreadWasUnresponsive(thread_id_); + metrics::CrashBecauseThreadWasUnresponsive(thread_id_); } } @@ -927,10 +834,10 @@ class StartupWatchDogThread : public base::Watchdog { // without crashing and in debug mode we break into the debugger. void Alarm() override { #if !defined(NDEBUG) - StartupHang(); + metrics::StartupHang(); return; #elif !defined(OS_ANDROID) - WatchDogThread::PostTask(FROM_HERE, base::Bind(&StartupHang)); + WatchDogThread::PostTask(FROM_HERE, base::Bind(&metrics::StartupHang)); return; #else // TODO(rtenneti): Enable crashing for Android. @@ -954,7 +861,7 @@ class ShutdownWatchDogThread : public base::Watchdog { // Alarm is called if the time expires after an Arm() without someone calling // Disarm(). We crash the browser if this method is called. - void Alarm() override { ShutdownHang(); } + void Alarm() override { metrics::ShutdownHang(); } private: DISALLOW_COPY_AND_ASSIGN(ShutdownWatchDogThread); diff --git a/chrome/browser/metrics/thread_watcher_report_hang.cc b/chrome/browser/metrics/thread_watcher_report_hang.cc new file mode 100644 index 000000000000..fd23414bb06a --- /dev/null +++ b/chrome/browser/metrics/thread_watcher_report_hang.cc @@ -0,0 +1,91 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/browser/metrics/thread_watcher_report_hang.h" + +// We disable optimizations for the whole file so the compiler doesn't merge +// them all together. +MSVC_DISABLE_OPTIMIZE() +MSVC_PUSH_DISABLE_WARNING(4748) + +#include "base/debug/debugger.h" +#include "base/debug/dump_without_crashing.h" +#include "build/build_config.h" + +namespace metrics { + +// The following are unique function names for forcing the crash when a thread +// is unresponsive. This makes it possible to tell from the callstack alone what +// thread was unresponsive. +NOINLINE void ReportThreadHang() { +#if defined(NDEBUG) + base::debug::DumpWithoutCrashing(); +#else + base::debug::BreakDebugger(); +#endif +} + +#if !defined(OS_ANDROID) || !defined(NDEBUG) +// TODO(rtenneti): Enabled crashing, after getting data. +NOINLINE void StartupHang() { + ReportThreadHang(); +} +#endif // OS_ANDROID + +NOINLINE void ShutdownHang() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_UI() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_DB() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_FILE() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_FILE_USER_BLOCKING() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_PROCESS_LAUNCHER() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_CACHE() { + ReportThreadHang(); +} + +NOINLINE void ThreadUnresponsive_IO() { + ReportThreadHang(); +} + +NOINLINE void CrashBecauseThreadWasUnresponsive(int thread_id) { + // TODO(rtenneti): The following is a temporary change to check thread_id + // numbers explicitly so that we will have minimum code. Will change after the + // test run to use content::BrowserThread::ID enum. + if (thread_id == 0) + return ThreadUnresponsive_UI(); + else if (thread_id == 1) + return ThreadUnresponsive_DB(); + else if (thread_id == 2) + return ThreadUnresponsive_FILE(); + else if (thread_id == 3) + return ThreadUnresponsive_FILE_USER_BLOCKING(); + else if (thread_id == 4) + return ThreadUnresponsive_PROCESS_LAUNCHER(); + else if (thread_id == 5) + return ThreadUnresponsive_CACHE(); + else if (thread_id == 6) + return ThreadUnresponsive_IO(); +} + +} // namespace metrics + +MSVC_POP_WARNING() +MSVC_ENABLE_OPTIMIZE(); diff --git a/chrome/browser/metrics/thread_watcher_report_hang.h b/chrome/browser/metrics/thread_watcher_report_hang.h new file mode 100644 index 000000000000..69acc85fa7cc --- /dev/null +++ b/chrome/browser/metrics/thread_watcher_report_hang.h @@ -0,0 +1,26 @@ +// Copyright 2014 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_BROWSER_METRICS_THREAD_WATCHER_REPORT_HANG_H_ +#define CHROME_BROWSER_METRICS_THREAD_WATCHER_REPORT_HANG_H_ + +#include "base/compiler_specific.h" + +namespace metrics { + +// This function makes it possible to tell from the callstack why startup is +// taking too long. +NOINLINE void StartupHang(); + +// This function makes it possible to tell from the callstack why shutdown is +// taking too long. +NOINLINE void ShutdownHang(); + +// This function makes it possible to tell from the callstack alone what thread +// was unresponsive. +NOINLINE void CrashBecauseThreadWasUnresponsive(int thread_id); + +} // namespace metrics + +#endif // CHROME_BROWSER_METRICS_THREAD_WATCHER_REPORT_HANG_H_ diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 471660009a90..4e97a646edeb 100644 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1161,6 +1161,8 @@ 'browser/metrics/thread_watcher.h', 'browser/metrics/thread_watcher_android.cc', 'browser/metrics/thread_watcher_android.h', + 'browser/metrics/thread_watcher_report_hang.cc', + 'browser/metrics/thread_watcher_report_hang.h', 'browser/metrics/time_ticks_experiment_win.cc', 'browser/metrics/time_ticks_experiment_win.h', 'browser/metrics/variations/generated_resources_map.h', -- 2.11.4.GIT