Change breakpads on the helper processes to keep our rimZ clean.

Initialize crash reporting in helper processes such as the renderer process.
Renderer crash reporting stopped working in r23006 when multiple .app bundles
were introduced, because the stats collection and crash reporting preference
is presently accessed via NSUserDefaults, keyed on the bundle ID.  The main
browser process and helper processes have distinct bundle IDs.  In the new
scheme, only the main browser process consults this preference, and passes it
to helper processes in their command lines.

BUG=19204
TEST=When reporting is enabled, Breakpad should pick up browser and renderer
     process crashes;
     When reporting is enabled, renderer should not log messages like
         [mmdd/hhmmss:WARNING:/path/to/breakpad_mac.mm(47)] Breakpad disabled;
     When reporting is disabled, browser and renderer should both log these
         messages.
Review URL: http://codereview.chromium.org/165546

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23509 0039d316-1c4b-4281-b951-d872f2087c98

13 files changed, 125 insertions, 96 deletions

diff --git a/chrome/app/breakpad_linux.cc b/chrome/app/breakpad_linux.cc
index b23a5d7..19c82ba 100644
--- a/chrome/app/breakpad_linux.cc
+++ b/chrome/app/breakpad_linux.cc
@@ -611,11 +611,11 @@ void InitCrashReporter() {
     // We might be chrooted in a zygote or renderer process so we cannot call
     // GetCollectStatsConsent because that needs access the the user's home
     // dir. Instead, we set a command line flag for these processes.
-    if (!parsed_command_line.HasSwitch(switches::kRendererCrashDump))
+    if (!parsed_command_line.HasSwitch(switches::kEnableCrashReporter))
       return;
     // Get the guid and linux distro from the command line switch.
     std::string switch_value = WideToASCII(
-        parsed_command_line.GetSwitchValue(switches::kRendererCrashDump));
+        parsed_command_line.GetSwitchValue(switches::kEnableCrashReporter));
     size_t separator = switch_value.find(",");
     if (separator != std::string::npos) {
       google_update::linux_guid = switch_value.substr(0, separator);
diff --git a/chrome/app/breakpad_mac.h b/chrome/app/breakpad_mac.h
index 8872191..ce6f25f 100644
--- a/chrome/app/breakpad_mac.h
+++ b/chrome/app/breakpad_mac.h
@@ -5,8 +5,6 @@
 #ifndef CHROME_APP_BREAKPAD_MAC_H_
 #define CHROME_APP_BREAKPAD_MAC_H_
 
-extern "C" {
-
 // This header defines the Chrome entry points for Breakpad integration.
 
 // Initializes Breakpad.
@@ -23,7 +21,7 @@ bool IsCrashReporterDisabled();
 // Call on clean process shutdown.
 void DestructCrashReporter();
 
-#if __OBJC__
+#ifdef __OBJC__
 
 @class NSString;
 
@@ -36,6 +34,4 @@ void ClearCrashKeyValue(NSString* key);
 
 #endif  // __OBJC__
 
-}
-
 #endif  // CHROME_APP_BREAKPAD_MAC_H_
diff --git a/chrome/app/breakpad_mac.mm b/chrome/app/breakpad_mac.mm
index 3bad98d..8a36f16 100644
--- a/chrome/app/breakpad_mac.mm
+++ b/chrome/app/breakpad_mac.mm
@@ -14,15 +14,14 @@
 #include "base/sys_string_conversions.h"
 #import "breakpad/src/client/mac/Framework/Breakpad.h"
 #include "chrome/common/child_process_logging.h"
+#include "chrome/common/chrome_switches.h"
 #include "chrome/installer/util/google_update_settings.h"
 
-extern "C" {
-
 namespace {
 
 BreakpadRef gBreakpadRef = NULL;
 
-} // namespace
+}  // namespace
 
 bool IsCrashReporterDisabled() {
   return gBreakpadRef == NULL;
@@ -40,66 +39,65 @@ void InitCrashReporter() {
   DCHECK(gBreakpadRef == NULL);
   base::ScopedNSAutoreleasePool autorelease_pool;
 
-  // Check for Send stats preference. If preference is not specifically turned
-  // on then disable crash reporting.
-  bool user_consented = GoogleUpdateSettings::GetCollectStatsConsent();
-  if (!user_consented) {
+  // Check whether the user has consented to stats and crash reporting.  The
+  // browser process can make this determination directly.  Helper processes
+  // may not have access to the disk or to the same data as the browser
+  // process, so the browser passes the consent preference to them on the
+  // command line.
+  NSBundle* main_bundle = [NSBundle mainBundle];
+  NSDictionary* info_dictionary = [main_bundle infoDictionary];
+  bool is_browser = [[info_dictionary objectForKey:@"LSUIElement"]
+                                   isEqualToString:@"1"] ? true : false;
+  bool enable_breakpad =
+      is_browser ? GoogleUpdateSettings::GetCollectStatsConsent() :
+                   CommandLine::ForCurrentProcess()->
+                       HasSwitch(switches::kEnableCrashReporter);
+
+  if (!enable_breakpad) {
     LOG(WARNING) << "Breakpad disabled";
     return;
   }
 
-  NSBundle* main_bundle = [NSBundle mainBundle];
+  // Tell Breakpad where crash_inspector and crash_report_sender are.
   NSString* resource_path = [main_bundle resourcePath];
-
-  NSDictionary* info_dictionary = [main_bundle infoDictionary];
-  NSMutableDictionary *breakpad_config = [info_dictionary
-                                             mutableCopy];
-
-  // Tell Breakpad where inspector & crash_reporter are.
-  NSString *inspector_location = [resource_path
-      stringByAppendingPathComponent:@"crash_inspector"];
-  NSString *reporter_bundle_location = [resource_path
-      stringByAppendingPathComponent:@"crash_report_sender.app"];
-  NSString *reporter_location = [[NSBundle
-                                      bundleWithPath:reporter_bundle_location]
-                                     executablePath];
-
+  NSString *inspector_location =
+      [resource_path stringByAppendingPathComponent:@"crash_inspector"];
+  NSString *reporter_bundle_location =
+      [resource_path stringByAppendingPathComponent:@"crash_report_sender.app"];
+  NSString *reporter_location =
+      [[NSBundle bundleWithPath:reporter_bundle_location] executablePath];
+
+  NSMutableDictionary *breakpad_config =
+      [[info_dictionary mutableCopy] autorelease];
   [breakpad_config setObject:inspector_location
                       forKey:@BREAKPAD_INSPECTOR_LOCATION];
   [breakpad_config setObject:reporter_location
                       forKey:@BREAKPAD_REPORTER_EXE_LOCATION];
 
-  // Pass crash to Crash Reporter if we're a foreground application [the
-  // browser process].  This is so the user gets notification when Chrome
-  // crashes and also since we get "restart ui" for free.
-  BOOL is_background_app = [[info_dictionary objectForKey:@"LSUIElement"]
-                                isEqualToString:@"1"];
-  if (!is_background_app) {
+  // In the main application (the browser process), crashes can be passed to
+  // the system's Crash Reporter.  This allows the system to notify the user
+  // when the application crashes, and provide the user with the option to
+  // restart it.
+  if (is_browser)
     [breakpad_config setObject:@"NO" forKey:@BREAKPAD_SEND_AND_EXIT];
-  }
 
-  // Init breakpad
-  BreakpadRef breakpad = NULL;
-  breakpad = BreakpadCreate(breakpad_config);
-  if (!breakpad) {
-    LOG(ERROR) << "Breakpad init failed.";
+  // Initialize Breakpad.
+  gBreakpadRef = BreakpadCreate(breakpad_config);
+  if (!gBreakpadRef) {
+    LOG(ERROR) << "Breakpad initializaiton failed";
     return;
   }
 
-  // This needs to be set before calling SetCrashKeyValue().
-  gBreakpadRef = breakpad;
-
-  // Set breakpad MetaData values.
-  // These values are added to the plist when building a branded Chrome.app.
-  NSString* version_str = [info_dictionary objectForKey:@BREAKPAD_VERSION];
-  SetCrashKeyValue(@"ver", version_str);
-  NSString* prod_name_str = [info_dictionary objectForKey:@BREAKPAD_PRODUCT];
-  SetCrashKeyValue(@"prod", prod_name_str);
+  // Set Breakpad metadata values.  These values are added to Info.plist during
+  // the branded Google Chrome.app build.
+  SetCrashKeyValue(@"ver", [info_dictionary objectForKey:@BREAKPAD_VERSION]);
+  SetCrashKeyValue(@"prod", [info_dictionary objectForKey:@BREAKPAD_PRODUCT]);
   SetCrashKeyValue(@"plat", @"OS X");
 
-  // Enable child process crashes to include the page url.
-  child_process_logging::SetCrashKeyFunctions(
-      SetCrashKeyValue, ClearCrashKeyValue);
+  // Enable child process crashes to include the page URL.
+  // TODO: Should this only be done for certain process types?
+  child_process_logging::SetCrashKeyFunctions(SetCrashKeyValue,
+                                              ClearCrashKeyValue);
 }
 
 void InitCrashProcessInfo() {
@@ -108,10 +106,9 @@ void InitCrashProcessInfo() {
   }
 
   // Determine the process type.
-  NSString *process_type = @"browser";
-  const CommandLine& parsed_command_line = *CommandLine::ForCurrentProcess();
+  NSString* process_type = @"browser";
   std::wstring process_type_switch =
-      parsed_command_line.GetSwitchValue(switches::kProcessType);
+      CommandLine::ForCurrentProcess()->GetSwitchValue(switches::kProcessType);
   if (!process_type_switch.empty()) {
     process_type = base::SysWideToNSString(process_type_switch);
   }
@@ -139,6 +136,3 @@ void ClearCrashKeyValue(NSString* key) {
 
   BreakpadRemoveUploadParameter(gBreakpadRef, key);
 }
-
-}
-
diff --git a/chrome/app/chrome_dll_main.cc b/chrome/app/chrome_dll_main.cc
index c17dcfe..4b23cc1 100644
--- a/chrome/app/chrome_dll_main.cc
+++ b/chrome/app/chrome_dll_main.cc
@@ -298,29 +298,14 @@ int ChromeMain(int argc, const char** argv) {
 #endif
 
 #if defined(OS_MACOSX)
-  // TODO(mark): Some of these things ought to be handled in chrome_exe_main.mm,
-  // such as Breakpad initialization.  Under the current architecture, nothing
-  // in chrome_exe_main can rely directly on chrome_dll code on the Mac,
-  // though, so until some of this code is refactored to avoid such a
-  // dependency, it lives here.  See also the TODO(mark) below at
-  // DestructCrashReporter().
+  // TODO(mark): Some of these things ought to be handled in chrome_exe_main.mm.
+  // Under the current architecture, nothing in chrome_exe_main can rely
+  // directly on chrome_dll code on the Mac, though, so until some of this code
+  // is refactored to avoid such a dependency, it lives here.  See also the
+  // TODO(mark) below at InitCrashReporter() and DestructCrashReporter().
   base::EnableTerminationOnHeapCorruption();
-
-  // The exit manager is in charge of calling the dtors of singletons.
-  // Win has one here, but we assert with multiples from BrowserMain() if we
-  // keep it.
-  // base::AtExitManager exit_manager;
-
-#if defined(GOOGLE_CHROME_BUILD)
-  InitCrashReporter();
-#endif
-
-  // If Breakpad is not present then turn off os crash dumps so we don't have
-  // to wait eons for Apple's Crash Reporter to generate a dump.
-  if (IsCrashReporterDisabled()) {
-    DebugUtil::DisableOSCrashDumps();
-  }
 #endif  // OS_MACOSX
+
   RegisterInvalidParamHandler();
 
   // The exit manager is in charge of calling the dtors of singleton objects.
@@ -350,9 +335,20 @@ int ChromeMain(int argc, const char** argv) {
 #endif
 
 #if defined(OS_MACOSX)
-  // Needs to be called after CommandLine::Init().
-  InitCrashProcessInfo();
-#endif
+  // TODO(mark): Right now, InitCrashReporter() needs to be called after
+  // CommandLine::Init().  Ideally, Breakpad initialization could occur
+  // sooner, preferably even before the framework dylib is even loaded, to
+  // catch potential early early crashes.
+  InitCrashReporter();
+
+  // If Breakpad is not present, turn off OS crash dumps to avoid having
+  // to wait eons for Apple's Crash Reporter to generate dumps for builds
+  // where debugging symbols are present.
+  if (IsCrashReporterDisabled())
+    DebugUtil::DisableOSCrashDumps();
+  else
+    InitCrashProcessInfo();
+#endif  // OS_MACOSX
 
   const CommandLine& parsed_command_line = *CommandLine::ForCurrentProcess();
 
diff --git a/chrome/browser/plugin_process_host.cc b/chrome/browser/plugin_process_host.cc
index 9a81b94..711b9df 100644
--- a/chrome/browser/plugin_process_host.cc
+++ b/chrome/browser/plugin_process_host.cc
@@ -404,6 +404,8 @@ bool PluginProcessHost::Init(const WebPluginInfo& info,
   cmd_line.AppendSwitchWithValue(switches::kPluginPath,
                                  info.path.ToWStringHack());
 
+  SetCrashReporterCommandLine(&cmd_line);
+
   base::ProcessHandle process = 0;
 #if defined(OS_WIN)
   process = sandbox::StartProcess(&cmd_line);
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index eddbdda..0a7c12b 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -401,19 +401,14 @@ bool BrowserRenderProcessHost::Init() {
   }
 #endif  // OS_POSIX
 
-#if defined(OS_LINUX)
-  if (GoogleUpdateSettings::GetCollectStatsConsent())
-    cmd_line.AppendSwitchWithValue(switches::kRendererCrashDump,
-                                   ASCIIToWide(google_update::linux_guid +
-                                               "," + base::GetLinuxDistro()));
-#endif
-
   cmd_line.AppendSwitchWithValue(switches::kProcessType,
                                  switches::kRendererProcess);
 
   cmd_line.AppendSwitchWithValue(switches::kProcessChannelID,
                                  ASCIIToWide(channel_id));
 
+  ChildProcessHost::SetCrashReporterCommandLine(&cmd_line);
+
   const std::wstring& profile_path =
       browser_command_line.GetSwitchValue(switches::kUserDataDir);
   if (!profile_path.empty())
diff --git a/chrome/browser/utility_process_host.cc b/chrome/browser/utility_process_host.cc
index c06749d..285197a 100644
--- a/chrome/browser/utility_process_host.cc
+++ b/chrome/browser/utility_process_host.cc
@@ -74,6 +74,7 @@ bool UtilityProcessHost::StartProcess(const FilePath& exposed_dir) {
                                  switches::kUtilityProcess);
   cmd_line.AppendSwitchWithValue(switches::kProcessChannelID,
                                  ASCIIToWide(channel_id()));
+  SetCrashReporterCommandLine(&cmd_line);
 
   base::ProcessHandle process;
 #if defined(OS_WIN)
diff --git a/chrome/browser/worker_host/worker_process_host.cc b/chrome/browser/worker_host/worker_process_host.cc
index 16cbada..3de362cf 100644
--- a/chrome/browser/worker_host/worker_process_host.cc
+++ b/chrome/browser/worker_host/worker_process_host.cc
@@ -97,6 +97,7 @@ bool WorkerProcessHost::Init() {
                                  switches::kWorkerProcess);
   cmd_line.AppendSwitchWithValue(switches::kProcessChannelID,
                                  ASCIIToWide(channel_id()));
+  SetCrashReporterCommandLine(&cmd_line);
 
   if (CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableNativeWebWorkers)) {
diff --git a/chrome/common/DEPS b/chrome/common/DEPS
index ed2c80a..50f16bfb 100644
--- a/chrome/common/DEPS
+++ b/chrome/common/DEPS
@@ -17,4 +17,5 @@ include_rules = [
   # FIXME - refactor code and remove these dependencies
   "+chrome/app",
   "+chrome/browser",
+  "+chrome/installer",
 ]
diff --git a/chrome/common/child_process_host.cc b/chrome/common/child_process_host.cc
index ca0f972..1fa91f9 100644
--- a/chrome/common/child_process_host.cc
+++ b/chrome/common/child_process_host.cc
@@ -12,6 +12,7 @@
 #include "base/path_service.h"
 #include "base/process_util.h"
 #include "base/singleton.h"
+#include "base/string_util.h"
 #include "base/waitable_event.h"
 #include "chrome/browser/chrome_thread.h"
 #include "chrome/common/chrome_switches.h"
@@ -20,10 +21,23 @@
 #include "chrome/common/plugin_messages.h"
 #include "chrome/common/process_watcher.h"
 #include "chrome/common/result_codes.h"
+#include "chrome/installer/util/google_update_settings.h"
 #include "ipc/ipc_logging.h"
 
+#if defined(OS_LINUX)
+#include "base/linux_util.h"
+
+// This is defined in chrome/browser/google_update_settings_linux.cc.  It's the
+// static string containing the user's unique GUID.  We send this in the crash
+// report.
+namespace google_update {
+extern std::string linux_guid;
+}  // namespace google_update
+#endif  // OS_LINUX
+
 
 namespace {
+
 typedef std::list<ChildProcessHost*> ChildProcessList;
 
 // The NotificationTask is used to notify about plugin process connection/
@@ -49,7 +63,6 @@ class ChildNotificationTask : public Task {
 }  // namespace
 
 
-
 ChildProcessHost::ChildProcessHost(
     ProcessType type, ResourceDispatcherHost* resource_dispatcher_host)
     : Receiver(type),
@@ -130,6 +143,22 @@ std::wstring ChildProcessHost::GetChildPath() {
 #endif  // OS_MACOSX
 }
 
+// static
+void ChildProcessHost::SetCrashReporterCommandLine(CommandLine* command_line) {
+#if defined(OS_POSIX)
+  if (GoogleUpdateSettings::GetCollectStatsConsent()) {
+#if defined(OS_LINUX)
+    command_line->AppendSwitchWithValue(switches::kEnableCrashReporter,
+                                        ASCIIToWide(google_update::linux_guid +
+                                                    "," +
+                                                    base::GetLinuxDistro()));
+#else  // !OS_LINUX
+    command_line->AppendSwitch(switches::kEnableCrashReporter);
+#endif  // !OS_LINUX
+  }
+#endif  // OS_POSIX
+}
+
 bool ChildProcessHost::CreateChannel() {
   channel_id_ = GenerateRandomChannelID(this);
   channel_.reset(new IPC::Channel(
diff --git a/chrome/common/child_process_host.h b/chrome/common/child_process_host.h
index 95b97b6..cb833dc 100644
--- a/chrome/common/child_process_host.h
+++ b/chrome/common/child_process_host.h
@@ -13,6 +13,7 @@
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "ipc/ipc_channel.h"
 
+class CommandLine;
 class NotificationType;
 
 // Plugins/workers and other child processes that live on the IO thread should
@@ -29,6 +30,13 @@ class ChildProcessHost : public ResourceDispatcherHost::Receiver,
   // returns an empty wstring.
   static std::wstring GetChildPath();
 
+  // Prepares command_line for crash reporting as appropriate.  On Linux and
+  // Mac, a command-line flag to enable crash reporting in the child process
+  // will be appended if needed, because the child process may not have access
+  // to the data that determines the status of crash reporting in the
+  // currently-executing process.  This function is a no-op on Windows.
+  static void SetCrashReporterCommandLine(CommandLine* command_line);
+
   // ResourceDispatcherHost::Receiver implementation:
   virtual bool Send(IPC::Message* msg);
 
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 6c8d381..4204e27 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -529,10 +529,13 @@ const wchar_t kEnableMonitorProfile[] = L"enable-monitor-profile";
 // still experimental.
 const wchar_t kEnableXSSAuditor[] = L"enable-xss-auditor";
 
-// A flag, generated internally by Chrome for renderer command lines (Linux
-// only). It tells the renderer to enable crash dumping since it cannot access
-// the user's home directory to find out for itself.
-const wchar_t kRendererCrashDump[] = L"renderer-crash-dumping";
+#if defined(OS_POSIX)
+// A flag, generated internally by Chrome for renderer and other helper process
+// command lines on Linux and Mac.  It tells the helper process to enable crash
+// dumping and reporting, because helpers cannot access the profile or other
+// files needed to make this decision.
+const wchar_t kEnableCrashReporter[] = L"enable-crash-reporter";
+#endif
 
 // Enables the new Tabstrip on Windows.
 const wchar_t kEnableTabtastic2[] = L"enable-tabtastic2";
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index 976d88a..e3ff1a9 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -7,6 +7,7 @@
 #ifndef CHROME_COMMON_CHROME_SWITCHES_H_
 #define CHROME_COMMON_CHROME_SWITCHES_H_
 
+#include "build/build_config.h"
 #include "base/base_switches.h"
 
 namespace switches {
@@ -200,7 +201,9 @@ extern const wchar_t kEnableMonitorProfile[];
 
 extern const wchar_t kEnableXSSAuditor[];
 
-extern const wchar_t kRendererCrashDump[];
+#if defined(OS_POSIX)
+extern const wchar_t kEnableCrashReporter[];
+#endif
 
 extern const wchar_t kEnableTabtastic2[];