Sequentialise access to callbacks in DWriteFontLookupTableBuilder

Since there may be multiple instance of DWriteFontProxyImpl instantiated
for multiple RenderProcessHosts, and
DWriteFontProxyImpl::GetUniqueNameLookupTable may access
DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady from
separate threads, there may be race conditions around the
pending_callbacks_ member of DWriteFontLookupTableBuilder.

Sequentialise and guard access to pending_callbacks_ with a separate
sequenced task runner.

Fixed: 1047054
Change-Id: Ib7d7a385273bd82eb4d1acf720dac5d688a3435e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2030864
Commit-Queue: Dominik Röttsches <drott@chromium.org>
Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Auto-Submit: Dominik Röttsches <drott@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737252}

content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc

@@ -23,6 +23,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/task/post_task.h"
 #include "base/task/task_traits.h"
+#include "base/task/thread_pool.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/threading/thread_restrictions.h"
@@ -149,9 +150,6 @@ DWriteFontLookupTableBuilder::~DWriteFontLookupTableBuilder() = default;
 
 base::ReadOnlySharedMemoryRegion
 DWriteFontLookupTableBuilder::DuplicateMemoryRegion() {
-  DCHECK(!TableCacheFilePath().empty())
-      << "Ensure that a cache_directory_ is set (see "
-         "InitializeCacheDirectoryFromProfile())";
   DCHECK(FontUniqueNameTableReady());
   return font_table_memory_.region.Duplicate();
 }
@@ -250,7 +248,8 @@ base::TimeDelta DWriteFontLookupTableBuilder::IndexingTimeout() {
   return font_indexing_timeout_;
 }
 
-void DWriteFontLookupTableBuilder::PostCallbacks() {
+void DWriteFontLookupTableBuilder::PostCallbacksImpl() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(callbacks_access_sequence_checker_);
   for (auto& pending_callback : pending_callbacks_) {
     pending_callback.task_runner->PostTask(
         FROM_HERE, base::BindOnce(std::move(pending_callback.mojo_callback),
@@ -259,6 +258,13 @@ void DWriteFontLookupTableBuilder::PostCallbacks() {
   pending_callbacks_.clear();
 }
 
+void DWriteFontLookupTableBuilder::PostCallbacks() {
+  callbacks_access_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&DWriteFontLookupTableBuilder::PostCallbacksImpl,
+                     base::Unretained(this)));
+}
+
 base::FilePath DWriteFontLookupTableBuilder::TableCacheFilePath() {
   if (!EnsureCacheDirectory(cache_directory_))
     return base::FilePath();
@@ -300,18 +306,38 @@ DWriteFontLookupTableBuilder::CallbackOnTaskRunner::CallbackOnTaskRunner(
 DWriteFontLookupTableBuilder::CallbackOnTaskRunner::~CallbackOnTaskRunner() =
     default;
 
+void DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReadyImpl(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    blink::mojom::DWriteFontProxy::GetUniqueNameLookupTableCallback callback) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(callbacks_access_sequence_checker_);
+
+  // Don't queue but post response directly if the table is already ready for
+  // sharing with renderers to cover the condition in which the font table
+  // becomes ready briefly after a renderer asking for
+  // GetUniqueNameLookupTableIfAvailable(), receiving the information that it
+  // wasn't ready. (https://crbug.com/977283)
+  if (font_table_built_.IsSignaled()) {
+    task_runner->PostTask(FROM_HERE, base::BindOnce(std::move(callback),
+                                                    DuplicateMemoryRegion()));
+    return;
+  }
+
+  pending_callbacks_.push_back(
+      CallbackOnTaskRunner(std::move(task_runner), std::move(callback)));
+}
+
 void DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady(
     scoped_refptr<base::SequencedTaskRunner> task_runner,
     blink::mojom::DWriteFontProxy::GetUniqueNameLookupTableCallback callback) {
   TRACE_EVENT0("dwrite,fonts",
                "DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady");
   DCHECK(!HasDWriteUniqueFontLookups());
-  pending_callbacks_.emplace_back(std::move(task_runner), std::move(callback));
-  // Cover for the condition in which the font table becomes ready briefly after
-  // a renderer asking for GetUniqueNameLookupTableIfAvailable(), receiving the
-  // information that it wasn't ready.
-  if (font_table_built_.IsSignaled())
-    PostCallbacks();
+  CHECK(callbacks_access_task_runner_);
+  callbacks_access_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(
+          &DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReadyImpl,
+          base::Unretained(this), std::move(task_runner), std::move(callback)));
 }
 
 bool DWriteFontLookupTableBuilder::FontUniqueNameTableReady() {
@@ -347,6 +373,15 @@ void DWriteFontLookupTableBuilder::
   start_time_table_ready_ = base::TimeTicks::Now();
   scanning_error_reasons_.clear();
 
+  // Create callbacks_access_task_runner_ here instead of the constructor
+  // because between unit tests the DWriteFontTableBuilder instance persists,
+  // but the ThreadPool changes as the TaskEnvironment recreates this. Recreate
+  // the task runner here so it is associated with the new ThreadPool.
+  callbacks_access_task_runner_ = base::ThreadPool::CreateSequencedTaskRunner(
+      {base::TaskPriority::USER_VISIBLE,
+       base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN});
+  DETACH_FROM_SEQUENCE(callbacks_access_sequence_checker_);
+
   scoped_refptr<base::SequencedTaskRunner> results_collection_task_runner =
       base::CreateSequencedTaskRunner(
           {base::ThreadPool(), base::MayBlock(),

content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h

@@ -176,6 +176,16 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
   // constructed protobuf to disk.
   void FinalizeFontTable();
 
+  // Internal implementation of adding a callback request to the list in order
+  // to sequentialise access to pending_callbacks_.
+  void QueueShareMemoryRegionWhenReadyImpl(
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      blink::mojom::DWriteFontProxy::GetUniqueNameLookupTableCallback callback);
+
+  // Internal implementation of posting the callbacks, running on the sequence
+  // that sequentialises access to pending_callbacks_.
+  void PostCallbacksImpl();
+
   void OnTimeout();
 
   bool IsFontUniqueNameTableValid();
@@ -232,6 +242,8 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
 
   std::vector<CallbackOnTaskRunner> pending_callbacks_;
   std::map<HRESULT, unsigned> scanning_error_reasons_;
+  scoped_refptr<base::SequencedTaskRunner> callbacks_access_task_runner_;
+  SEQUENCE_CHECKER(callbacks_access_sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(DWriteFontLookupTableBuilder);
 };

content/browser/renderer_host/dwrite_font_lookup_table_builder_win_unittest.cc

@@ -109,6 +109,7 @@ TEST_P(DWriteFontLookupTableBuilderTimeoutTest, TestTimeout) {
   font_lookup_table_builder_->SetSlowDownIndexingForTestingWithTimeout(
       GetParam(), kTestingTimeout);
   font_lookup_table_builder_->SchedulePrepareFontUniqueNameTableIfNeeded();
+
   bool test_callback_executed = false;
   font_lookup_table_builder_->QueueShareMemoryRegionWhenReady(
       base::SequencedTaskRunnerHandle::Get(),