Merge remote-tracking branch 'google/main' into kq-merge-march-2023

Author: KrzaQ

Diff for: .gitignore

@@ -36,6 +36,11 @@
 /third_party/linux/clang
 /third_party/linux/sysroot
 /third_party/gyp/gyp
+/third_party/mini_chromium/mini_chromium
+/third_party/ninja/linux
+/third_party/ninja/mac*
+/third_party/ninja/ninja.exe
+/third_party/zlib/zlib
 /xcodebuild
 tag
 /cbuild

Diff for: BUILD.gn

@@ -39,6 +39,8 @@ if (crashpad_is_in_chromium || crashpad_is_in_fuchsia) {
     if (crashpad_is_in_fuchsia) {
       # TODO(fuchsia:46559): Fix the leaks and remove this.
       deps += [ "//build/config/sanitizers:suppress-lsan.DO-NOT-USE-THIS" ]
+      # TODO(fxbug.dev/108368): Remove this once the underlying issue is addressed.
+      exclude_toolchain_tags = [ "hwasan" ]
     }
     if (crashpad_is_android) {
       use_raw_android_executable = true

Diff for: DEPS

@@ -14,7 +14,10 @@
 
 vars = {
   'chromium_git': 'https://chromium.googlesource.com',
-  'gn_version': 'git_revision:2ecd43a10266bd091c98e6dcde507c64f6a0dad3',
+  'gn_version': 'git_revision:5e19d2fb166fbd4f6f32147fbb2f497091a54ad8',
+  # ninja CIPD package version.
+  # https://chrome-infra-packages.appspot.com/p/infra/3pp/tools/ninja
+  'ninja_version': 'version:[email protected]',
   'pull_linux_clang': False,
   'pull_win_toolchain': False,
   # Controls whether crashpad/build/ios/setup-ios-gn.py is run as part of
@@ -133,6 +136,51 @@ deps = {
     'condition': 'checkout_fuchsia and host_os == "linux"',
     'dep_type': 'cipd'
   },
+  # depot_tools/ninja wrapper calls third_party/ninja/{ninja, ninja.exe}.
+  # crashpad/third_party/ninja/ninja is another wrapper to call linux ninja
+  # or mac ninja.
+  # This allows crashpad developers to work for multiple platforms on the same
+  # machine.
+  'crashpad/third_party/ninja': {
+    'packages': [
+      {
+        'package': 'infra/3pp/tools/ninja/${{platform}}',
+        'version': Var('ninja_version'),
+      }
+    ],
+    'condition': 'host_os == "win"',
+    'dep_type': 'cipd',
+  },
+  'crashpad/third_party/ninja/linux': {
+    'packages': [
+      {
+        'package': 'infra/3pp/tools/ninja/${{platform}}',
+        'version': Var('ninja_version'),
+      }
+    ],
+    'condition': 'host_os == "linux"',
+    'dep_type': 'cipd',
+  },
+  'crashpad/third_party/ninja/mac-amd64': {
+    'packages': [
+      {
+        'package': 'infra/3pp/tools/ninja/mac-amd64',
+        'version': Var('ninja_version'),
+      }
+    ],
+    'condition': 'host_os == "mac" and host_cpu == "x64"',
+    'dep_type': 'cipd',
+  },
+  'crashpad/third_party/ninja/mac-arm64': {
+    'packages': [
+      {
+        'package': 'infra/3pp/tools/ninja/mac-arm64',
+        'version': Var('ninja_version'),
+      }
+    ],
+    'condition': 'host_os == "mac" and host_cpu == "arm64"',
+    'dep_type': 'cipd',
+  },
   'crashpad/third_party/win/toolchain': {
     # This package is only updated when the solution in .gclient includes an
     # entry like:

Diff for: build/BUILD.gn

@@ -30,6 +30,12 @@ config("crashpad_is_in_fuchsia") {
   }
 }
 
+config("flock_always_supported_defines") {
+  defines = [
+    "CRASHPAD_FLOCK_ALWAYS_SUPPORTED=$crashpad_flock_always_supported",
+  ]
+}
+
 group("default_exe_manifest_win") {
   if (crashpad_is_in_chromium) {
     deps = [ "//build/win:default_exe_manifest" ]

Diff for: build/crashpad_buildconfig.gni

@@ -37,7 +37,9 @@ crashpad_is_standalone = crashpad_dependencies == "standalone"
 # This is the parent directory that contains the mini_chromium source dir.
 # This variable is not used when crashpad_is_in_chromium.
 if (crashpad_is_in_fuchsia) {
-  mini_chromium_source_parent = "//third_party/crashpad/third_party/mini_chromium"
+  import("//third_party/crashpad/fuchsia_buildconfig.gni")
+  mini_chromium_source_parent =
+      fuchsia_crashpad_root + "/third_party/mini_chromium"
 } else {
   mini_chromium_source_parent = "../third_party/mini_chromium"
 }
@@ -49,7 +51,7 @@ _mini_chromium_source_root = "$mini_chromium_source_parent/mini_chromium"
 if (crashpad_is_external || crashpad_is_in_dart) {
   mini_chromium_import_root = "../../../$_mini_chromium_source_root"
 } else if (crashpad_is_in_fuchsia) {
-  mini_chromium_import_root = "//third_party/mini_chromium"
+  mini_chromium_import_root = fuchsia_mini_chromium_root
 } else {
   mini_chromium_import_root = _mini_chromium_source_root
 }
@@ -81,6 +83,8 @@ if (crashpad_is_in_chromium) {
   crashpad_is_clang = mini_chromium_is_clang
 }
 
+crashpad_flock_always_supported = !(crashpad_is_android || crashpad_is_fuchsia)
+
 template("crashpad_executable") {
   executable(target_name) {
     forward_variables_from(invoker,

Diff for: build/run_tests.py

@@ -312,10 +312,10 @@ def _adb_shell(command_args, env={}):
         _adb_shell(['rm', '-rf', device_temp_dir])
 
 
-def _RunOnIOSTarget(binary_dir, test, is_xcuitest=False):
+def _RunOnIOSTarget(binary_dir, test, is_xcuitest=False, gtest_filter=None):
     """Runs the given iOS |test| app on iPhone 8 with the default OS version."""
 
-    def xctest(binary_dir, test):
+    def xctest(binary_dir, test, gtest_filter=None):
         """Returns a dict containing the xctestrun data needed to run an
         XCTest-based test app."""
         test_path = os.path.join(CRASHPAD_DIR, binary_dir)
@@ -332,6 +332,8 @@ def xctest(binary_dir, test):
                 'XCInjectBundleInto': '__TESTHOST__/' + test,
             }
         }
+        if gtest_filter:
+            module_data['CommandLineArguments'] = ['--gtest_filter='+gtest_filter]
         return {test: module_data}
 
     def xcuitest(binary_dir, test):
@@ -368,16 +370,18 @@ def xcuitest(binary_dir, test):
 
         xctestrun_path = f.name
         print(xctestrun_path)
+        command = [
+            'xcodebuild', 'test-without-building', '-xctestrun', xctestrun_path,
+            '-destination', 'platform=iOS Simulator,name=iPhone 8',
+        ]
         with open(xctestrun_path, 'wb') as fp:
             if is_xcuitest:
                 plistlib.dump(xcuitest(binary_dir, test), fp)
+                if gtest_filter:
+                    command.append('-only-testing:' + test + '/' + gtest_filter)
             else:
-                plistlib.dump(xctest(binary_dir, test), fp)
-
-        subprocess.check_call([
-            'xcodebuild', 'test-without-building', '-xctestrun', xctestrun_path,
-            '-destination', 'platform=iOS Simulator,name=iPhone 8'
-        ])
+                plistlib.dump(xctest(binary_dir, test, gtest_filter), fp)
+        subprocess.check_call(command)
 
 
 # This script is primarily used from the waterfall so that the list of tests
@@ -468,7 +472,8 @@ def main(args):
             elif is_ios:
                 _RunOnIOSTarget(args.binary_dir,
                                 test,
-                                is_xcuitest=test.startswith('ios'))
+                                is_xcuitest=test.startswith('ios'),
+                                gtest_filter=args.gtest_filter)
             else:
                 subprocess.check_call([os.path.join(args.binary_dir, test)] +
                                       extra_command_line)

Diff for: client/BUILD.gn

@@ -119,6 +119,8 @@ static_library("common") {
     "crash_report_database.h",
     "crashpad_info.cc",
     "crashpad_info.h",
+    "length_delimited_ring_buffer.h",
+    "ring_buffer_annotation.h",
     "settings.cc",
     "settings.h",
     "simple_address_range_bag.h",
@@ -144,6 +146,19 @@ static_library("common") {
     "../util",
   ]
   deps = [ "../util" ]
+  configs += [ "../build:flock_always_supported_defines" ]
+}
+
+crashpad_executable("ring_buffer_annotation_load_test") {
+  testonly = true
+  sources = [
+    "ring_buffer_annotation_load_test_main.cc",
+  ]
+  deps = [
+    ":client",
+    "../tools:tool_support",
+    "$mini_chromium_source_parent:base",
+  ]
 }
 
 source_set("client_test") {
@@ -153,7 +168,9 @@ source_set("client_test") {
     "annotation_list_test.cc",
     "annotation_test.cc",
     "crash_report_database_test.cc",
+    "length_delimited_ring_buffer_test.cc",
     "prune_crash_reports_test.cc",
+    "ring_buffer_annotation_test.cc",
     "settings_test.cc",
     "simple_address_range_bag_test.cc",
     "simple_string_dictionary_test.cc",

Diff for: client/CMakeLists.txt

@@ -55,4 +55,3 @@ target_include_directories(client PUBLIC ..)
 if (LINUX)
     target_link_libraries(client PUBLIC Threads::Threads)
 endif (LINUX)
-

Diff for: client/annotation.h

@@ -17,6 +17,7 @@
 
 #include <algorithm>
 #include <atomic>
+#include <optional>
 
 #include <stdint.h>
 #include <string.h>
@@ -26,6 +27,7 @@
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_piece.h"
 #include "build/build_config.h"
+#include "util/synchronization/scoped_spin_guard.h"
 
 namespace crashpad {
 #if BUILDFLAG(IS_IOS)
@@ -94,6 +96,20 @@ class Annotation {
     kUserDefinedStart = 0x8000,
   };
 
+  //! \brief Mode used to guard concurrent reads from writes.
+  enum class ConcurrentAccessGuardMode : bool {
+    //! \!brief Annotation does not guard reads from concurrent
+    //!     writes. Annotation values can be corrupted if the process crashes
+    //!     mid-write and the handler tries to read from the Annotation while
+    //!     being written to.
+    kUnguarded = false,
+
+    //! \!brief Annotation guards reads from concurrent writes using
+    //!     ScopedSpinGuard. Clients must use TryCreateScopedSpinGuard()
+    //!     before reading or writing the data in this Annotation.
+    kScopedSpinGuard = true,
+  };
+
   //! \brief Creates a user-defined Annotation::Type.
   //!
   //! This exists to remove the casting overhead of `enum class`.
@@ -132,12 +148,11 @@ class Annotation {
   //! \param[in] value_ptr A pointer to the value for the annotation. The
   //!     pointer may not be changed once associated with an annotation, but
   //!     the data may be mutated.
-  constexpr Annotation(Type type, const char name[], void* const value_ptr)
-      : link_node_(nullptr),
-        name_(name),
-        value_ptr_(value_ptr),
-        size_(0),
-        type_(type) {}
+  constexpr Annotation(Type type, const char name[], void* value_ptr)
+      : Annotation(type,
+                   name,
+                   value_ptr,
+                   ConcurrentAccessGuardMode::kUnguarded) {}
 
   Annotation(const Annotation&) = delete;
   Annotation& operator=(const Annotation&) = delete;
@@ -172,7 +187,58 @@ class Annotation {
   const char* name() const { return name_; }
   const void* value() const { return value_ptr_; }
 
+  ConcurrentAccessGuardMode concurrent_access_guard_mode() const {
+    return concurrent_access_guard_mode_;
+  }
+
+  //! \brief If this Annotation guards concurrent access using ScopedSpinGuard,
+  //!     tries to obtain the spin guard and returns the result.
+  //!
+  //! \param[in] timeout_ns The timeout in nanoseconds after which to give up
+  //!     trying to obtain the spin guard.
+  //! \return std::nullopt if the spin guard could not be obtained within
+  //!     timeout_ns, or the obtained spin guard otherwise.
+  std::optional<ScopedSpinGuard> TryCreateScopedSpinGuard(uint64_t timeout_ns) {
+    // This can't use DCHECK_EQ() because ostream doesn't support
+    // operator<<(bool).
+    DCHECK(concurrent_access_guard_mode_ ==
+           ConcurrentAccessGuardMode::kScopedSpinGuard);
+    if (concurrent_access_guard_mode_ ==
+        ConcurrentAccessGuardMode::kUnguarded) {
+      return std::nullopt;
+    }
+    return ScopedSpinGuard::TryCreateScopedSpinGuard(timeout_ns,
+                                                     spin_guard_state_);
+  }
+
  protected:
+  //! \brief Constructs a new annotation.
+  //!
+  //! Upon construction, the annotation will not be included in any crash
+  //! reports until \sa SetSize() is called with a value greater than `0`.
+  //!
+  //! \param[in] type The data type of the value of the annotation.
+  //! \param[in] name A `NUL`-terminated C-string name for the annotation. Names
+  //!     do not have to be unique, though not all crash processors may handle
+  //!     Annotations with the same name. Names should be constexpr data with
+  //!     static storage duration.
+  //! \param[in] value_ptr A pointer to the value for the annotation. The
+  //!     pointer may not be changed once associated with an annotation, but
+  //!     the data may be mutated.
+  //! \param[in] concurrent_access_guard_mode Mode used to guard concurrent
+  //!     reads from writes.
+  constexpr Annotation(Type type,
+                       const char name[],
+                       void* value_ptr,
+                       ConcurrentAccessGuardMode concurrent_access_guard_mode)
+      : link_node_(nullptr),
+        name_(name),
+        value_ptr_(value_ptr),
+        size_(0),
+        type_(type),
+        concurrent_access_guard_mode_(concurrent_access_guard_mode),
+        spin_guard_state_() {}
+
   friend class AnnotationList;
 #if BUILDFLAG(IS_IOS)
   friend class internal::InProcessIntermediateDumpHandler;
@@ -192,6 +258,11 @@ class Annotation {
   void* const value_ptr_;
   ValueSizeType size_;
   const Type type_;
+
+  //! \brief Mode used to guard concurrent reads from writes.
+  const ConcurrentAccessGuardMode concurrent_access_guard_mode_;
+
+  SpinGuardState spin_guard_state_;
 };
 
 //! \brief An \sa Annotation that stores a `NUL`-terminated C-string value.