Browse Source

chore: cherry-pick d4564dcc2520 from chromium (#22979)

Jeremy Apthorp 5 years ago
parent
commit
3cb2484184

+ 1 - 0
patches/chromium/.patches

@@ -100,4 +100,5 @@ move_readablestream_requests_onto_the_stack_before_iteration.patch
 streams_convert_state_dchecks_to_checks.patch
 audiocontext_haspendingactivity_unless_it_s_closed.patch
 protect_automatic_pull_handlers_with_mutex.patch
+reland_sequentialise_access_to_callbacks_in.patch
 handle_err_cache_race_in_dodoneheadersaddtoentrycomplete.patch

+ 216 - 0
patches/chromium/reland_sequentialise_access_to_callbacks_in.patch

@@ -0,0 +1,216 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: =?UTF-8?q?Dominik=20R=C3=B6ttsches?= <[email protected]>
+Date: Fri, 31 Jan 2020 21:40:11 +0000
+Subject: Reland: Sequentialise access to callbacks in
+ DWriteFontLookupTableBuilder
+MIME-Version: 1.0
+Content-Type: text/plain; charset=UTF-8
+Content-Transfer-Encoding: 8bit
+
+This reland fixes the sequenced task runner initialisation. Set the
+task runner in the constructor and reset it for each unit test execution.
+
+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.
+
+Remove explicit configuration of the FontUniqueNameBrowserTest cache
+directory as [1] introduced a callback function in
+ShellContentBrowserClient which sets this correctly. This avoids
+instantiating DWriteFontLookupTableBuilder too early when the ThreadPool
+is not yet available in a BrowserTest.
+
+[1] https://chromium-review.googlesource.com/c/chromium/src/+/1776358/9/content/shell/browser/shell_content_browser_client.cc#422
+
+Fixed: 1047054
+Change-Id: I38cf8b84a48315980624b68bbf55d3727be457b8
+Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032119
+Reviewed-by: Matthew Denton <[email protected]>
+Commit-Queue: Dominik Röttsches <[email protected]>
+Cr-Commit-Position: refs/heads/master@{#737466}
+
+diff --git a/content/browser/font_unique_name_lookup/font_unique_name_browsertest.cc b/content/browser/font_unique_name_lookup/font_unique_name_browsertest.cc
+index 18e63dfaec08f37e107215876f4a0511542148d8..8266e6fc1628310dafd497debbe1c6ba43aa02ca 100644
+--- a/content/browser/font_unique_name_lookup/font_unique_name_browsertest.cc
++++ b/content/browser/font_unique_name_lookup/font_unique_name_browsertest.cc
+@@ -119,20 +119,10 @@ class FontUniqueNameBrowserTest : public DevToolsProtocolTest {
+   }
+ 
+ #if defined(OS_WIN)
+-  // The Windows service for font unique name lookup needs a cache directory to
+-  // persist the cached information. Configure a temporary one before running
+-  // this test.
+-  void SetUpInProcessBrowserTestFixture() override {
+-    DevToolsProtocolTest::SetUpInProcessBrowserTestFixture();
+-    DWriteFontLookupTableBuilder* table_builder =
+-        DWriteFontLookupTableBuilder::GetInstance();
+-    ASSERT_TRUE(cache_directory_.CreateUniqueTempDir());
+-    table_builder->SetCacheDirectoryForTesting(cache_directory_.GetPath());
+-  }
+-
+   void PreRunTestOnMainThread() override {
+     DWriteFontLookupTableBuilder* table_builder =
+         DWriteFontLookupTableBuilder::GetInstance();
++    table_builder->ResetStateForTesting();
+     table_builder->SchedulePrepareFontUniqueNameTableIfNeeded();
+     DevToolsProtocolTest::PreRunTestOnMainThread();
+   }
+diff --git a/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc b/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc
+index f8ba10769691864f59ea6f2ef55700d0cd9c4969..f151db4394e1b77f8ffbda6851af586943c8da3a 100644
+--- a/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc
++++ b/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.cc
+@@ -116,16 +116,30 @@ DWriteFontLookupTableBuilder::FontFileWithUniqueNames::FontFileWithUniqueNames(
+ 
+ DWriteFontLookupTableBuilder::DWriteFontLookupTableBuilder()
+     : font_indexing_timeout_(kFontIndexingTimeoutDefault) {
++  ResetCallbacksAccessTaskRunner();
+   InitializeCacheDirectoryFromProfile();
+ }
+ 
++void DWriteFontLookupTableBuilder::ResetCallbacksAccessTaskRunner() {
++  callbacks_access_task_runner_ = base::CreateSequencedTaskRunner({
++    base::ThreadPool(),
++    base::TaskPriority::USER_VISIBLE,
++#if DCHECK_IS_ON()
++        // Needed for DCHECK in DuplicateMemoryRegion() which performs file
++        // operations to detect cache directory.
++        base::MayBlock(),
++#endif
++        base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN
++  });
++  DETACH_FROM_SEQUENCE(callbacks_access_sequence_checker_);
++}
++
+ void DWriteFontLookupTableBuilder::InitializeCacheDirectoryFromProfile() {
+-  // In FontUniqueNameBrowserTest the DWriteFontLookupTableBuilder is
+-  // instantiated to configure the cache directory for testing explicitly before
+-  // GetContentClient() is available. Catch this case here. It is safe to not
+-  // set the cache directory here, as an invalid cache directory would be
+-  // detected by TableCacheFilePath and the LoadFromFile and PersistToFile
+-  // methods.
++  // Unit tests that do not launch a full browser environment usually don't need
++  // testing of src:local()-style font matching. Check that an environment is
++  // present here and configcure the cache directory based on that. If none is
++  // configured, catch this in DuplicateMemoryRegion(), i.e. when a client
++  // tries to use this API.
+   cache_directory_ =
+       GetContentClient() && GetContentClient()->browser()
+           ? GetContentClient()->browser()->GetFontLookupTableCacheDir()
+@@ -237,7 +251,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),
+@@ -246,6 +261,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();
+@@ -287,18 +309,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() {
+@@ -665,6 +707,7 @@ void DWriteFontLookupTableBuilder::ResetLookupTableForTesting() {
+   font_indexing_timeout_ = kFontIndexingTimeoutDefault;
+   font_table_memory_ = base::MappedReadOnlyRegion();
+   caching_enabled_ = true;
++  ResetCallbacksAccessTaskRunner();
+   font_table_built_.Reset();
+ }
+ 
+diff --git a/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h b/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h
+index 4d23f8ac3dada73b486b19bdfd033824140e10f6..3d19c8a8d36a9e263ff9373f489d38e7489368ff 100644
+--- a/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h
++++ b/content/browser/renderer_host/dwrite_font_lookup_table_builder_win.h
+@@ -168,6 +168,21 @@ 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();
++
++  // Resets the internal task runner guarding access to pending_callbacks_, used
++  // in unit tests, as the TaskEnvironment used in tests tears down and resets
++  // the ThreadPool between tests, and the TaskRunner depends on it.
++  void ResetCallbacksAccessTaskRunner();
++
+   void OnTimeout();
+ 
+   bool IsFontUniqueNameTableValid();
+@@ -221,6 +236,8 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
+   };
+ 
+   std::vector<CallbackOnTaskRunner> pending_callbacks_;
++  scoped_refptr<base::SequencedTaskRunner> callbacks_access_task_runner_;
++  SEQUENCE_CHECKER(callbacks_access_sequence_checker_);
+ 
+   DISALLOW_COPY_AND_ASSIGN(DWriteFontLookupTableBuilder);
+ };