Commit 079e294e authored by Ioana Pandele's avatar Ioana Pandele Committed by Commit Bot

Revert "Sequentialise access to callbacks in DWriteFontLookupTableBuilder"

This reverts commit 36911404.

Reason for revert: Multiple test targets are failing on Win7 Tests with: 3724:4788:0131/021830.830:FATAL:dwrite_font_lookup_table_builder_win.cc(335)] Check failed: callbacks_access_task_runner_.

Bug: 1047647
Original change's description:
> 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}

TBR=drott@chromium.org,mpdenton@chromium.org

Change-Id: I23ae8f14e6f82171d6b6950a937b51faa00b3a63
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2031043Reviewed-by: default avatarIoana Pandele <ioanap@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737285}
parent 052630eb
...@@ -23,7 +23,6 @@ ...@@ -23,7 +23,6 @@
#include "base/strings/utf_string_conversions.h" #include "base/strings/utf_string_conversions.h"
#include "base/task/post_task.h" #include "base/task/post_task.h"
#include "base/task/task_traits.h" #include "base/task/task_traits.h"
#include "base/task/thread_pool.h"
#include "base/threading/scoped_blocking_call.h" #include "base/threading/scoped_blocking_call.h"
#include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/sequenced_task_runner_handle.h"
#include "base/threading/thread_restrictions.h" #include "base/threading/thread_restrictions.h"
...@@ -150,6 +149,9 @@ DWriteFontLookupTableBuilder::~DWriteFontLookupTableBuilder() = default; ...@@ -150,6 +149,9 @@ DWriteFontLookupTableBuilder::~DWriteFontLookupTableBuilder() = default;
base::ReadOnlySharedMemoryRegion base::ReadOnlySharedMemoryRegion
DWriteFontLookupTableBuilder::DuplicateMemoryRegion() { DWriteFontLookupTableBuilder::DuplicateMemoryRegion() {
DCHECK(!TableCacheFilePath().empty())
<< "Ensure that a cache_directory_ is set (see "
"InitializeCacheDirectoryFromProfile())";
DCHECK(FontUniqueNameTableReady()); DCHECK(FontUniqueNameTableReady());
return font_table_memory_.region.Duplicate(); return font_table_memory_.region.Duplicate();
} }
...@@ -248,8 +250,7 @@ base::TimeDelta DWriteFontLookupTableBuilder::IndexingTimeout() { ...@@ -248,8 +250,7 @@ base::TimeDelta DWriteFontLookupTableBuilder::IndexingTimeout() {
return font_indexing_timeout_; return font_indexing_timeout_;
} }
void DWriteFontLookupTableBuilder::PostCallbacksImpl() { void DWriteFontLookupTableBuilder::PostCallbacks() {
DCHECK_CALLED_ON_VALID_SEQUENCE(callbacks_access_sequence_checker_);
for (auto& pending_callback : pending_callbacks_) { for (auto& pending_callback : pending_callbacks_) {
pending_callback.task_runner->PostTask( pending_callback.task_runner->PostTask(
FROM_HERE, base::BindOnce(std::move(pending_callback.mojo_callback), FROM_HERE, base::BindOnce(std::move(pending_callback.mojo_callback),
...@@ -258,13 +259,6 @@ void DWriteFontLookupTableBuilder::PostCallbacksImpl() { ...@@ -258,13 +259,6 @@ void DWriteFontLookupTableBuilder::PostCallbacksImpl() {
pending_callbacks_.clear(); pending_callbacks_.clear();
} }
void DWriteFontLookupTableBuilder::PostCallbacks() {
callbacks_access_task_runner_->PostTask(
FROM_HERE,
base::BindOnce(&DWriteFontLookupTableBuilder::PostCallbacksImpl,
base::Unretained(this)));
}
base::FilePath DWriteFontLookupTableBuilder::TableCacheFilePath() { base::FilePath DWriteFontLookupTableBuilder::TableCacheFilePath() {
if (!EnsureCacheDirectory(cache_directory_)) if (!EnsureCacheDirectory(cache_directory_))
return base::FilePath(); return base::FilePath();
...@@ -306,38 +300,18 @@ DWriteFontLookupTableBuilder::CallbackOnTaskRunner::CallbackOnTaskRunner( ...@@ -306,38 +300,18 @@ DWriteFontLookupTableBuilder::CallbackOnTaskRunner::CallbackOnTaskRunner(
DWriteFontLookupTableBuilder::CallbackOnTaskRunner::~CallbackOnTaskRunner() = DWriteFontLookupTableBuilder::CallbackOnTaskRunner::~CallbackOnTaskRunner() =
default; 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( void DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady(
scoped_refptr<base::SequencedTaskRunner> task_runner, scoped_refptr<base::SequencedTaskRunner> task_runner,
blink::mojom::DWriteFontProxy::GetUniqueNameLookupTableCallback callback) { blink::mojom::DWriteFontProxy::GetUniqueNameLookupTableCallback callback) {
TRACE_EVENT0("dwrite,fonts", TRACE_EVENT0("dwrite,fonts",
"DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady"); "DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReady");
DCHECK(!HasDWriteUniqueFontLookups()); DCHECK(!HasDWriteUniqueFontLookups());
CHECK(callbacks_access_task_runner_); pending_callbacks_.emplace_back(std::move(task_runner), std::move(callback));
callbacks_access_task_runner_->PostTask( // Cover for the condition in which the font table becomes ready briefly after
FROM_HERE, // a renderer asking for GetUniqueNameLookupTableIfAvailable(), receiving the
base::BindOnce( // information that it wasn't ready.
&DWriteFontLookupTableBuilder::QueueShareMemoryRegionWhenReadyImpl, if (font_table_built_.IsSignaled())
base::Unretained(this), std::move(task_runner), std::move(callback))); PostCallbacks();
} }
bool DWriteFontLookupTableBuilder::FontUniqueNameTableReady() { bool DWriteFontLookupTableBuilder::FontUniqueNameTableReady() {
...@@ -373,15 +347,6 @@ void DWriteFontLookupTableBuilder:: ...@@ -373,15 +347,6 @@ void DWriteFontLookupTableBuilder::
start_time_table_ready_ = base::TimeTicks::Now(); start_time_table_ready_ = base::TimeTicks::Now();
scanning_error_reasons_.clear(); 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 = scoped_refptr<base::SequencedTaskRunner> results_collection_task_runner =
base::CreateSequencedTaskRunner( base::CreateSequencedTaskRunner(
{base::ThreadPool(), base::MayBlock(), {base::ThreadPool(), base::MayBlock(),
......
...@@ -176,16 +176,6 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder { ...@@ -176,16 +176,6 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
// constructed protobuf to disk. // constructed protobuf to disk.
void FinalizeFontTable(); 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(); void OnTimeout();
bool IsFontUniqueNameTableValid(); bool IsFontUniqueNameTableValid();
...@@ -242,8 +232,6 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder { ...@@ -242,8 +232,6 @@ class CONTENT_EXPORT DWriteFontLookupTableBuilder {
std::vector<CallbackOnTaskRunner> pending_callbacks_; std::vector<CallbackOnTaskRunner> pending_callbacks_;
std::map<HRESULT, unsigned> scanning_error_reasons_; 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); DISALLOW_COPY_AND_ASSIGN(DWriteFontLookupTableBuilder);
}; };
......
...@@ -109,7 +109,6 @@ TEST_P(DWriteFontLookupTableBuilderTimeoutTest, TestTimeout) { ...@@ -109,7 +109,6 @@ TEST_P(DWriteFontLookupTableBuilderTimeoutTest, TestTimeout) {
font_lookup_table_builder_->SetSlowDownIndexingForTestingWithTimeout( font_lookup_table_builder_->SetSlowDownIndexingForTestingWithTimeout(
GetParam(), kTestingTimeout); GetParam(), kTestingTimeout);
font_lookup_table_builder_->SchedulePrepareFontUniqueNameTableIfNeeded(); font_lookup_table_builder_->SchedulePrepareFontUniqueNameTableIfNeeded();
bool test_callback_executed = false; bool test_callback_executed = false;
font_lookup_table_builder_->QueueShareMemoryRegionWhenReady( font_lookup_table_builder_->QueueShareMemoryRegionWhenReady(
base::SequencedTaskRunnerHandle::Get(), base::SequencedTaskRunnerHandle::Get(),
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment