Commit ddced547 authored by John Delaney's avatar John Delaney Committed by Commit Bot

[conversions] Add unified storage task runner

What:
Use a single LazyThreadPoolSequencedTaskRunner for all conversion
storage operations.

Why:
Conversion Storage doesn't have strong ordering guarantees regarding
databases being torn down and recreated for the same profile.

E.g.

Profile closed -> delete posted to task runner A
Profile opened -> create posted to task runner B

We assume that task runner A will finish executing the delete,
freeing the database prior to task runner B trying to open it. However,
if task runner A has a long running operation already sequenced, this
isn't necessarily true.

Using a unified task runner removes this edge case.

This also removes the need to do any dependency injection which is
discouraged for task runners.

Bug: 1114783
Change-Id: If09d0a9a68ec0d461321744f2734c28b0ccbe068
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2490256Reviewed-by: default avatarCharlie Harrison <csharrison@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Commit-Queue: John Delaney <johnidel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#821411}
parent 218049b0
...@@ -10,7 +10,6 @@ ...@@ -10,7 +10,6 @@
#include "base/bind.h" #include "base/bind.h"
#include "base/command_line.h" #include "base/command_line.h"
#include "base/memory/ptr_util.h" #include "base/memory/ptr_util.h"
#include "base/task_runner_util.h"
#include "base/time/default_clock.h" #include "base/time/default_clock.h"
#include "content/browser/conversions/conversion_reporter_impl.h" #include "content/browser/conversions/conversion_reporter_impl.h"
#include "content/browser/conversions/conversion_storage_delegate_impl.h" #include "content/browser/conversions/conversion_storage_delegate_impl.h"
...@@ -44,17 +43,14 @@ std::unique_ptr<ConversionManagerImpl> ConversionManagerImpl::CreateForTesting( ...@@ -44,17 +43,14 @@ std::unique_ptr<ConversionManagerImpl> ConversionManagerImpl::CreateForTesting(
std::unique_ptr<ConversionReporter> reporter, std::unique_ptr<ConversionReporter> reporter,
std::unique_ptr<ConversionPolicy> policy, std::unique_ptr<ConversionPolicy> policy,
const base::Clock* clock, const base::Clock* clock,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory) {
scoped_refptr<base::SequencedTaskRunner> storage_task_runner) {
return base::WrapUnique<ConversionManagerImpl>(new ConversionManagerImpl( return base::WrapUnique<ConversionManagerImpl>(new ConversionManagerImpl(
std::move(reporter), std::move(policy), clock, user_data_directory, std::move(reporter), std::move(policy), clock, user_data_directory));
std::move(storage_task_runner)));
} }
ConversionManagerImpl::ConversionManagerImpl( ConversionManagerImpl::ConversionManagerImpl(
StoragePartition* storage_partition, StoragePartition* storage_partition,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory)
scoped_refptr<base::SequencedTaskRunner> task_runner)
: ConversionManagerImpl( : ConversionManagerImpl(
std::make_unique<ConversionReporterImpl>( std::make_unique<ConversionReporterImpl>(
storage_partition, storage_partition,
...@@ -63,22 +59,19 @@ ConversionManagerImpl::ConversionManagerImpl( ...@@ -63,22 +59,19 @@ ConversionManagerImpl::ConversionManagerImpl(
base::CommandLine::ForCurrentProcess()->HasSwitch( base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kConversionsDebugMode)), switches::kConversionsDebugMode)),
base::DefaultClock::GetInstance(), base::DefaultClock::GetInstance(),
user_data_directory, user_data_directory) {}
std::move(task_runner)) {}
ConversionManagerImpl::ConversionManagerImpl( ConversionManagerImpl::ConversionManagerImpl(
std::unique_ptr<ConversionReporter> reporter, std::unique_ptr<ConversionReporter> reporter,
std::unique_ptr<ConversionPolicy> policy, std::unique_ptr<ConversionPolicy> policy,
const base::Clock* clock, const base::Clock* clock,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory)
scoped_refptr<base::SequencedTaskRunner> storage_task_runner)
: debug_mode_(base::CommandLine::ForCurrentProcess()->HasSwitch( : debug_mode_(base::CommandLine::ForCurrentProcess()->HasSwitch(
switches::kConversionsDebugMode)), switches::kConversionsDebugMode)),
clock_(clock), clock_(clock),
reporter_(std::move(reporter)), reporter_(std::move(reporter)),
conversion_storage_context_( conversion_storage_context_(
base::MakeRefCounted<ConversionStorageContext>( base::MakeRefCounted<ConversionStorageContext>(
std::move(storage_task_runner),
user_data_directory, user_data_directory,
std::make_unique<ConversionStorageDelegateImpl>(debug_mode_), std::make_unique<ConversionStorageDelegateImpl>(debug_mode_),
clock_)), clock_)),
...@@ -122,8 +115,6 @@ void ConversionManagerImpl::HandleConversion( ...@@ -122,8 +115,6 @@ void ConversionManagerImpl::HandleConversion(
void ConversionManagerImpl::GetActiveImpressionsForWebUI( void ConversionManagerImpl::GetActiveImpressionsForWebUI(
base::OnceCallback<void(std::vector<StorableImpression>)> callback) { base::OnceCallback<void(std::vector<StorableImpression>)> callback) {
// Unretained is safe because any task to delete |storage_| will be posted
// after this one because |storage_| uses base::OnTaskRunnerDeleter.
conversion_storage_context_->GetActiveImpressions(std::move(callback)); conversion_storage_context_->GetActiveImpressions(std::move(callback));
} }
......
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "base/macros.h" #include "base/macros.h"
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "base/memory/weak_ptr.h" #include "base/memory/weak_ptr.h"
#include "base/sequenced_task_runner.h"
#include "base/time/time.h" #include "base/time/time.h"
#include "base/timer/timer.h" #include "base/timer/timer.h"
#include "content/browser/conversions/conversion_manager.h" #include "content/browser/conversions/conversion_manager.h"
...@@ -34,7 +33,6 @@ namespace content { ...@@ -34,7 +33,6 @@ namespace content {
extern CONTENT_EXPORT const base::TimeDelta extern CONTENT_EXPORT const base::TimeDelta
kConversionManagerQueueReportsInterval; kConversionManagerQueueReportsInterval;
class ConversionStorage;
class StoragePartition; class StoragePartition;
// Provides access to the manager owned by the default StoragePartition. // Provides access to the manager owned by the default StoragePartition.
...@@ -77,14 +75,10 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager { ...@@ -77,14 +75,10 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
std::unique_ptr<ConversionReporter> reporter, std::unique_ptr<ConversionReporter> reporter,
std::unique_ptr<ConversionPolicy> policy, std::unique_ptr<ConversionPolicy> policy,
const base::Clock* clock, const base::Clock* clock,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory);
scoped_refptr<base::SequencedTaskRunner> storage_task_runner);
ConversionManagerImpl(StoragePartition* storage_partition,
// |storage_task_runner| should run with base::TaskPriority::BEST_EFFORT. const base::FilePath& user_data_directory);
ConversionManagerImpl(
StoragePartition* storage_partition,
const base::FilePath& user_data_directory,
scoped_refptr<base::SequencedTaskRunner> storage_task_runner);
ConversionManagerImpl(const ConversionManagerImpl& other) = delete; ConversionManagerImpl(const ConversionManagerImpl& other) = delete;
ConversionManagerImpl& operator=(const ConversionManagerImpl& other) = delete; ConversionManagerImpl& operator=(const ConversionManagerImpl& other) = delete;
~ConversionManagerImpl() override; ~ConversionManagerImpl() override;
...@@ -106,12 +100,10 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager { ...@@ -106,12 +100,10 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
base::OnceClosure done) override; base::OnceClosure done) override;
private: private:
ConversionManagerImpl( ConversionManagerImpl(std::unique_ptr<ConversionReporter> reporter,
std::unique_ptr<ConversionReporter> reporter, std::unique_ptr<ConversionPolicy> policy,
std::unique_ptr<ConversionPolicy> policy, const base::Clock* clock,
const base::Clock* clock, const base::FilePath& user_data_directory);
const base::FilePath& user_data_directory,
scoped_refptr<base::SequencedTaskRunner> storage_task_runner);
// Retrieves reports from storage whose |report_time| <= |max_report_time|, // Retrieves reports from storage whose |report_time| <= |max_report_time|,
// and calls |handler_function| on them. // and calls |handler_function| on them.
...@@ -142,8 +134,7 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager { ...@@ -142,8 +134,7 @@ class CONTENT_EXPORT ConversionManagerImpl : public ConversionManager {
void OnReportSentFromWebUI(base::OnceClosure reports_sent_barrier, void OnReportSentFromWebUI(base::OnceClosure reports_sent_barrier,
int64_t conversion_id); int64_t conversion_id);
// Friend to expose the ConversionStorage and task runner, consider changing // Friend to expose the ConversionStorageContext for certain tests.
// to just expose the storage if it moves to SequenceBound.
friend std::vector<ConversionReport> GetConversionsToReportForTesting( friend std::vector<ConversionReport> GetConversionsToReportForTesting(
ConversionManagerImpl* manager, ConversionManagerImpl* manager,
base::Time max_report_time); base::Time max_report_time);
......
...@@ -120,8 +120,7 @@ class ConversionManagerImplTest : public testing::Test { ...@@ -120,8 +120,7 @@ class ConversionManagerImplTest : public testing::Test {
test_reporter_ = reporter.get(); test_reporter_ = reporter.get();
conversion_manager_ = ConversionManagerImpl::CreateForTesting( conversion_manager_ = ConversionManagerImpl::CreateForTesting(
std::move(reporter), std::make_unique<ConstantStartupDelayPolicy>(), std::move(reporter), std::make_unique<ConstantStartupDelayPolicy>(),
task_environment_.GetMockClock(), dir_.GetPath(), task_environment_.GetMockClock(), dir_.GetPath());
base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()}));
} }
const base::Clock& clock() { return *task_environment_.GetMockClock(); } const base::Clock& clock() { return *task_environment_.GetMockClock(); }
......
...@@ -5,16 +5,28 @@ ...@@ -5,16 +5,28 @@
#include "content/browser/conversions/conversion_storage_context.h" #include "content/browser/conversions/conversion_storage_context.h"
#include "base/bind_helpers.h" #include "base/bind_helpers.h"
#include "base/task/lazy_thread_pool_task_runner.h"
#include "content/browser/conversions/conversion_storage_sql.h" #include "content/browser/conversions/conversion_storage_sql.h"
namespace content { namespace content {
namespace {
// The shared-task runner for all conversion storage operations. Note that
// different ConversionStorageContext perform operations on the same task
// runner. This prevents any potential races when a given context is destroyed
// and recreated for the same backing storage.
base::LazyThreadPoolSequencedTaskRunner g_storage_task_runner =
LAZY_THREAD_POOL_SEQUENCED_TASK_RUNNER_INITIALIZER(
base::TaskTraits(base::TaskPriority::BEST_EFFORT, base::MayBlock()));
} // namespace
ConversionStorageContext::ConversionStorageContext( ConversionStorageContext::ConversionStorageContext(
scoped_refptr<base::SequencedTaskRunner> storage_task_runner,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory,
std::unique_ptr<ConversionStorageDelegateImpl> delegate, std::unique_ptr<ConversionStorageDelegateImpl> delegate,
const base::Clock* clock) const base::Clock* clock)
: storage_task_runner_(std::move(storage_task_runner)), : storage_task_runner_(g_storage_task_runner.Get()),
storage_(new ConversionStorageSql(user_data_directory, storage_(new ConversionStorageSql(user_data_directory,
std::move(delegate), std::move(delegate),
clock), clock),
......
...@@ -37,7 +37,6 @@ class CONTENT_EXPORT ConversionStorageContext ...@@ -37,7 +37,6 @@ class CONTENT_EXPORT ConversionStorageContext
: public base::RefCountedThreadSafe<ConversionStorageContext> { : public base::RefCountedThreadSafe<ConversionStorageContext> {
public: public:
ConversionStorageContext( ConversionStorageContext(
scoped_refptr<base::SequencedTaskRunner> storage_task_runner,
const base::FilePath& user_data_directory, const base::FilePath& user_data_directory,
std::unique_ptr<ConversionStorageDelegateImpl> delegate, std::unique_ptr<ConversionStorageDelegateImpl> delegate,
const base::Clock* clock); const base::Clock* clock);
......
...@@ -1354,10 +1354,7 @@ void StoragePartitionImpl::Initialize( ...@@ -1354,10 +1354,7 @@ void StoragePartitionImpl::Initialize(
// The Conversion Measurement API is not available in Incognito mode. // The Conversion Measurement API is not available in Incognito mode.
if (!is_in_memory_ && if (!is_in_memory_ &&
base::FeatureList::IsEnabled(features::kConversionMeasurement)) { base::FeatureList::IsEnabled(features::kConversionMeasurement)) {
conversion_manager_ = std::make_unique<ConversionManagerImpl>( conversion_manager_ = std::make_unique<ConversionManagerImpl>(this, path);
this, path,
base::ThreadPool::CreateSequencedTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT}));
} }
GeneratedCodeCacheSettings settings = GeneratedCodeCacheSettings settings =
......
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