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}

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(

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;

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,

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);

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() {

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;

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

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.

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)

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,

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[];

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;

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;

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);
 

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(

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

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();

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)

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;
 

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;
 }

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;