From 7bcc0ec5dd38106192174dc2ad9f59d4217158b5 Mon Sep 17 00:00:00 2001
From: Dominik Röttsches <drott@chromium.org>
Date: Mon, 14 Sep 2026 02:50:38 -0700
Subject: [PATCH] Sequentialize access to callbacks in FontUniqueNameLookup

Serialize callback registrations, iterations, and drains onto a
dedicated base::SequencedTaskRunner (callback_access_runner_) to
eliminate race conditions around access to pending_callbacks_.

Add FontUniqueNameLookupCallbackTest.ConcurrentCallbackQueuing to verify
safe concurrent callback queuing and dispatch across ThreadPool tasks
while asynchronous table generation and drainage are in progress.

Bug: 492374387
Change-Id: Iee5e0d761bf82c38df5d0d0d14945f6ced3e912a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8379663
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Francois Pierre Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1697142}
---
diff --git a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.cc b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.cc
index f356b3b..d87bb37 100644
--- a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.cc
+++ b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.cc
@@ -124,7 +124,13 @@
 
 FontUniqueNameLookup::FontUniqueNameLookup(
     const base::FilePath& cache_directory)
-    : cache_directory_(cache_directory) {
+    : callback_access_runner_(base::ThreadPool::CreateSequencedTaskRunner(
+          {base::TaskPriority::USER_BLOCKING,
+           base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
+      cache_directory_(cache_directory) {
+  CHECK(callback_access_runner_);
+  DETACH_FROM_SEQUENCE(callback_sequence_checker_);
+
   if (!DirectoryExists(cache_directory_) ||
       !base::PathIsWritable(cache_directory_)) {
     DCHECK(false) << "Error accessing cache directory for writing: "
@@ -145,6 +151,32 @@
     scoped_refptr<base::SequencedTaskRunner> task_runner,
     blink::mojom::FontUniqueNameLookup::GetUniqueNameLookupTableCallback
         callback) {
+  if (proto_storage_ready_.IsSignaled()) {
+    task_runner->PostTask(FROM_HERE, base::BindOnce(std::move(callback),
+                                                    DuplicateMemoryRegion()));
+    return;
+  }
+
+  CHECK(callback_access_runner_);
+  callback_access_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&FontUniqueNameLookup::EnqueueCallbackOnSequence,
+                     base::Unretained(this), std::move(task_runner),
+                     std::move(callback)));
+}
+
+void FontUniqueNameLookup::EnqueueCallbackOnSequence(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    blink::mojom::FontUniqueNameLookup::GetUniqueNameLookupTableCallback
+        callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(callback_sequence_checker_);
+
+  if (proto_storage_ready_.IsSignaled()) {
+    task_runner->PostTask(FROM_HERE, base::BindOnce(std::move(callback),
+                                                    DuplicateMemoryRegion()));
+    return;
+  }
+
   pending_callbacks_.emplace_back(std::move(task_runner), std::move(callback));
 }
 
@@ -210,6 +242,7 @@
 }
 
 void FontUniqueNameLookup::ScheduleLoadOrUpdateTable() {
+  CHECK(callback_access_runner_);
   base::ThreadPool::PostTask(
       FROM_HERE,
       {base::MayBlock(), base::TaskPriority::BEST_EFFORT,
@@ -229,7 +262,11 @@
               instance->PersistToFile();
             }
             instance->proto_storage_ready_.Signal();
-            instance->PostCallbacks();
+            // Safe to use base::Unretained(instance) here because instance
+            // is a never-deleted singleton.
+            instance->callback_access_runner_->PostTask(
+                FROM_HERE, base::BindOnce(&FontUniqueNameLookup::PostCallbacks,
+                                          base::Unretained(instance)));
           },
           base::Unretained(this)));
 }
@@ -283,6 +320,9 @@
 FontUniqueNameLookup::CallbackOnTaskRunner::~CallbackOnTaskRunner() = default;
 
 void FontUniqueNameLookup::PostCallbacks() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(callback_sequence_checker_);
+  CHECK(proto_storage_ready_.IsSignaled());
+
   for (auto& pending_callback : pending_callbacks_) {
     pending_callback.task_runner->PostTask(
         FROM_HERE, base::BindOnce(std::move(pending_callback.mojo_callback),
diff --git a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.h b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.h
index 649659a..cf8ad31 100644
--- a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.h
+++ b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android.h
@@ -10,8 +10,10 @@
 
 #include "base/files/file_path.h"
 #include "base/memory/read_only_shared_memory_region.h"
+#include "base/sequence_checker.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/task/sequenced_task_runner.h"
+#include "base/thread_annotations.h"
 #include "content/common/content_export.h"
 #include "third_party/blink/public/mojom/font_unique_name_lookup/font_unique_name_lookup.mojom.h"
 
@@ -114,8 +116,15 @@
 
   base::FilePath TableCacheFilePath();
 
+  // Dispatches all queued callbacks; runs exclusively on
+  // callback_access_runner_.
   void PostCallbacks();
 
+  void EnqueueCallbackOnSequence(
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      blink::mojom::FontUniqueNameLookup::GetUniqueNameLookupTableCallback
+          callback);
+
   // We have a asynchronous update tasks which need write access to the
   // proto_storage_ MappedReadOnlyRegion after reading the index file from disk,
   // or after scanning and indexing metadata from font files. At the same time,
@@ -126,6 +135,10 @@
   base::WaitableEvent proto_storage_ready_;
   base::MappedReadOnlyRegion proto_storage_;
 
+  // Sequence runner dedicated to serializing pending_callbacks_ access:
+  scoped_refptr<base::SequencedTaskRunner> callback_access_runner_;
+  SEQUENCE_CHECKER(callback_sequence_checker_);
+
   base::FilePath cache_directory_;
   std::string android_build_fingerprint_for_testing_;
   std::vector<base::FilePath> font_file_paths_for_testing_ =
@@ -142,7 +155,8 @@
         mojo_callback;
   };
 
-  std::vector<CallbackOnTaskRunner> pending_callbacks_;
+  std::vector<CallbackOnTaskRunner> pending_callbacks_
+      GUARDED_BY_CONTEXT(callback_sequence_checker_);
 };
 }  // namespace content
 
diff --git a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
index 4726b7f0..a3c2141 100644
--- a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
+++ b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
@@ -8,12 +8,18 @@
 #include <memory>
 
 #include "base/android/android_info.h"
+#include "base/barrier_closure.h"
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/functional/function_ref.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
+#include "base/test/task_environment.h"
 #include "content/browser/font_unique_name_lookup/name_table_ffi.rs.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/font_unique_name_lookup/font_table_matcher.h"
@@ -79,6 +85,7 @@
         std::make_unique<FontUniqueNameLookup>(temp_dir_.GetPath());
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
 };
@@ -311,6 +318,7 @@
         font_file_corruptor_.GetFontFilesList());
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   FontFileCorruptor font_file_corruptor_;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
@@ -343,6 +351,7 @@
     font_unique_name_lookup_->SetAndroidBuildFingerprintForTesting("A");
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir lookup_table_storage_dir;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
 };
@@ -364,4 +373,94 @@
   ASSERT_TRUE(matcher_initial.FontListIsDisjointFrom(matcher_second_half));
 }
 
+class TestFontUniqueNameLookup : public FontUniqueNameLookup {
+ public:
+  using FontUniqueNameLookup::FontUniqueNameLookup;
+  using FontUniqueNameLookup::ScheduleLoadOrUpdateTable;
+};
+
+class FontUniqueNameLookupCallbackTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    lookup_ = std::make_unique<TestFontUniqueNameLookup>(temp_dir_.GetPath());
+    std::vector<base::FilePath> font_files = AndroidFontFilesList();
+    ASSERT_FALSE(font_files.empty());
+    // Restrict to a single font to minimize test execution time and CPU load.
+    lookup_->SetFontFilePathsForTesting({font_files.front()});
+  }
+
+  base::test::TaskEnvironment task_environment_;
+  base::ScopedTempDir temp_dir_;
+  std::unique_ptr<TestFontUniqueNameLookup> lookup_;
+};
+
+TEST_F(FontUniqueNameLookupCallbackTest, ConcurrentCallbackQueuing) {
+  constexpr size_t kPreQueueCount = 10;
+  constexpr size_t kConcurrentThreads = 4;
+  constexpr size_t kPerThreadQueueCount = 5;
+  constexpr size_t kTotalCallbacks =
+      kPreQueueCount + (kConcurrentThreads * kPerThreadQueueCount);
+
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(kTotalCallbacks, run_loop.QuitClosure());
+
+  auto make_callback = [&]() {
+    return base::BindLambdaForTesting(
+        [&, barrier](base::ReadOnlySharedMemoryRegion region) {
+          EXPECT_TRUE(region.IsValid());
+          base::ReadOnlySharedMemoryMapping mapping = region.Map();
+          EXPECT_TRUE(mapping.IsValid());
+          blink::FontTableMatcher matcher(mapping);
+          EXPECT_GT(matcher.AvailableFonts(), 0u);
+          barrier.Run();
+        });
+  };
+
+  // Phase A: Pre-queue callbacks before table build starts.
+  // proto_storage_ready_ is guaranteed to be unsignaled, ensuring all callbacks
+  // are routed to callback_access_runner_ and enqueued in pending_callbacks_.
+  for (size_t i = 0; i < kPreQueueCount; ++i) {
+    scoped_refptr<base::SequencedTaskRunner> caller_runner =
+        base::ThreadPool::CreateSequencedTaskRunner({});
+    lookup_->QueueShareMemoryRegionWhenReady(caller_runner, make_callback());
+  }
+
+  // Start asynchronous table build on ThreadPool.
+  lookup_->ScheduleLoadOrUpdateTable();
+
+  // Phase B: Concurrently queue callbacks from multiple threads while table
+  // build and drain are in-flight.
+  for (size_t i = 0; i < kConcurrentThreads; ++i) {
+    base::ThreadPool::PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          for (size_t j = 0; j < kPerThreadQueueCount; ++j) {
+            scoped_refptr<base::SequencedTaskRunner> caller_runner =
+                base::ThreadPool::CreateSequencedTaskRunner({});
+            lookup_->QueueShareMemoryRegionWhenReady(caller_runner,
+                                                     make_callback());
+          }
+        }));
+  }
+
+  // Wait for all pre-queued and concurrently queued callbacks to complete.
+  run_loop.Run();
+
+  // Phase C: Late / fast-path queueing verification.
+  // Callbacks queued after the table is built should be dispatched directly.
+  base::RunLoop late_run_loop;
+  lookup_->QueueShareMemoryRegionWhenReady(
+      base::SequencedTaskRunner::GetCurrentDefault(),
+      base::BindLambdaForTesting([&](base::ReadOnlySharedMemoryRegion region) {
+        EXPECT_TRUE(region.IsValid());
+        base::ReadOnlySharedMemoryMapping mapping = region.Map();
+        EXPECT_TRUE(mapping.IsValid());
+        blink::FontTableMatcher matcher(mapping);
+        EXPECT_GT(matcher.AvailableFonts(), 0u);
+        late_run_loop.Quit();
+      }));
+  late_run_loop.Run();
+}
+
 }  // namespace content

diff --git a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
index 4726b7f0..a3c2141 100644
--- a/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
+++ b/content/browser/font_unique_name_lookup/font_unique_name_lookup_android_unittest.cc
@@ -8,12 +8,18 @@
 #include <memory>
 
 #include "base/android/android_info.h"
+#include "base/barrier_closure.h"
 #include "base/files/file.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/functional/function_ref.h"
+#include "base/run_loop.h"
 #include "base/strings/string_util.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
+#include "base/test/task_environment.h"
 #include "content/browser/font_unique_name_lookup/name_table_ffi.rs.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/common/font_unique_name_lookup/font_table_matcher.h"
@@ -79,6 +85,7 @@
         std::make_unique<FontUniqueNameLookup>(temp_dir_.GetPath());
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
 };
@@ -311,6 +318,7 @@
         font_file_corruptor_.GetFontFilesList());
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir temp_dir_;
   FontFileCorruptor font_file_corruptor_;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
@@ -343,6 +351,7 @@
     font_unique_name_lookup_->SetAndroidBuildFingerprintForTesting("A");
   }
 
+  base::test::TaskEnvironment task_environment_;
   base::ScopedTempDir lookup_table_storage_dir;
   std::unique_ptr<FontUniqueNameLookup> font_unique_name_lookup_;
 };
@@ -364,4 +373,94 @@
   ASSERT_TRUE(matcher_initial.FontListIsDisjointFrom(matcher_second_half));
 }
 
+class TestFontUniqueNameLookup : public FontUniqueNameLookup {
+ public:
+  using FontUniqueNameLookup::FontUniqueNameLookup;
+  using FontUniqueNameLookup::ScheduleLoadOrUpdateTable;
+};
+
+class FontUniqueNameLookupCallbackTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
+    lookup_ = std::make_unique<TestFontUniqueNameLookup>(temp_dir_.GetPath());
+    std::vector<base::FilePath> font_files = AndroidFontFilesList();
+    ASSERT_FALSE(font_files.empty());
+    // Restrict to a single font to minimize test execution time and CPU load.
+    lookup_->SetFontFilePathsForTesting({font_files.front()});
+  }
+
+  base::test::TaskEnvironment task_environment_;
+  base::ScopedTempDir temp_dir_;
+  std::unique_ptr<TestFontUniqueNameLookup> lookup_;
+};
+
+TEST_F(FontUniqueNameLookupCallbackTest, ConcurrentCallbackQueuing) {
+  constexpr size_t kPreQueueCount = 10;
+  constexpr size_t kConcurrentThreads = 4;
+  constexpr size_t kPerThreadQueueCount = 5;
+  constexpr size_t kTotalCallbacks =
+      kPreQueueCount + (kConcurrentThreads * kPerThreadQueueCount);
+
+  base::RunLoop run_loop;
+  base::RepeatingClosure barrier =
+      base::BarrierClosure(kTotalCallbacks, run_loop.QuitClosure());
+
+  auto make_callback = [&]() {
+    return base::BindLambdaForTesting(
+        [&, barrier](base::ReadOnlySharedMemoryRegion region) {
+          EXPECT_TRUE(region.IsValid());
+          base::ReadOnlySharedMemoryMapping mapping = region.Map();
+          EXPECT_TRUE(mapping.IsValid());
+          blink::FontTableMatcher matcher(mapping);
+          EXPECT_GT(matcher.AvailableFonts(), 0u);
+          barrier.Run();
+        });
+  };
+
+  // Phase A: Pre-queue callbacks before table build starts.
+  // proto_storage_ready_ is guaranteed to be unsignaled, ensuring all callbacks
+  // are routed to callback_access_runner_ and enqueued in pending_callbacks_.
+  for (size_t i = 0; i < kPreQueueCount; ++i) {
+    scoped_refptr<base::SequencedTaskRunner> caller_runner =
+        base::ThreadPool::CreateSequencedTaskRunner({});
+    lookup_->QueueShareMemoryRegionWhenReady(caller_runner, make_callback());
+  }
+
+  // Start asynchronous table build on ThreadPool.
+  lookup_->ScheduleLoadOrUpdateTable();
+
+  // Phase B: Concurrently queue callbacks from multiple threads while table
+  // build and drain are in-flight.
+  for (size_t i = 0; i < kConcurrentThreads; ++i) {
+    base::ThreadPool::PostTask(
+        FROM_HERE, base::BindLambdaForTesting([&]() {
+          for (size_t j = 0; j < kPerThreadQueueCount; ++j) {
+            scoped_refptr<base::SequencedTaskRunner> caller_runner =
+                base::ThreadPool::CreateSequencedTaskRunner({});
+            lookup_->QueueShareMemoryRegionWhenReady(caller_runner,
+                                                     make_callback());
+          }
+        }));
+  }
+
+  // Wait for all pre-queued and concurrently queued callbacks to complete.
+  run_loop.Run();
+
+  // Phase C: Late / fast-path queueing verification.
+  // Callbacks queued after the table is built should be dispatched directly.
+  base::RunLoop late_run_loop;
+  lookup_->QueueShareMemoryRegionWhenReady(
+      base::SequencedTaskRunner::GetCurrentDefault(),
+      base::BindLambdaForTesting([&](base::ReadOnlySharedMemoryRegion region) {
+        EXPECT_TRUE(region.IsValid());
+        base::ReadOnlySharedMemoryMapping mapping = region.Map();
+        EXPECT_TRUE(mapping.IsValid());
+        blink::FontTableMatcher matcher(mapping);
+        EXPECT_GT(matcher.AvailableFonts(), 0u);
+        late_run_loop.Quit();
+      }));
+  late_run_loop.Run();
+}
+
 }  // namespace content