Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick d4564dcc2520 from chromium #22979

Merged
merged 7 commits into from Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -99,3 +99,4 @@ allow_restricted_clock_nanosleep_in_linux_sandbox.patch
move_readablestream_requests_onto_the_stack_before_iteration.patch
streams_convert_state_dchecks_to_checks.patch
cherry-pick-fd211b44535c.patch
cherry-pick-d4564dcc2520.patch
228 changes: 228 additions & 0 deletions patches/chromium/cherry-pick-d4564dcc2520.patch
@@ -0,0 +1,228 @@
From d4564dcc2520b00c23c287461f8fa0489e6d1d32 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: [PATCH] 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}
---
.../font_unique_name_browsertest.cc | 12 +---
.../dwrite_font_lookup_table_builder_win.cc | 69 +++++++++++++++----
.../dwrite_font_lookup_table_builder_win.h | 17 +++++
3 files changed, 74 insertions(+), 24 deletions(-)

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 6b9ee1c98bd01..f9967d2502b41 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
@@ -117,20 +117,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 55bacfb313223..de346ae1cae8e 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
@@ -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"
@@ -129,16 +130,29 @@ DWriteFontLookupTableBuilder::FamilyResult::~FamilyResult() = default;

DWriteFontLookupTableBuilder::DWriteFontLookupTableBuilder()
: font_indexing_timeout_(kFontIndexingTimeoutDefault) {
+ ResetCallbacksAccessTaskRunner();
InitializeCacheDirectoryFromProfile();
}

+void DWriteFontLookupTableBuilder::ResetCallbacksAccessTaskRunner() {
+ callbacks_access_task_runner_ = base::ThreadPool::CreateSequencedTaskRunner({
+ 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()
@@ -250,7 +264,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 +274,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 +322,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() {
@@ -730,6 +772,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 80c75e0633f3e..2d743753a07c9 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
@@ -176,6 +176,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();
@@ -232,6 +247,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);
};