From dd21b7b159ee1575fe6e21a82d9ff2a60f6feca0 Mon Sep 17 00:00:00 2001
From: Daniel Cheng <dcheng@chromium.org>
Date: Wed, 22 Apr 2026 15:43:36 -0700
Subject: [PATCH] Implement helper for a thread-safe shared cache of weakly-owned values

The core logic is taken from blink::AtomicStringTable; the key insight
is the final decrement to zero needs to be coordinated with the table to
avoid a race where one thread acquires a new reference just as another
thread is trying to drop the final reference. In the future, this may be
rewritten to take advantage of primitives from the standard library,
i.e. `std::shared_ptr` and `std::weak_ptr`.

Migrate the font service to use this new helper; in theory, it might be
possible to layer `blink::AtomicStringTable` on top of this helper as
well, but `base::AtomicStringTable` has a lot of additional complexity
to ensure atomicity of strings, so that is left as an exercise for a
future reader.

gemini-cli was used to expand test TODOs into actual tests as well as
suggest and implement additional test coverage.

Bug: 492374380
Change-Id: If903b247109e11be2fb6440297a59e78d20d4ac7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7706875
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Sean Maher <spvm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1619150}
---

diff --git a/base/BUILD.gn b/base/BUILD.gn
index c9967c8..9b20760 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -276,6 +276,7 @@
     "containers/to_vector.h",
     "containers/unique_ptr_adapters.h",
     "containers/vector_buffer.h",
+    "containers/weak_value_table.h",
     "critical_closure.h",
     "debug/alias.cc",
     "debug/alias.h",
@@ -3575,6 +3576,7 @@
     "containers/to_vector_unittest.cc",
     "containers/unique_ptr_adapters_unittest.cc",
     "containers/vector_buffer_unittest.cc",
+    "containers/weak_value_table_unittest.cc",
     "cpu_unittest.cc",
     "debug/alias_unittest.cc",
     "debug/asan_service_unittest.cc",
diff --git a/base/containers/weak_value_table.h b/base/containers/weak_value_table.h
new file mode 100644
index 0000000..a13f476
--- /dev/null
+++ b/base/containers/weak_value_table.h
@@ -0,0 +1,210 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_CONTAINERS_WEAK_VALUE_TABLE_H_
+#define BASE_CONTAINERS_WEAK_VALUE_TABLE_H_
+
+#include <atomic>
+#include <type_traits>
+
+#include "base/check.h"
+#include "base/dcheck_is_on.h"
+#include "base/memory/raw_ptr.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
+#include "third_party/abseil-cpp/absl/container/flat_hash_map.h"
+
+// A table that weakly owns thread-safe ref-counted values. The primary
+// supported operation is `FindOrCreate()`, which either:
+// - returns an owning reference to the value already associated with the key
+//   in the table if the key is already present or
+// - creates a new value, associates it with key in the table, and returns an
+//   owning reference to the new value.
+//
+// Value type implementations must:
+// - inherit from `RefCountedWeakValue<Key, Value>` using the CRTP.
+// - provide an idempotent `Getkey()` method that returns the key associated
+//   with the given value in the table–this is needed to remove the entry from
+//   the table when the refcount reaches zero.
+// - be constructed with `base::MakeRefCounted`, since the initial refcount
+//   starts at 1.
+
+namespace base::subtle {
+
+template <typename Key, typename ValueT>
+class WeakValueTable;
+
+template <typename Key, typename Value>
+class RefCountedWeakValue {
+ public:
+  REQUIRE_ADOPTION_FOR_REFCOUNTED_TYPE();
+
+  RefCountedWeakValue() = default;
+  ~RefCountedWeakValue() = default;
+
+  RefCountedWeakValue(const RefCountedWeakValue&) = delete;
+  RefCountedWeakValue& operator=(const RefCountedWeakValue&) = delete;
+
+  [[nodiscard]] bool HasOneRef() const {
+    return count_.load(std::memory_order_acquire) == 1;
+  }
+
+  void AddRef() {
+#if DCHECK_IS_ON()
+    DCHECK(!needs_adoption_);
+#endif
+    CHECK(count_.fetch_add(1, std::memory_order_relaxed) != 0);
+  }
+
+  void Release() {
+    size_t current = count_.load(std::memory_order_relaxed);
+    do {
+      // The last refcount needs to be released in coordination with `table_`
+      // to avoid races with `AddRef()`.
+      if (current == 1) {
+        static_cast<Value*>(this)->InternalTestHookForReleaseRace();
+        if (table_) {
+          // CRTP promises this cast is safe.
+          if (table_->ReleaseAndRemoveIfUnused(static_cast<Value*>(this))) {
+            // No race to acquire a new reference, so it's safe to delete
+            // `this`.
+            DeleteThis();
+          }
+        } else {
+          // Not inserted into the table yet, so no coordination needed; just
+          // delete `this` directly.
+          DeleteThis();
+        }
+        return;
+      }
+      // If the compare-and-swap fails, it updates `current` so no need to
+      // reload `current`.
+    } while (!count_.compare_exchange_weak(current, current - 1,
+                                           std::memory_order_acq_rel));
+  }
+
+  void Adopted() {
+#if DCHECK_IS_ON()
+    DCHECK(needs_adoption_);
+    needs_adoption_ = false;
+#endif
+  }
+
+  void InternalTestHookForReleaseRace() {}
+
+ private:
+  friend class WeakValueTable<Key, Value>;
+
+  // Returns `true` if the last ref was released.
+  [[nodiscard]] bool MaybeReleaseLastRef() {
+    return count_.fetch_sub(1, std::memory_order_acq_rel) == 1;
+  }
+
+  void DeleteThis() { delete static_cast<Value*>(this); }
+
+  std::atomic<size_t> count_ = 1;
+  raw_ptr<WeakValueTable<Key, Value>> table_;
+#if DCHECK_IS_ON()
+  bool needs_adoption_ = true;
+#endif
+};
+
+template <typename Key, typename ValueT>
+class WeakValueTable {
+  static_assert(std::is_base_of_v<RefCountedWeakValue<Key, ValueT>, ValueT>);
+
+ public:
+  WeakValueTable() = default;
+  ~WeakValueTable() { CHECK(empty()); }
+
+  WeakValueTable(const WeakValueTable&) = delete;
+  WeakValueTable& operator=(const WeakValueTable&) = delete;
+
+  // Finds and returns the value keyed to `key` in the table, or invokes
+  // `callable` to create a new value to associate with `key` in the table and
+  // returns the newly-created value. Creation can race on different threads,
+  // since the lock is dropped while invoking `callable`.
+  template <typename K, typename Callable>
+    requires requires(Callable&& callable) {
+      // The constraint is intentionally somewhat strict, since `ValueT*` is
+      // also convertible to `scoped_refptr<ValueT>`.
+      { callable() } -> std::same_as<scoped_refptr<ValueT>>;
+    }
+  [[nodiscard]] scoped_refptr<ValueT> FindOrCreate(K&& key,
+                                                   Callable&& callable) {
+    {
+      AutoLock lock(table_lock_);
+      auto it = table_.find(key);
+      if (it != table_.end()) {
+        return scoped_refptr<ValueT>(it->second.get());
+      }
+    }
+

diff --git a/base/containers/weak_value_table_unittest.cc b/base/containers/weak_value_table_unittest.cc
new file mode 100644
index 0000000..b2c20eb
--- /dev/null
+++ b/base/containers/weak_value_table_unittest.cc
@@ -0,0 +1,340 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/containers/weak_value_table.h"
+
+#include <optional>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "base/functional/callback.h"
+#include "base/functional/callback_helpers.h"
+#include "base/memory/raw_ptr.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/bind.h"
+#include "base/test/gtest_util.h"
+#include "base/threading/simple_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+
+using subtle::RefCountedWeakValue;
+using subtle::WeakValueTable;
+
+class TestValue : public RefCountedWeakValue<std::string, TestValue> {
+ public:
+  explicit TestValue(std::string key, OnceClosure on_destroy = NullCallback())
+      : key_(std::move(key)), on_destroy_(std::move(on_destroy)) {}
+
+  const std::string& GetKey() const { return key_; }
+
+  void set_on_release_race_closure(base::OnceClosure closure) {
+    on_release_race_ = std::move(closure);
+  }
+
+  void InternalTestHookForReleaseRace() {
+    if (on_release_race_) {
+      std::move(on_release_race_).Run();
+    }
+  }
+
+ private:
+  friend RefCountedWeakValue;
+
+  ~TestValue() {
+    if (on_destroy_) {
+      std::move(on_destroy_).Run();
+    }
+  }
+
+  std::string key_;
+  OnceClosure on_release_race_;
+  OnceClosure on_destroy_;
+};
+
+TEST(WeakValueTableTest, FindOrCreate) {
+  WeakValueTable<std::string, TestValue> table;
+
+  bool hello_was_destroyed = false;
+  bool world_was_destroyed = false;
+  scoped_refptr<TestValue> hello;
+
+  {
+    bool called = false;
+    hello = table.FindOrCreate("hello", [&] {
+      called = true;
+      return MakeRefCounted<TestValue>(
+          "hello", BindLambdaForTesting([&] { hello_was_destroyed = true; }));
+    });
+    EXPECT_TRUE(called);
+    EXPECT_EQ("hello", hello->GetKey());
+  }
+
+  // The callable should not be invoked when trying to insert another value with
+  // the same key; the value already in the table should be returned instead.
+  {
+    bool called = false;
+    scoped_refptr<TestValue> hello2 =
+        table.FindOrCreate("hello", [&] -> scoped_refptr<TestValue> {
+          called = true;
+          return nullptr;
+        });
+    EXPECT_FALSE(called);
+    EXPECT_EQ(hello.get(), hello2.get());
+  }
+
+  {
+    bool called = false;
+    scoped_refptr<TestValue> world = table.FindOrCreate("world", [&] {
+      called = true;
+      return MakeRefCounted<TestValue>(
+          "world", BindLambdaForTesting([&] { world_was_destroyed = true; }));
+    });
+    EXPECT_TRUE(called);
+    EXPECT_EQ("world", world->GetKey());
+
+    EXPECT_FALSE(table.empty());
+    EXPECT_FALSE(world_was_destroyed);
+    world = nullptr;
+    EXPECT_FALSE(table.empty());
+    EXPECT_TRUE(world_was_destroyed);
+  }
+
+  EXPECT_FALSE(table.empty());
+  EXPECT_FALSE(hello_was_destroyed);
+  hello = nullptr;
+  EXPECT_TRUE(table.empty());
+  EXPECT_TRUE(hello_was_destroyed);
+}
+
+TEST(WeakValueTableTest, FindOrCreateNullptr) {
+  WeakValueTable<std::string, TestValue> table;
+
+  {
+    bool called = false;
+    scoped_refptr<TestValue> value =
+        table.FindOrCreate("hello", [&] -> scoped_refptr<TestValue> {
+          called = true;
+          return nullptr;
+        });
+    EXPECT_TRUE(called);
+    EXPECT_EQ(nullptr, value);
+  }
+
+  // The first insert failed since the callable returned nullptr, so the second
+  // insert should invoke the callable.
+  {
+    bool called = false;
+    scoped_refptr<TestValue> value = table.FindOrCreate("hello", [&] {
+      called = true;
+      return MakeRefCounted<TestValue>("hello");
+    });
+    EXPECT_TRUE(called);
+    EXPECT_EQ("hello", value->GetKey());
+  }
+}
+
+TEST(WeakValueTableTest, DestroyNeverInsertedValue) {
+  base::MakeRefCounted<TestValue>("hello");
+}
+
+TEST(WeakValueTableTest, RaceToCreate) {
+  WeakValueTable<std::string, TestValue> table;
+
+  bool first_destroyed = false;
+  bool second_destroyed = false;
+  scoped_refptr<TestValue> keep_alive;
+
+  scoped_refptr<TestValue> value = table.FindOrCreate("hello", [&] {
+    auto first = MakeRefCounted<TestValue>(
+        "hello", BindLambdaForTesting([&] { first_destroyed = true; }));
+    // While inserting the first element, reentrantly insert the second one.
+    auto second = table.FindOrCreate("hello", [&] {
+      return MakeRefCounted<TestValue>(
+          "hello", BindLambdaForTesting([&] { second_destroyed = true; }));
+    });
+    // Both objects are still live and should have distinct addresses.
+    EXPECT_NE(first, second);
+    // Allow the reference to escape; otherwise, `second` will be destroyed
+    // and removed from the table before returning to the first
+    // `FindOrCreate()` call.
+    keep_alive = std::move(second);
+    return first;
+  });
+
+  EXPECT_FALSE(table.empty());
+  EXPECT_TRUE(first_destroyed);
+  EXPECT_FALSE(second_destroyed);
+  // At this point, both `keep_alive` and `value` should be the same object.
+  EXPECT_EQ(keep_alive, value);
+  EXPECT_FALSE(keep_alive->HasOneRef());
+  value = nullptr;
+  EXPECT_TRUE(keep_alive->HasOneRef());
+  keep_alive = nullptr;
+  EXPECT_TRUE(second_destroyed);
+  EXPECT_TRUE(table.empty());
+}
+
+TEST(WeakValueTableTest, RaceRemovalAndFind) {
+  WeakValueTable<std::string, TestValue> table;
+
+  bool was_destroyed = false;
+  scoped_refptr<TestValue> keep_alive;
+
+  scoped_refptr<TestValue> value = table.FindOrCreate("hello", [&] {
+    auto value = base::MakeRefCounted<TestValue>(
+        "hello", base::BindLambdaForTesting([&] { was_destroyed = true; }));
+    value->set_on_release_race_closure(base::BindLambdaForTesting([&] {
+      bool called = false;
+      keep_alive = table.FindOrCreate("hello", [&] -> scoped_refptr<TestValue> {
+        called = true;
+        return nullptr;
+      });
+      EXPECT_FALSE(called);
+    }));
+    return value;
+  });
+
+  EXPECT_FALSE(table.empty());
+  EXPECT_FALSE(was_destroyed);
+  EXPECT_TRUE(value->HasOneRef());
+  EXPECT_EQ(nullptr, keep_alive);
+  // Trigger the race by releasing the last reference. The on release race
+  // callback bound earlier will take a reference right before the value tries
+  // to release it's last reference, so the end result should be that the value
+  // is not destroyed yet.
+  value = nullptr;
+  EXPECT_FALSE(table.empty());
+  EXPECT_FALSE(was_destroyed);
+  ASSERT_TRUE(!!keep_alive);
+  EXPECT_TRUE(keep_alive->HasOneRef());
+  keep_alive = nullptr;
+  EXPECT_TRUE(table.empty());
+  EXPECT_TRUE(was_destroyed);
+}
+
+TEST(WeakValueTableTest, CreateDuringValueDestruction) {
+  WeakValueTable<std::string, TestValue> table;
+
+  bool first_destroyed = false;
+  bool second_destroyed = false;
+  scoped_refptr<TestValue> second;
+
+  scoped_refptr<TestValue> first = table.FindOrCreate("hello", [&] {
+    return MakeRefCounted<TestValue>(
+        "hello", BindLambdaForTesting([&] {
+          first_destroyed = true;
+          second = table.FindOrCreate("hello", [&] {
+            return MakeRefCounted<TestValue>("hello", BindLambdaForTesting([&] {
+                                               second_destroyed = true;
+                                             }));
+          });
+        }));
+  });
+
+  EXPECT_FALSE(first_destroyed);
+  EXPECT_FALSE(second_destroyed);
+  first = nullptr;
+  EXPECT_TRUE(first_destroyed);
+  EXPECT_FALSE(second_destroyed);
+  second = nullptr;
+  EXPECT_TRUE(second_destroyed);
+}
+
+TEST(WeakValueTableTest, DestroyNonEmptyTable) {
+  std::optional<WeakValueTable<std::string, TestValue>> table(std::in_place);
+  scoped_refptr<TestValue> value = table->FindOrCreate(
+      "hello", [] { return MakeRefCounted<TestValue>("hello"); });
+
+  // `value` is still live
+  EXPECT_FALSE(table->empty());
+  // Destroying a non-empty table is unsafe so crash instead.
+  EXPECT_CHECK_DEATH(table.reset());
+}
+
+TEST(WeakValueTableTest, CallableReturnsPointerWithMultipleRefs) {
+  WeakValueTable<std::string, TestValue> table;
+  EXPECT_CHECK_DEATH({
+    scoped_refptr<TestValue> extra_ref;
+    (void)table.FindOrCreate("hello", [&] {
+      auto value = MakeRefCounted<TestValue>("hello");
+      extra_ref = value;
+      return value;
+    });
+  });
+}
+
+TEST(WeakValueTableTest, ReentrantCreateDifferentKey) {
+  WeakValueTable<std::string, TestValue> table;
+
+  scoped_refptr<TestValue> first = table.FindOrCreate("hello", [&] {
+    // Re-entrantly insert a different key while constructing "hello"
+    scoped_refptr<TestValue> second = table.FindOrCreate(
+        "world", [] { return MakeRefCounted<TestValue>("world"); });
+    EXPECT_EQ("world", second->GetKey());
+    return MakeRefCounted<TestValue>("hello");
+  });
+
+  EXPECT_EQ("hello", first->GetKey());
+  EXPECT_FALSE(table.empty());
+}
+
+TEST(WeakValueTableTest, MismatchedKey) {
+  WeakValueTable<std::string, TestValue> table;
+  // This should crash because TestValue::GetKey() will return "world", which
+  // does not match with the originally-requested key "hello".
+  EXPECT_CHECK_DEATH((void)table.FindOrCreate(
+      "hello", [] { return MakeRefCounted<TestValue>("world"); }));
+}
+
+namespace {
+
... (truncated)