Make //components/crash/core/common:crash_key a component when using Breakpad.

With Breakpad, the crash key storage is stored in a static global, and
having the target be a static_library resulted in having multiple copies
existing in the component build.

This also changes the Breakpad crash key storage to be lazily
initialized, which matches what Crashpad does after
https://chromium-review.googlesource.com/c/crashpad/crashpad/+/793981.
The storage is still initialized deterministically in the static release
build, but is done to support unit tests.

Bug: 598854
Change-Id: Ic6f100fd0310545fe2697152bd076846153f16d2
Reviewed-on: https://chromium-review.googlesource.com/802274Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521382}

components/crash/core/common/BUILD.gn

@@ -13,22 +13,48 @@ group("common") {
   }
 }
 
-static_library("crash_key") {
+use_crashpad = is_mac || is_win
+use_stubs = is_fuchsia
+
+# Crashpad's annotation system can store data on a per-module basis (i.e.,
+# in different shared libraries in the component build) without issue. The
+# Breakpad implementation uses a static global variable, so ensure there is
+# only one instance of the symbol in the component build by making this
+# target a component.
+if (use_stubs || use_crashpad) {
+  crash_key_target_type = "static_library"
+} else {
+  crash_key_target_type = "component"
+}
+target(crash_key_target_type, "crash_key") {
   sources = [
+    "crash_export.h",
     "crash_key.cc",
     "crash_key.h",
     "crash_key_base_support.cc",
     "crash_key_base_support.h",
   ]
 
+  defines = [ "CRASH_CORE_COMMON_IMPLEMENTATION" ]
+
+  # This target is not always a component, depending on the implementation.
+  # When it is not a component, annotating functions with the standard
+  # CRASH_EXPORT macro causes linking errors on Windows (clients of this target
+  # expect it to be dllimport but it is linked statically). Instead, provide a
+  # wrapper macro CRASH_KEY_EXPORT that only evaluates to CRASH_EXPORT if this
+  # target is really a component.
+  if (crash_key_target_type == "component") {
+    defines += [ "CRASH_KEY_EXPORT=CRASH_EXPORT" ]
+  }
+
   deps = [
     "//base",
   ]
 
-  if (is_mac || is_win) {
+  if (use_crashpad) {
     sources += [ "crash_key_crashpad.cc" ]
     deps += [ "//third_party/crashpad/crashpad/client" ]
-  } else if (is_fuchsia) {
+  } else if (use_stubs) {
     sources += [ "crash_key_stubs.cc" ]
   } else {
     include_dirs = [ "//third_party/breakpad/breakpad/src/" ]

components/crash/core/common/crash_export.h

@@ -26,4 +26,9 @@
 #define CRASH_EXPORT
 #endif
 
+// See BUILD.gn :crash_key target for the declaration.
+#if !defined(CRASH_KEY_EXPORT)
+#define CRASH_KEY_EXPORT
+#endif
+
 #endif  // COMPONENTS_CRASH_CORE_COMMON_CRASH_EXPORT_H_

components/crash/core/common/crash_key.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 #include "build/build_config.h"
+#include "components/crash/core/common/crash_export.h"
 
 // The crash key interface exposed by this file is the same as the Crashpad
 // Annotation interface. Because not all platforms use Crashpad yet, a
@@ -61,7 +62,7 @@ constexpr size_t kCrashKeyStorageValueSize = 128;
 // Base implementation of a CrashKeyString for non-Crashpad clients. A separate
 // base class is used to avoid inlining complex logic into the public template
 // API.
-class CrashKeyStringImpl {
+class CRASH_KEY_EXPORT CrashKeyStringImpl {
  public:
   constexpr explicit CrashKeyStringImpl(const char name[],
                                         size_t* index_array,
@@ -178,8 +179,9 @@ namespace internal {
 // Formats a stack trace into a string whose length will not exceed
 // |max_length|. This function ensures no addresses are truncated when
 // being formatted.
-std::string FormatStackTrace(const base::debug::StackTrace& trace,
-                             size_t max_length);
+CRASH_KEY_EXPORT std::string FormatStackTrace(
+    const base::debug::StackTrace& trace,
+    size_t max_length);
 }  // namespace internal
 
 // Formats a base::debug::StackTrace as a string of space-separated hexadecimal
@@ -194,7 +196,7 @@ void SetCrashKeyStringToStackTrace(CrashKeyString<Size>* key,
 }
 
 // Initializes the crash key subsystem if it is required.
-void InitializeCrashKeys();
+CRASH_KEY_EXPORT void InitializeCrashKeys();
 
 }  // namespace crash_reporter
 

components/crash/core/common/crash_key_breakpad.cc

@@ -31,6 +31,9 @@ static TransitionalCrashKeyStorage* g_storage = nullptr;
 }  // namespace
 
 TransitionalCrashKeyStorage* GetCrashKeyStorage() {
+  if (!g_storage) {
+    g_storage = new internal::TransitionalCrashKeyStorage();
+  }
   return g_storage;
 }
 
@@ -98,10 +101,8 @@ bool CrashKeyStringImpl::is_set() const {
 }  // namespace internal
 
 void InitializeCrashKeys() {
-  if (!internal::g_storage) {
-    internal::g_storage = new internal::TransitionalCrashKeyStorage();
-    InitializeCrashKeyBaseSupport();
-  }
+  internal::GetCrashKeyStorage();
+  InitializeCrashKeyBaseSupport();
 }
 
 }  // namespace crash_reporter

components/crash/core/common/crash_key_internal.h

@@ -5,6 +5,7 @@
 #ifndef COMPONENTS_CRASH_CORE_COMMON_CRASH_KEY_INTERNAL_H_
 #define COMPONENTS_CRASH_CORE_COMMON_CRASH_KEY_INTERNAL_H_
 
+#include "components/crash/core/common/crash_export.h"
 #include "components/crash/core/common/crash_key.h"
 #include "third_party/breakpad/breakpad/src/common/simple_string_dictionary.h"
 
@@ -15,9 +16,9 @@ using TransitionalCrashKeyStorage = google_breakpad::
     NonAllocatingMap<40, kCrashKeyStorageValueSize, kCrashKeyStorageNumEntries>;
 
 // Accesses the underlying storage for crash keys for non-Crashpad clients.
-TransitionalCrashKeyStorage* GetCrashKeyStorage();
+CRASH_KEY_EXPORT TransitionalCrashKeyStorage* GetCrashKeyStorage();
 
-void ResetCrashKeyStorageForTesting();
+CRASH_KEY_EXPORT void ResetCrashKeyStorageForTesting();
 
 }  // namespace internal
 }  // namespace crash_reporter