Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick d4564dcc2520 from chromium (#22979)
- Loading branch information
Showing
2 changed files
with
217 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
216 changes: 216 additions & 0 deletions
216
patches/chromium/reland_sequentialise_access_to_callbacks_in.patch
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,216 @@ | ||
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: =?UTF-8?q?Dominik=20R=C3=B6ttsches?= <drott@chromium.org> | ||
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 <mpdenton@chromium.org> | ||
Commit-Queue: Dominik Röttsches <drott@chromium.org> | ||
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); | ||
}; |