Merge pull request #80 from supervacuus/meta/update

meta: update 2023-02-07

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

DEPS

@@ -14,7 +14,7 @@
 
 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]',

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
 }

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)

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",
@@ -147,14 +149,28 @@ static_library("common") {
   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") {
   testonly = true
 
   sources = [
     "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",

client/CMakeLists.txt

@@ -8,6 +8,8 @@ add_library(crashpad_client STATIC
     crashpad_client.h
     crashpad_info.cc
     crashpad_info.h
+    length_delimited_ring_buffer.h
+    ring_buffer_annotation.h
     prune_crash_reports.cc
     prune_crash_reports.h
     settings.cc

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.

client/annotation_test.cc

@@ -21,11 +21,58 @@
 #include "client/crashpad_info.h"
 #include "gtest/gtest.h"
 #include "test/gtest_death.h"
+#include "util/misc/clock.h"
+#include "util/synchronization/scoped_spin_guard.h"
+#include "util/thread/thread.h"
 
 namespace crashpad {
 namespace test {
 namespace {
 
+class SpinGuardAnnotation final : public Annotation {
+ public:
+  SpinGuardAnnotation(Annotation::Type type, const char name[])
+      : Annotation(type,
+                   name,
+                   &value_,
+                   ConcurrentAccessGuardMode::kScopedSpinGuard) {}
+
+  bool Set(bool value, uint64_t spin_guard_timeout_ns) {
+    auto guard = TryCreateScopedSpinGuard(spin_guard_timeout_ns);
+    if (!guard) {
+      return false;
+    }
+    value_ = value;
+    SetSize(sizeof(value_));
+    return true;
+  }
+
+ private:
+  bool value_;
+};
+
+class ScopedSpinGuardUnlockThread final : public Thread {
+ public:
+  ScopedSpinGuardUnlockThread(ScopedSpinGuard scoped_spin_guard,
+                              uint64_t sleep_time_ns)
+      : scoped_spin_guard_(std::move(scoped_spin_guard)),
+        sleep_time_ns_(sleep_time_ns) {}
+
+ private:
+  void ThreadMain() override {
+    SleepNanoseconds(sleep_time_ns_);
+
+    // Move the ScopedSpinGuard member into a local variable which is
+    // destroyed when ThreadMain() returns.
+    ScopedSpinGuard local_scoped_spin_guard(std::move(scoped_spin_guard_));
+
+    // After this point, local_scoped_spin_guard will be destroyed and unlocked.
+  }
+
+  ScopedSpinGuard scoped_spin_guard_;
+  const uint64_t sleep_time_ns_;
+};
+
 class Annotation : public testing::Test {
  public:
   void SetUp() override {
@@ -108,6 +155,61 @@ TEST_F(Annotation, StringType) {
   EXPECT_EQ("loooo", annotation.value());
 }
 
+TEST_F(Annotation, BaseAnnotationShouldNotSupportSpinGuard) {
+  char buffer[1024];
+  crashpad::Annotation annotation(
+      crashpad::Annotation::Type::kString, "no-spin-guard", buffer);
+  EXPECT_EQ(annotation.concurrent_access_guard_mode(),
+            crashpad::Annotation::ConcurrentAccessGuardMode::kUnguarded);
+#ifdef NDEBUG
+  // This fails a DCHECK() in debug builds, so only test it for NDEBUG builds.
+  EXPECT_EQ(std::nullopt, annotation.TryCreateScopedSpinGuard(0));
+#endif
+}
+
+TEST_F(Annotation, CustomAnnotationShouldSupportSpinGuardAndSet) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  EXPECT_EQ(spin_guard_annotation.concurrent_access_guard_mode(),
+            crashpad::Annotation::ConcurrentAccessGuardMode::kScopedSpinGuard);
+  EXPECT_TRUE(spin_guard_annotation.Set(true, 0));
+  EXPECT_EQ(1U, spin_guard_annotation.size());
+}
+
+TEST_F(Annotation, CustomAnnotationSetShouldFailIfRunConcurrently) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  auto guard = spin_guard_annotation.TryCreateScopedSpinGuard(0);
+  EXPECT_NE(std::nullopt, guard);
+  // This should fail, since the guard is already held and the timeout is 0.
+  EXPECT_FALSE(spin_guard_annotation.Set(true, 0));
+}
+
+TEST_F(Annotation,
+       CustomAnnotationSetShouldSucceedIfSpinGuardUnlockedAsynchronously) {
+  constexpr crashpad::Annotation::Type kType =
+      crashpad::Annotation::UserDefinedType(1);
+  SpinGuardAnnotation spin_guard_annotation(kType, "spin-guard");
+  auto guard = spin_guard_annotation.TryCreateScopedSpinGuard(0);
+  EXPECT_NE(std::nullopt, guard);
+  // Pass the guard off to a background thread which unlocks it after 1 ms.
+  constexpr uint64_t kSleepTimeNs = 1000000;  // 1 ms
+  ScopedSpinGuardUnlockThread unlock_thread(std::move(guard.value()),
+                                            kSleepTimeNs);
+  unlock_thread.Start();
+
+  // Try to set the annotation with a 100 ms timeout.
+  constexpr uint64_t kSpinGuardTimeoutNanos = 100000000;  // 100 ms
+
+  // This should succeed after 1 ms, since the timeout is much larger than the
+  // time the background thread holds the guard.
+  EXPECT_TRUE(spin_guard_annotation.Set(true, kSpinGuardTimeoutNanos));
+
+  unlock_thread.Join();
+}
+
 TEST(StringAnnotation, ArrayOfString) {
   static crashpad::StringAnnotation<4> annotations[] = {
       {"test-1", crashpad::StringAnnotation<4>::Tag::kArray},