Set a "metrics_client_id" crash key instead of "guid" on Mac OS X.

Crashpad maintains the client ID on its own and is responsible for
sending this form parameter to the Breakpad server during report upload.
When using Crashpad as its crash-reporting implementation, Chrome does
not need to set this crash key.

When Chrome does attempt to set this crash key on its own,
crashpad_handler produces these harmless log messages:

[mmdd/hhmmss:WARNING:crash_report_upload_thread.cc(44)] duplicate key
guid, discarding value 0123456789ABCDEF0123456789ABCDEF

There are valid reasons to provide the metrics client ID along with
the crash report, so this is placed in a distinct crash key,
"metrics_client_id".

BUG=466964
R=cpu@chromium.org, isherman@chromium.org, rsesek@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#321996}

21 files changed, 116 insertions, 22 deletions

diff --git a/chrome/app/chrome_crash_reporter_client.cc b/chrome/app/chrome_crash_reporter_client.cc
index 2f6f7a8..1c9570e 100644
--- a/chrome/app/chrome_crash_reporter_client.cc
+++ b/chrome/app/chrome_crash_reporter_client.cc
@@ -80,10 +80,12 @@ ChromeCrashReporterClient::ChromeCrashReporterClient() {}
 
 ChromeCrashReporterClient::~ChromeCrashReporterClient() {}
 
+#if !defined(OS_MACOSX)
 void ChromeCrashReporterClient::SetCrashReporterClientIdFromGUID(
     const std::string& client_guid) {
-  crash_keys::SetCrashClientIdFromGUID(client_guid);
+  crash_keys::SetMetricsClientIdFromGUID(client_guid);
 }
+#endif
 
 #if defined(OS_WIN)
 bool ChromeCrashReporterClient::GetAlternativeCrashDumpLocation(
diff --git a/chrome/app/chrome_crash_reporter_client.h b/chrome/app/chrome_crash_reporter_client.h
index a48f79e..80b069f 100644
--- a/chrome/app/chrome_crash_reporter_client.h
+++ b/chrome/app/chrome_crash_reporter_client.h
@@ -17,8 +17,10 @@ class ChromeCrashReporterClient : public crash_reporter::CrashReporterClient {
   ~ChromeCrashReporterClient() override;
 
   // crash_reporter::CrashReporterClient implementation.
+#if !defined(OS_MACOSX)
   void SetCrashReporterClientIdFromGUID(
       const std::string& client_guid) override;
+#endif
 #if defined(OS_WIN)
   virtual bool GetAlternativeCrashDumpLocation(base::FilePath* crash_dir)
       override;
diff --git a/chrome/app/chrome_main_delegate.cc b/chrome/app/chrome_main_delegate.cc
index 3b56ecf..e25ff4f 100644
--- a/chrome/app/chrome_main_delegate.cc
+++ b/chrome/app/chrome_main_delegate.cc
@@ -569,6 +569,13 @@ void ChromeMainDelegate::InitMacCrashReporter(
   // dylib is even loaded, to catch potential early crashes.
   crash_reporter::InitializeCrashpad(process_type);
 
+  const bool browser_process = process_type.empty();
+  if (!browser_process) {
+    std::string metrics_client_id =
+        command_line.GetSwitchValueASCII(switches::kMetricsClientID);
+    crash_keys::SetMetricsClientIdFromGUID(metrics_client_id);
+  }
+
   // Mac Chrome is packaged with a main app bundle and a helper app bundle.
   // The main app bundle should only be used for the browser process, so it
   // should never see a --type switch (switches::kProcessType).  Likewise,
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index f3d767f..836c59e 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -1238,7 +1238,14 @@ bool IsAutoReloadVisibleOnlyEnabled() {
 void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
     base::CommandLine* command_line,
     int child_process_id) {
-#if defined(OS_POSIX) && !defined(OS_MACOSX)
+#if defined(OS_MACOSX)
+  scoped_ptr<metrics::ClientInfo> client_info =
+      GoogleUpdateSettings::LoadMetricsClientInfo();
+  if (client_info) {
+    command_line->AppendSwitchASCII(switches::kMetricsClientID,
+                                    client_info->client_id);
+  }
+#elif defined(OS_POSIX)
   if (breakpad::IsCrashReporterEnabled()) {
     scoped_ptr<metrics::ClientInfo> client_info =
         GoogleUpdateSettings::LoadMetricsClientInfo();
@@ -1246,7 +1253,7 @@ void ChromeContentBrowserClient::AppendExtraCommandLineSwitches(
                                     client_info ? client_info->client_id
                                                 : std::string());
   }
-#endif  // defined(OS_POSIX) && !defined(OS_MACOSX)
+#endif
 
   if (logging::DialogsAreSuppressed())
     command_line->AppendSwitch(switches::kNoErrorDialogs);
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.cc b/chrome/browser/metrics/chrome_metrics_service_client.cc
index 2082442..f7bdca9 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.cc
+++ b/chrome/browser/metrics/chrome_metrics_service_client.cc
@@ -181,7 +181,11 @@ void ChromeMetricsServiceClient::RegisterPrefs(PrefRegistrySimple* registry) {
 
 void ChromeMetricsServiceClient::SetMetricsClientId(
     const std::string& client_id) {
-  crash_keys::SetCrashClientIdFromGUID(client_id);
+  crash_keys::SetMetricsClientIdFromGUID(client_id);
+}
+
+void ChromeMetricsServiceClient::OnRecordingDisabled() {
+  crash_keys::ClearMetricsClientId();
 }
 
 bool ChromeMetricsServiceClient::IsOffTheRecordSessionActive() {
diff --git a/chrome/browser/metrics/chrome_metrics_service_client.h b/chrome/browser/metrics/chrome_metrics_service_client.h
index a29d5c6..c240192 100644
--- a/chrome/browser/metrics/chrome_metrics_service_client.h
+++ b/chrome/browser/metrics/chrome_metrics_service_client.h
@@ -58,6 +58,7 @@ class ChromeMetricsServiceClient
 
   // metrics::MetricsServiceClient:
   void SetMetricsClientId(const std::string& client_id) override;
+  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32 GetProduct() override;
   std::string GetApplicationLocale() override;
diff --git a/chrome/common/child_process_logging_win.cc b/chrome/common/child_process_logging_win.cc
index 37a8d3e..9571701 100644
--- a/chrome/common/child_process_logging_win.cc
+++ b/chrome/common/child_process_logging_win.cc
@@ -74,7 +74,7 @@ void Init() {
   scoped_ptr<metrics::ClientInfo> client_info =
       GoogleUpdateSettings::LoadMetricsClientInfo();
   if (client_info)
-    crash_keys::SetCrashClientIdFromGUID(client_info->client_id);
+    crash_keys::SetMetricsClientIdFromGUID(client_info->client_id);
 }
 
 }  // namespace child_process_logging
diff --git a/chrome/common/chrome_switches.cc b/chrome/common/chrome_switches.cc
index 9d0a7c6..193dc25 100644
--- a/chrome/common/chrome_switches.cc
+++ b/chrome/common/chrome_switches.cc
@@ -1270,6 +1270,11 @@ const char kDisableSystemFullscreenForTesting[] =
 // chrome/browser/mac/relauncher.h.
 const char kRelauncherProcess[]             = "relauncher";
 
+// This is how the metrics client ID is passed from the browser process to its
+// children. With Crashpad, the metrics client ID is distinct from the crash
+// client ID.
+const char kMetricsClientID[]               = "metrics-client-id";
+
 #endif
 
 // Use bubbles for content permissions requests instead of infobars.
diff --git a/chrome/common/chrome_switches.h b/chrome/common/chrome_switches.h
index bf948bb..feb28fe6 100644
--- a/chrome/common/chrome_switches.h
+++ b/chrome/common/chrome_switches.h
@@ -368,6 +368,7 @@ extern const char kHostedAppQuitNotification[];
 extern const char kDisableHostedAppShimCreation[];
 extern const char kDisableSystemFullscreenForTesting[];
 extern const char kRelauncherProcess[];
+extern const char kMetricsClientID[];
 #endif
 
 #if defined(OS_WIN)
diff --git a/chrome/common/crash_keys.cc b/chrome/common/crash_keys.cc
index 1983f0d..dfe0f42 100644
--- a/chrome/common/crash_keys.cc
+++ b/chrome/common/crash_keys.cc
@@ -55,7 +55,13 @@ static_assert(kMediumSize <= kSingleChunkLength,
               "mac has medium size crash key chunks");
 #endif
 
+#if defined(OS_MACOSX)
+// Crashpad owns the "guid" key. Chrome's metrics client ID is a separate ID
+// carried in a distinct "metrics_client_id" field.
+const char kMetricsClientId[] = "metrics_client_id";
+#else
 const char kClientId[] = "guid";
+#endif
 
 const char kChannel[] = "channel";
 
@@ -123,7 +129,11 @@ size_t RegisterChromeCrashKeys() {
   // The following keys may be chunked by the underlying crash logging system,
   // but ultimately constitute a single key-value pair.
   base::debug::CrashKey fixed_keys[] = {
+#if defined(OS_MACOSX)
+    { kMetricsClientId, kSmallSize },
+#else
     { kClientId, kSmallSize },
+#endif
     { kChannel, kSmallSize },
     { kActiveURL, kLargeSize },
     { kNumSwitches, kSmallSize },
@@ -234,14 +244,40 @@ size_t RegisterChromeCrashKeys() {
                                     kSingleChunkLength);
 }
 
-void SetCrashClientIdFromGUID(const std::string& client_guid) {
-  std::string stripped_guid(client_guid);
+void SetMetricsClientIdFromGUID(const std::string& metrics_client_guid) {
+  std::string stripped_guid(metrics_client_guid);
   // Remove all instance of '-' char from the GUID. So BCD-WXY becomes BCDWXY.
   ReplaceSubstringsAfterOffset(&stripped_guid, 0, "-", "");
   if (stripped_guid.empty())
     return;
 
+#if defined(OS_MACOSX)
+  // The crash client ID is maintained by Crashpad and is distinct from the
+  // metrics client ID, which is carried in its own key.
+  base::debug::SetCrashKeyValue(kMetricsClientId, stripped_guid);
+#else
+  // The crash client ID is set by the application when Breakpad is in use.
+  // The same ID as the metrics client ID is used.
   base::debug::SetCrashKeyValue(kClientId, stripped_guid);
+#endif
+}
+
+void ClearMetricsClientId() {
+#if defined(OS_MACOSX)
+  // Crashpad always monitors for crashes, but doesn't upload them when
+  // crash reporting is disabled. The preference to upload crash reports is
+  // linked to the preference for metrics reporting. When metrics reporting is
+  // disabled, don't put the metrics client ID into crash dumps. This way, crash
+  // reports that are saved but not uploaded will not have a metrics client ID
+  // from the time that metrics reporting was disabled even if they are uploaded
+  // by user action at a later date.
+  //
+  // Breakpad cannot be enabled or disabled without an application restart, and
+  // it needs to use the metrics client ID as its stable crash client ID, so
+  // leave its client ID intact even when metrics reporting is disabled while
+  // the application is running.
+  base::debug::ClearCrashKey(kMetricsClientId);
+#endif
 }
 
 static bool IsBoringSwitch(const std::string& flag) {
@@ -271,6 +307,11 @@ static bool IsBoringSwitch(const std::string& flag) {
          // (If you need to know can always look at the PEB).
          flag == "--flag-switches-begin" ||
          flag == "--flag-switches-end";
+#elif defined(OS_MACOSX)
+  // These are carried in their own fields.
+  return StartsWithASCII(flag, "--channel=", true) ||
+         StartsWithASCII(flag, "--type=", true) ||
+         StartsWithASCII(flag, "--metrics-client-id=", true);
 #elif defined(OS_CHROMEOS)
   static const char* const kIgnoreSwitches[] = {
     ::switches::kEnableLogging,
diff --git a/chrome/common/crash_keys.h b/chrome/common/crash_keys.h
index d9dd52e..3f40ff5 100644
--- a/chrome/common/crash_keys.h
+++ b/chrome/common/crash_keys.h
@@ -21,11 +21,11 @@ namespace crash_keys {
 // reporting server. Returns the size of the union of all keys.
 size_t RegisterChromeCrashKeys();
 
-// Sets the ID (based on |client_guid| which may either be a full GUID or a
-// GUID that was already stripped from its dashes -- in either cases this method
-// will strip remaining dashes before setting the crash key) by which this crash
-// reporting client can be identified.
-void SetCrashClientIdFromGUID(const std::string& client_guid);
+// Sets the ID (which may either be a full GUID or a GUID that was already
+// stripped from its dashes -- in either case this method will strip remaining
+// dashes before setting the crash key).
+void SetMetricsClientIdFromGUID(const std::string& metrics_client_guid);
+void ClearMetricsClientId();
 
 // Sets the kSwitch and kNumSwitches keys based on the given |command_line|.
 void SetSwitchesFromCommandLine(const base::CommandLine* command_line);
@@ -55,7 +55,16 @@ class ScopedPrinterInfo {
 // Crash Key Name Constants ////////////////////////////////////////////////////
 
 // The GUID used to identify this client to the crash system.
+#if defined(OS_MACOSX)
+// On Mac OS X, the crash reporting client ID is the responsibility of Crashpad.
+// It is not set directly by Chrome. To make the metrics client ID available on
+// the server, it's stored in a distinct key.
+extern const char kMetricsClientID[];
+#else
+// When using Breakpad instead of Crashpad, the crash reporting client ID is the
+// same as the metrics client ID.
 extern const char kClientID[];
+#endif
 
 // The product release/distribution channel.
 extern const char kChannel[];
diff --git a/chromecast/browser/metrics/cast_metrics_service_client.cc b/chromecast/browser/metrics/cast_metrics_service_client.cc
index 48bf45b..a367322 100644
--- a/chromecast/browser/metrics/cast_metrics_service_client.cc
+++ b/chromecast/browser/metrics/cast_metrics_service_client.cc
@@ -52,6 +52,9 @@ void CastMetricsServiceClient::SetMetricsClientId(
   PlatformSetClientID(cast_service_, client_id);
 }
 
+void CastMetricsServiceClient::OnRecordingDisabled() {
+}
+
 void CastMetricsServiceClient::StoreClientInfo(
     const ::metrics::ClientInfo& client_info) {
   const std::string& client_id = client_info.client_id;
diff --git a/chromecast/browser/metrics/cast_metrics_service_client.h b/chromecast/browser/metrics/cast_metrics_service_client.h
index ffa17a3..abc2b57 100644
--- a/chromecast/browser/metrics/cast_metrics_service_client.h
+++ b/chromecast/browser/metrics/cast_metrics_service_client.h
@@ -51,6 +51,7 @@ class CastMetricsServiceClient : public ::metrics::MetricsServiceClient {
 
   // metrics::MetricsServiceClient implementation:
   void SetMetricsClientId(const std::string& client_id) override;
+  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;
diff --git a/components/crash/app/breakpad_mac.mm b/components/crash/app/breakpad_mac.mm
index 8a869d6..f435483 100644
--- a/components/crash/app/breakpad_mac.mm
+++ b/components/crash/app/breakpad_mac.mm
@@ -252,13 +252,6 @@ void InitCrashReporter(const std::string& process_type) {
   SetCrashKeyValue(@"prod", [info_dictionary objectForKey:@BREAKPAD_PRODUCT]);
   SetCrashKeyValue(@"plat", @"OS X");
 
-  if (!is_browser) {
-    // Get the guid from the command line switch.
-    std::string client_guid =
-        command_line->GetSwitchValueASCII(switches::kEnableCrashReporter);
-    GetCrashReporterClient()->SetCrashReporterClientIdFromGUID(client_guid);
-  }
-
   logging::SetLogMessageHandler(&FatalMessageHandler);
   base::debug::SetDumpWithoutCrashingFunction(&DumpHelper::DumpWithoutCrashing);
 
diff --git a/components/crash/app/crash_reporter_client.cc b/components/crash/app/crash_reporter_client.cc
index 3440a18..57edf59 100644
--- a/components/crash/app/crash_reporter_client.cc
+++ b/components/crash/app/crash_reporter_client.cc
@@ -27,9 +27,11 @@ CrashReporterClient* GetCrashReporterClient() {
 CrashReporterClient::CrashReporterClient() {}
 CrashReporterClient::~CrashReporterClient() {}
 
+#if !defined(OS_MACOSX)
 void CrashReporterClient::SetCrashReporterClientIdFromGUID(
     const std::string& client_guid) {
 }
+#endif
 
 #if defined(OS_WIN)
 bool CrashReporterClient::GetAlternativeCrashDumpLocation(
diff --git a/components/crash/app/crash_reporter_client.h b/components/crash/app/crash_reporter_client.h
index af816ef..cff8b7e 100644
--- a/components/crash/app/crash_reporter_client.h
+++ b/components/crash/app/crash_reporter_client.h
@@ -45,11 +45,16 @@ class CrashReporterClient {
   CrashReporterClient();
   virtual ~CrashReporterClient();
 
+#if !defined(OS_MACOSX)
   // Sets the crash reporting client ID, a unique identifier for the client
   // that is sending crash reports. After it is set, it should not be changed.
   // |client_guid| may either be a full GUID or a GUID that was already stripped
   // from its dashes.
+  //
+  // On Mac OS X, this is the responsibility of Crashpad, and can not be set
+  // directly by the client.
   virtual void SetCrashReporterClientIdFromGUID(const std::string& client_guid);
+#endif
 
 #if defined(OS_WIN)
   // Returns true if an alternative location to store the minidump files was
diff --git a/components/crash/app/crashpad_mac.mm b/components/crash/app/crashpad_mac.mm
index 5a3eed8..962cce6 100644
--- a/components/crash/app/crashpad_mac.mm
+++ b/components/crash/app/crashpad_mac.mm
@@ -158,6 +158,10 @@ void InitializeCrashpad(const std::string& process_type) {
 
   g_simple_string_dictionary = new crashpad::SimpleStringDictionary();
   crashpad_info->set_simple_annotations(g_simple_string_dictionary);
+
+  base::debug::SetCrashKeyReportingFunctions(SetCrashKeyValue, ClearCrashKey);
+  crash_reporter_client->RegisterCrashKeys();
+
   SetCrashKeyValue("ptype", browser_process ? base::StringPiece("browser")
                                             : base::StringPiece(process_type));
   SetCrashKeyValue("pid", base::StringPrintf("%d", getpid()));
@@ -171,9 +175,6 @@ void InitializeCrashpad(const std::string& process_type) {
   // the same file and line.
   base::debug::SetDumpWithoutCrashingFunction(DumpWithoutCrashing);
 
-  base::debug::SetCrashKeyReportingFunctions(SetCrashKeyValue, ClearCrashKey);
-  crash_reporter_client->RegisterCrashKeys();
-
   if (browser_process) {
     g_database =
         crashpad::CrashReportDatabase::Initialize(database_path).release();
diff --git a/components/metrics/metrics_service.cc b/components/metrics/metrics_service.cc
index f86f33a..358d423 100644
--- a/components/metrics/metrics_service.cc
+++ b/components/metrics/metrics_service.cc
@@ -388,6 +388,8 @@ void MetricsService::DisableRecording() {
     return;
   recording_active_ = false;
 
+  client_->OnRecordingDisabled();
+
   base::RemoveActionCallback(action_callback_);
 
   for (size_t i = 0; i < metrics_providers_.size(); ++i)
diff --git a/components/metrics/metrics_service_client.h b/components/metrics/metrics_service_client.h
index 1b481f9..489080a 100644
--- a/components/metrics/metrics_service_client.h
+++ b/components/metrics/metrics_service_client.h
@@ -29,6 +29,10 @@ class MetricsServiceClient {
   // when metrics recording gets enabled.
   virtual void SetMetricsClientId(const std::string& client_id) = 0;
 
+  // Notifies the client that recording is disabled, so that other services
+  // (such as crash reporting) can clear any association with metrics.
+  virtual void OnRecordingDisabled() = 0;
+
   // Whether there's an "off the record" (aka "Incognito") session active.
   virtual bool IsOffTheRecordSessionActive() = 0;
 
diff --git a/components/metrics/test_metrics_service_client.cc b/components/metrics/test_metrics_service_client.cc
index 9dcb370..b016672 100644
--- a/components/metrics/test_metrics_service_client.cc
+++ b/components/metrics/test_metrics_service_client.cc
@@ -26,6 +26,9 @@ void TestMetricsServiceClient::SetMetricsClientId(
   client_id_ = client_id;
 }
 
+void TestMetricsServiceClient::OnRecordingDisabled() {
+}
+
 bool TestMetricsServiceClient::IsOffTheRecordSessionActive() {
   return false;
 }
diff --git a/components/metrics/test_metrics_service_client.h b/components/metrics/test_metrics_service_client.h
index 973d6e3..52f1889 100644
--- a/components/metrics/test_metrics_service_client.h
+++ b/components/metrics/test_metrics_service_client.h
@@ -22,6 +22,7 @@ class TestMetricsServiceClient : public MetricsServiceClient {
 
   // MetricsServiceClient:
   void SetMetricsClientId(const std::string& client_id) override;
+  void OnRecordingDisabled() override;
   bool IsOffTheRecordSessionActive() override;
   int32_t GetProduct() override;
   std::string GetApplicationLocale() override;