diff options
33 files changed, 330 insertions, 188 deletions
diff --git a/base/files/important_file_writer.cc b/base/files/important_file_writer.cc index 5663bdd..536b0fb 100644 --- a/base/files/important_file_writer.cc +++ b/base/files/important_file_writer.cc @@ -13,7 +13,7 @@ #include "base/file_path.h" #include "base/file_util.h" #include "base/logging.h" -#include "base/message_loop_proxy.h" +#include "base/task_runner.h" #include "base/metrics/histogram.h" #include "base/string_number_conversions.h" #include "base/threading/thread.h" @@ -90,14 +90,14 @@ void WriteToDiskTask(const FilePath& path, const std::string& data) { } // namespace ImportantFileWriter::ImportantFileWriter( - const FilePath& path, MessageLoopProxy* file_message_loop_proxy) + const FilePath& path, base::SequencedTaskRunner* task_runner) : path_(path), - file_message_loop_proxy_(file_message_loop_proxy), + task_runner_(task_runner), serializer_(NULL), commit_interval_(TimeDelta::FromMilliseconds( kDefaultCommitIntervalMs)) { DCHECK(CalledOnValidThread()); - DCHECK(file_message_loop_proxy_.get()); + DCHECK(task_runner_.get()); } ImportantFileWriter::~ImportantFileWriter() { @@ -122,8 +122,8 @@ void ImportantFileWriter::WriteNow(const std::string& data) { if (HasPendingWrite()) timer_.Stop(); - if (!file_message_loop_proxy_->PostTask( - FROM_HERE, MakeCriticalClosure(Bind(&WriteToDiskTask, path_, data)))) { + if (!task_runner_->PostTask(FROM_HERE, + MakeCriticalClosure(Bind(&WriteToDiskTask, path_, data)))) { // Posting the task to background message loop is not expected // to fail, but if it does, avoid losing data and just hit the disk // on the current thread. diff --git a/base/files/important_file_writer.h b/base/files/important_file_writer.h index 4a6794c..f0981aa 100644 --- a/base/files/important_file_writer.h +++ b/base/files/important_file_writer.h @@ -17,7 +17,7 @@ namespace base { -class MessageLoopProxy; +class SequencedTaskRunner; class Thread; // Helper to ensure that a file won't be corrupted by the write (for example on @@ -53,11 +53,11 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { // Initialize the writer. // |path| is the name of file to write. - // |file_message_loop_proxy| is the MessageLoopProxy for a thread on which - // file I/O can be done. + // |task_runner| is the SequencedTaskRunner instance where on which we will + // execute file I/O operations. // All non-const methods, ctor and dtor must be called on the same thread. ImportantFileWriter(const FilePath& path, - MessageLoopProxy* file_message_loop_proxy); + base::SequencedTaskRunner* task_runner); // You have to ensure that there are no pending writes at the moment // of destruction. @@ -96,8 +96,8 @@ class BASE_EXPORT ImportantFileWriter : public NonThreadSafe { // Path being written to. const FilePath path_; - // MessageLoopProxy for the thread on which file I/O can be done. - scoped_refptr<MessageLoopProxy> file_message_loop_proxy_; + // TaskRunner for the thread on which file I/O can be done. + const scoped_refptr<base::SequencedTaskRunner> task_runner_; // Timer used to schedule commit after ScheduleWrite. OneShotTimer<ImportantFileWriter> timer_; diff --git a/base/prefs/json_pref_store.cc b/base/prefs/json_pref_store.cc index 18dc0b7..98810d5 100644 --- a/base/prefs/json_pref_store.cc +++ b/base/prefs/json_pref_store.cc @@ -13,6 +13,8 @@ #include "base/json/json_string_value_serializer.h" #include "base/memory/ref_counted.h" #include "base/message_loop_proxy.h" +#include "base/sequenced_task_runner.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/values.h" namespace { @@ -26,25 +28,25 @@ class FileThreadDeserializer : public base::RefCountedThreadSafe<FileThreadDeserializer> { public: FileThreadDeserializer(JsonPrefStore* delegate, - base::MessageLoopProxy* file_loop_proxy) + base::SequencedTaskRunner* sequenced_task_runner) : no_dir_(false), error_(PersistentPrefStore::PREF_READ_ERROR_NONE), delegate_(delegate), - file_loop_proxy_(file_loop_proxy), + sequenced_task_runner_(sequenced_task_runner), origin_loop_proxy_(base::MessageLoopProxy::current()) { } void Start(const FilePath& path) { DCHECK(origin_loop_proxy_->BelongsToCurrentThread()); - file_loop_proxy_->PostTask( + sequenced_task_runner_->PostTask( FROM_HERE, base::Bind(&FileThreadDeserializer::ReadFileAndReport, this, path)); } - // Deserializes JSON on the file thread. + // Deserializes JSON on the sequenced task runner. void ReadFileAndReport(const FilePath& path) { - DCHECK(file_loop_proxy_->BelongsToCurrentThread()); + DCHECK(sequenced_task_runner_->RunsTasksOnCurrentThread()); value_.reset(DoReading(path, &error_, &no_dir_)); @@ -84,9 +86,9 @@ class FileThreadDeserializer bool no_dir_; PersistentPrefStore::PrefReadError error_; scoped_ptr<Value> value_; - scoped_refptr<JsonPrefStore> delegate_; - scoped_refptr<base::MessageLoopProxy> file_loop_proxy_; - scoped_refptr<base::MessageLoopProxy> origin_loop_proxy_; + const scoped_refptr<JsonPrefStore> delegate_; + const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_; + const scoped_refptr<base::MessageLoopProxy> origin_loop_proxy_; }; // static @@ -137,13 +139,23 @@ void FileThreadDeserializer::HandleErrors( } // namespace +scoped_refptr<base::SequencedTaskRunner> JsonPrefStore::GetTaskRunnerForFile( + const FilePath& filename, + base::SequencedWorkerPool* worker_pool) { + std::string token("json_pref_store-"); + token.append(filename.AsUTF8Unsafe()); + return worker_pool->GetSequencedTaskRunnerWithShutdownBehavior( + worker_pool->GetNamedSequenceToken(token), + base::SequencedWorkerPool::BLOCK_SHUTDOWN); +} + JsonPrefStore::JsonPrefStore(const FilePath& filename, - base::MessageLoopProxy* file_message_loop_proxy) + base::SequencedTaskRunner* sequenced_task_runner) : path_(filename), - file_message_loop_proxy_(file_message_loop_proxy), + sequenced_task_runner_(sequenced_task_runner), prefs_(new DictionaryValue()), read_only_(false), - writer_(filename, file_message_loop_proxy), + writer_(filename, sequenced_task_runner), error_delegate_(NULL), initialized_(false), read_error_(PREF_READ_ERROR_OTHER) { @@ -245,7 +257,7 @@ void JsonPrefStore::ReadPrefsAsync(ReadErrorDelegate *error_delegate) { // Start async reading of the preferences file. It will delete itself // in the end. scoped_refptr<FileThreadDeserializer> deserializer( - new FileThreadDeserializer(this, file_message_loop_proxy_.get())); + new FileThreadDeserializer(this, sequenced_task_runner_.get())); deserializer->Start(path_); } diff --git a/base/prefs/json_pref_store.h b/base/prefs/json_pref_store.h index f5f083a..f5e08d5 100644 --- a/base/prefs/json_pref_store.h +++ b/base/prefs/json_pref_store.h @@ -19,7 +19,8 @@ namespace base { class DictionaryValue; -class MessageLoopProxy; +class SequencedWorkerPool; +class SequencedTaskRunner; class Value; } @@ -29,10 +30,16 @@ class FilePath; class JsonPrefStore : public PersistentPrefStore, public base::ImportantFileWriter::DataSerializer { public: - // |file_message_loop_proxy| is the MessageLoopProxy for a thread on which - // file I/O can be done. + // Returns instance of SequencedTaskRunner which guarantees that file + // operations on the same file will be executed in sequenced order. + static scoped_refptr<base::SequencedTaskRunner> GetTaskRunnerForFile( + const FilePath& pref_filename, + base::SequencedWorkerPool* worker_pool); + + // |sequenced_task_runner| is must be a shutdown-blocking task runner, ideally + // created by GetTaskRunnerForFile() method above. JsonPrefStore(const FilePath& pref_filename, - base::MessageLoopProxy* file_message_loop_proxy); + base::SequencedTaskRunner* sequenced_task_runner); // PrefStore overrides: virtual ReadResult GetValue(const std::string& key, @@ -70,7 +77,7 @@ class JsonPrefStore : public PersistentPrefStore, virtual bool SerializeData(std::string* output) OVERRIDE; FilePath path_; - scoped_refptr<base::MessageLoopProxy> file_message_loop_proxy_; + const scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_; scoped_ptr<base::DictionaryValue> prefs_; diff --git a/base/prefs/json_pref_store_unittest.cc b/base/prefs/json_pref_store_unittest.cc index 7661acd..e7239bc 100644 --- a/base/prefs/json_pref_store_unittest.cc +++ b/base/prefs/json_pref_store_unittest.cc @@ -5,13 +5,12 @@ #include "base/file_util.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" -#include "base/message_loop.h" -#include "base/message_loop_proxy.h" #include "base/path_service.h" #include "base/prefs/json_pref_store.h" #include "base/scoped_temp_dir.h" #include "base/string_number_conversions.h" #include "base/string_util.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/threading/thread.h" #include "base/utf_string_conversions.h" #include "base/values.h" @@ -37,9 +36,7 @@ class MockReadErrorDelegate : public PersistentPrefStore::ReadErrorDelegate { class JsonPrefStoreTest : public testing::Test { protected: - virtual void SetUp() { - message_loop_proxy_ = base::MessageLoopProxy::current(); - + virtual void SetUp() OVERRIDE { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); ASSERT_TRUE(PathService::Get(chrome::DIR_TEST_DATA, &data_dir_)); @@ -53,7 +50,6 @@ class JsonPrefStoreTest : public testing::Test { FilePath data_dir_; // A message loop that we can use as the file thread message loop. MessageLoop message_loop_; - scoped_refptr<base::MessageLoopProxy> message_loop_proxy_; }; // Test fallback behavior for a nonexistent file. @@ -61,7 +57,8 @@ TEST_F(JsonPrefStoreTest, NonExistentFile) { FilePath bogus_input_file = data_dir_.AppendASCII("read.txt"); ASSERT_FALSE(file_util::PathExists(bogus_input_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(bogus_input_file, message_loop_proxy_.get()); + new JsonPrefStore( + bogus_input_file, message_loop_.message_loop_proxy()); EXPECT_EQ(PersistentPrefStore::PREF_READ_ERROR_NO_FILE, pref_store->ReadPrefs()); EXPECT_FALSE(pref_store->ReadOnly()); @@ -73,7 +70,8 @@ TEST_F(JsonPrefStoreTest, InvalidFile) { FilePath invalid_file = temp_dir_.path().AppendASCII("invalid.json"); ASSERT_TRUE(file_util::CopyFile(invalid_file_original, invalid_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(invalid_file, message_loop_proxy_.get()); + new JsonPrefStore( + invalid_file, message_loop_.message_loop_proxy()); EXPECT_EQ(PersistentPrefStore::PREF_READ_ERROR_JSON_PARSE, pref_store->ReadPrefs()); EXPECT_FALSE(pref_store->ReadOnly()); @@ -88,7 +86,7 @@ TEST_F(JsonPrefStoreTest, InvalidFile) { // This function is used to avoid code duplication while testing synchronous and // asynchronous version of the JsonPrefStore loading. -void RunBasicJsonPrefStoreTest(JsonPrefStore *pref_store, +void RunBasicJsonPrefStoreTest(JsonPrefStore* pref_store, const FilePath& output_file, const FilePath& golden_output_file) { const char kNewWindowsInTabs[] = "tabs.new_windows_in_tabs"; @@ -166,7 +164,8 @@ TEST_F(JsonPrefStoreTest, Basic) { FilePath input_file = temp_dir_.path().AppendASCII("write.json"); ASSERT_TRUE(file_util::PathExists(input_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(input_file, message_loop_proxy_.get()); + new JsonPrefStore( + input_file, message_loop_.message_loop_proxy()); ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs()); ASSERT_FALSE(pref_store->ReadOnly()); @@ -193,21 +192,24 @@ TEST_F(JsonPrefStoreTest, BasicAsync) { FilePath input_file = temp_dir_.path().AppendASCII("write.json"); ASSERT_TRUE(file_util::PathExists(input_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(input_file, message_loop_proxy_.get()); + new JsonPrefStore( + input_file, message_loop_.message_loop_proxy()); - MockPrefStoreObserver mock_observer; - pref_store->AddObserver(&mock_observer); + { + MockPrefStoreObserver mock_observer; + pref_store->AddObserver(&mock_observer); - MockReadErrorDelegate *mock_error_delegate = new MockReadErrorDelegate; - pref_store->ReadPrefsAsync(mock_error_delegate); + MockReadErrorDelegate* mock_error_delegate = new MockReadErrorDelegate; + pref_store->ReadPrefsAsync(mock_error_delegate); - EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(1); - EXPECT_CALL(*mock_error_delegate, - OnError(PersistentPrefStore::PREF_READ_ERROR_NONE)).Times(0); - message_loop_.RunUntilIdle(); - pref_store->RemoveObserver(&mock_observer); + EXPECT_CALL(mock_observer, OnInitializationCompleted(true)).Times(1); + EXPECT_CALL(*mock_error_delegate, + OnError(PersistentPrefStore::PREF_READ_ERROR_NONE)).Times(0); + message_loop_.RunUntilIdle(); + pref_store->RemoveObserver(&mock_observer); - ASSERT_FALSE(pref_store->ReadOnly()); + ASSERT_FALSE(pref_store->ReadOnly()); + } // The JSON file looks like this: // { @@ -229,7 +231,8 @@ TEST_F(JsonPrefStoreTest, AsyncNonExistingFile) { FilePath bogus_input_file = data_dir_.AppendASCII("read.txt"); ASSERT_FALSE(file_util::PathExists(bogus_input_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(bogus_input_file, message_loop_proxy_.get()); + new JsonPrefStore( + bogus_input_file, message_loop_.message_loop_proxy()); MockPrefStoreObserver mock_observer; pref_store->AddObserver(&mock_observer); @@ -255,7 +258,8 @@ TEST_F(JsonPrefStoreTest, NeedsEmptyValue) { // Test that the persistent value can be loaded. ASSERT_TRUE(file_util::PathExists(pref_file)); scoped_refptr<JsonPrefStore> pref_store = - new JsonPrefStore(pref_file, message_loop_proxy_.get()); + new JsonPrefStore( + pref_file, message_loop_.message_loop_proxy()); ASSERT_EQ(PersistentPrefStore::PREF_READ_ERROR_NONE, pref_store->ReadPrefs()); ASSERT_FALSE(pref_store->ReadOnly()); diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc index d28013b..28d7512 100644 --- a/chrome/browser/browser_process_impl.cc +++ b/chrome/browser/browser_process_impl.cc @@ -131,7 +131,9 @@ using content::ChildProcessSecurityPolicy; using content::PluginService; using content::ResourceDispatcherHost; -BrowserProcessImpl::BrowserProcessImpl(const CommandLine& command_line) +BrowserProcessImpl::BrowserProcessImpl( + base::SequencedTaskRunner* local_state_task_runner, + const CommandLine& command_line) : created_metrics_service_(false), created_watchdog_thread_(false), created_browser_policy_connector_(false), @@ -145,7 +147,8 @@ BrowserProcessImpl::BrowserProcessImpl(const CommandLine& command_line) checked_for_new_frames_(false), using_new_frames_(false), render_widget_snapshot_taker_(new RenderWidgetSnapshotTaker), - download_status_updater_(new DownloadStatusUpdater) { + download_status_updater_(new DownloadStatusUpdater), + local_state_task_runner_(local_state_task_runner) { g_browser_process = this; #if defined(ENABLE_PRINTING) @@ -712,10 +715,10 @@ void BrowserProcessImpl::CreateLocalState() { created_local_state_ = true; FilePath local_state_path; - PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path); + CHECK(PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path)); local_state_.reset( - PrefService::CreatePrefService(local_state_path, policy_service(), NULL, - false)); + PrefService::CreatePrefService(local_state_path, local_state_task_runner_, + policy_service(), NULL, false)); // Initialize the prefs of the local state. chrome::RegisterLocalState(local_state_.get()); diff --git a/chrome/browser/browser_process_impl.h b/chrome/browser/browser_process_impl.h index b354983..18c00ae 100644 --- a/chrome/browser/browser_process_impl.h +++ b/chrome/browser/browser_process_impl.h @@ -32,6 +32,10 @@ class RemoteDebuggingServer; class PluginsResourceService; #endif +namespace base { +class SequencedTaskRunner; +} + namespace policy { class BrowserPolicyConnector; class PolicyService; @@ -42,7 +46,9 @@ class BrowserProcessImpl : public BrowserProcess, public base::NonThreadSafe, public content::NotificationObserver { public: - explicit BrowserProcessImpl(const CommandLine& command_line); + // |local_state_task_runner| must be a shutdown-blocking task runner. + BrowserProcessImpl(base::SequencedTaskRunner* local_state_task_runner, + const CommandLine& command_line); virtual ~BrowserProcessImpl(); // Called before the browser threads are created. @@ -225,6 +231,9 @@ class BrowserProcessImpl : public BrowserProcess, scoped_refptr<DownloadRequestLimiter> download_request_limiter_; + // Sequenced task runner for local state related I/O tasks. + const scoped_refptr<base::SequencedTaskRunner> local_state_task_runner_; + // Ensures that the observers of plugin/print disable/enable state // notifications are properly added and removed. PrefChangeRegistrar pref_change_registrar_; diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc index 1d98c38..06d96b9 100644 --- a/chrome/browser/chrome_browser_main.cc +++ b/chrome/browser/chrome_browser_main.cc @@ -274,8 +274,11 @@ void InitializeNetworkOptions(const CommandLine& parsed_command_line, } // Returns the new local state object, guaranteed non-NULL. -PrefService* InitializeLocalState(const CommandLine& parsed_command_line, - bool is_first_run) { +// |local_state_task_runner| must be a shutdown-blocking task runner. +PrefService* InitializeLocalState( + base::SequencedTaskRunner* local_state_task_runner, + const CommandLine& parsed_command_line, + bool is_first_run) { FilePath local_state_path; PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path); bool local_state_file_exists = file_util::PathExists(local_state_path); @@ -329,7 +332,7 @@ PrefService* InitializeLocalState(const CommandLine& parsed_command_line, FilePath parent_profile = parsed_command_line.GetSwitchValuePath(switches::kParentProfile); scoped_ptr<PrefService> parent_local_state( - PrefService::CreatePrefService(parent_profile, + PrefService::CreatePrefService(parent_profile, local_state_task_runner, g_browser_process->policy_service(), NULL, false)); parent_local_state->RegisterStringPref(prefs::kApplicationLocale, @@ -710,7 +713,14 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() { parsed_command_line().HasSwitch(switches::kFirstRun)) && !HasImportSwitch(parsed_command_line()); #endif - browser_process_.reset(new BrowserProcessImpl(parsed_command_line())); + + FilePath local_state_path; + CHECK(PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path)); + scoped_refptr<base::SequencedTaskRunner> local_state_task_runner = + JsonPrefStore::GetTaskRunnerForFile(local_state_path, + BrowserThread::GetBlockingPool()); + browser_process_.reset(new BrowserProcessImpl(local_state_task_runner, + parsed_command_line())); if (parsed_command_line().HasSwitch(switches::kEnableProfiling)) { // User wants to override default tracking status. @@ -732,7 +742,9 @@ int ChromeBrowserMainParts::PreCreateThreadsImpl() { switches::kProfilingOutputFile)); } - local_state_ = InitializeLocalState(parsed_command_line(), is_first_run_); + local_state_ = InitializeLocalState(local_state_task_runner, + parsed_command_line(), + is_first_run_); // These members must be initialized before returning from this function. master_prefs_.reset(new first_run::MasterPrefs); diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc index fb8060e..22a8c50 100644 --- a/chrome/browser/extensions/extension_prefs_unittest.cc +++ b/chrome/browser/extensions/extension_prefs_unittest.cc @@ -52,10 +52,11 @@ static void AddPattern(URLPatternSet* extent, const std::string& pattern) { ExtensionPrefsTest::ExtensionPrefsTest() : ui_thread_(BrowserThread::UI, &message_loop_), - file_thread_(BrowserThread::FILE, &message_loop_) { + prefs_(message_loop_.message_loop_proxy()) { } -ExtensionPrefsTest::~ExtensionPrefsTest() {} +ExtensionPrefsTest::~ExtensionPrefsTest() { +} void ExtensionPrefsTest::RegisterPreferences() {} @@ -71,6 +72,8 @@ void ExtensionPrefsTest::TearDown() { prefs_.RecreateExtensionPrefs(); RegisterPreferences(); Verify(); + prefs_.pref_service()->CommitPendingWrite(); + message_loop_.RunAllPending(); } // Tests the LastPingDay/SetLastPingDay functions. diff --git a/chrome/browser/extensions/extension_prefs_unittest.h b/chrome/browser/extensions/extension_prefs_unittest.h index ba5832a..f96ffb6 100644 --- a/chrome/browser/extensions/extension_prefs_unittest.h +++ b/chrome/browser/extensions/extension_prefs_unittest.h @@ -44,7 +44,6 @@ class ExtensionPrefsTest : public testing::Test { MessageLoop message_loop_; content::TestBrowserThread ui_thread_; - content::TestBrowserThread file_thread_; TestExtensionPrefs prefs_; diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc index 1a68f05..e1e3dc4 100644 --- a/chrome/browser/extensions/extension_service_unittest.cc +++ b/chrome/browser/extensions/extension_service_unittest.cc @@ -422,7 +422,8 @@ void ExtensionServiceTestBase::InitializeExtensionService( TestingProfile::Builder profile_builder; // Create a PrefService that only contains user defined preference values. scoped_ptr<PrefService> prefs( - PrefServiceMockBuilder().WithUserFilePrefs(pref_file).Create()); + PrefServiceMockBuilder().WithUserFilePrefs( + pref_file, loop_.message_loop_proxy()).Create()); Profile::RegisterUserPrefs(prefs.get()); chrome::RegisterUserPrefs(prefs.get()); profile_builder.SetPrefService(prefs.Pass()); diff --git a/chrome/browser/extensions/menu_manager_unittest.cc b/chrome/browser/extensions/menu_manager_unittest.cc index ef20359..445a6c4 100644 --- a/chrome/browser/extensions/menu_manager_unittest.cc +++ b/chrome/browser/extensions/menu_manager_unittest.cc @@ -15,6 +15,7 @@ #include "chrome/browser/extensions/event_router.h" #include "chrome/browser/extensions/extension_system_factory.h" #include "chrome/browser/extensions/menu_manager.h" +#include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/extensions/test_extension_prefs.h" #include "chrome/browser/extensions/test_extension_system.h" #include "chrome/common/chrome_notification_types.h" @@ -44,9 +45,15 @@ class MenuManagerTest : public testing::Test { MenuManagerTest() : ui_thread_(BrowserThread::UI, &message_loop_), file_thread_(BrowserThread::FILE, &message_loop_), manager_(&profile_), + prefs_(message_loop_.message_loop_proxy()), next_id_(1) { } + virtual void TearDown() OVERRIDE { + prefs_.pref_service()->CommitPendingWrite(); + message_loop_.RunAllPending(); + } + // Returns a test item. MenuItem* CreateTestItem(Extension* extension, bool incognito = false) { MenuItem::Type type = MenuItem::NORMAL; diff --git a/chrome/browser/extensions/test_extension_prefs.cc b/chrome/browser/extensions/test_extension_prefs.cc index 7c3cf8e..f03df25 100644 --- a/chrome/browser/extensions/test_extension_prefs.cc +++ b/chrome/browser/extensions/test_extension_prefs.cc @@ -11,6 +11,8 @@ #include "base/message_loop.h" #include "base/message_loop_proxy.h" #include "base/prefs/json_pref_store.h" +#include "base/run_loop.h" +#include "base/sequenced_task_runner.h" #include "base/synchronization/waitable_event.h" #include "base/values.h" #include "chrome/browser/extensions/extension_pref_store.h" @@ -31,6 +33,8 @@ namespace extensions { namespace { +void DoNothing() {} + // Mock ExtensionPrefs class with artificial clock to guarantee that no two // extensions get the same installation time stamp and we can reliably // assert the installation order in the tests below. @@ -54,9 +58,10 @@ class MockExtensionPrefs : public ExtensionPrefs { } // namespace -TestExtensionPrefs::TestExtensionPrefs() - : pref_service_(NULL), - extensions_disabled_(false) { +TestExtensionPrefs::TestExtensionPrefs( + base::SequencedTaskRunner* task_runner) : pref_service_(NULL), + task_runner_(task_runner), + extensions_disabled_(false) { EXPECT_TRUE(temp_dir_.CreateUniqueTempDir()); preferences_file_ = temp_dir_.path().AppendASCII("Preferences"); extensions_dir_ = temp_dir_.path().AppendASCII("Extensions"); @@ -65,36 +70,29 @@ TestExtensionPrefs::TestExtensionPrefs() RecreateExtensionPrefs(); } -TestExtensionPrefs::~TestExtensionPrefs() {} +TestExtensionPrefs::~TestExtensionPrefs() { +} void TestExtensionPrefs::RecreateExtensionPrefs() { // We persist and reload the PrefService's PrefStores because this process // deletes all empty dictionaries. The ExtensionPrefs implementation // needs to be able to handle this situation. if (pref_service_.get()) { - // The PrefService writes its persistent file on the file thread, so we - // need to wait for any pending I/O to complete before creating a new - // PrefService. - base::WaitableEvent io_finished(false, false); - pref_service_-> CommitPendingWrite(); - EXPECT_TRUE(BrowserThread::PostTask( - BrowserThread::FILE, - FROM_HERE, - base::Bind(&base::WaitableEvent::Signal, - base::Unretained(&io_finished)))); - - // If the FILE thread is in fact the current thread (possible in testing - // scenarios), we have to ensure the task has a chance to run. If the FILE - // thread is a different thread, the test must ensure that thread is running - // (otherwise the Wait below will hang). - MessageLoop::current()->RunAllPending(); - - io_finished.Wait(); + // Commit a pending write (which posts a task to task_runner_) and wait for + // it to finish. + pref_service_->CommitPendingWrite(); + base::RunLoop run_loop; + ASSERT_TRUE( + task_runner_->PostTaskAndReply( + FROM_HERE, + base::Bind(&DoNothing), + run_loop.QuitClosure())); + run_loop.Run(); } extension_pref_value_map_.reset(new ExtensionPrefValueMap); PrefServiceMockBuilder builder; - builder.WithUserFilePrefs(preferences_file_); + builder.WithUserFilePrefs(preferences_file_, task_runner_); builder.WithExtensionPrefs( new ExtensionPrefStore(extension_pref_value_map_.get(), false)); pref_service_.reset(builder.Create()); diff --git a/chrome/browser/extensions/test_extension_prefs.h b/chrome/browser/extensions/test_extension_prefs.h index 5fb1ec5..b2b1faa 100644 --- a/chrome/browser/extensions/test_extension_prefs.h +++ b/chrome/browser/extensions/test_extension_prefs.h @@ -16,6 +16,7 @@ class PrefService; namespace base { class DictionaryValue; +class SequencedTaskRunner; } namespace extensions { @@ -25,7 +26,7 @@ class ExtensionPrefs; // in tests. class TestExtensionPrefs { public: - TestExtensionPrefs(); + explicit TestExtensionPrefs(base::SequencedTaskRunner* task_runner); virtual ~TestExtensionPrefs(); ExtensionPrefs* prefs() { return prefs_.get(); } @@ -76,6 +77,7 @@ class TestExtensionPrefs { scoped_ptr<PrefService> pref_service_; scoped_ptr<ExtensionPrefs> prefs_; scoped_ptr<ExtensionPrefValueMap> extension_pref_value_map_; + const scoped_refptr<base::SequencedTaskRunner> task_runner_; private: bool extensions_disabled_; diff --git a/chrome/browser/extensions/updater/extension_updater_unittest.cc b/chrome/browser/extensions/updater/extension_updater_unittest.cc index 5bf0548..8f0c464 100644 --- a/chrome/browser/extensions/updater/extension_updater_unittest.cc +++ b/chrome/browser/extensions/updater/extension_updater_unittest.cc @@ -162,10 +162,12 @@ class NotificationsObserver : public content::NotificationObserver { // Base class for further specialized test classes. class MockService : public TestExtensionService { public: - MockService() - : pending_extension_manager_(ALLOW_THIS_IN_INITIALIZER_LIST(*this)) { + explicit MockService(TestExtensionPrefs* prefs) + : prefs_(prefs), + pending_extension_manager_(ALLOW_THIS_IN_INITIALIZER_LIST(*this)) { profile_.CreateRequestContext(); } + virtual ~MockService() {} virtual PendingExtensionManager* pending_extension_manager() OVERRIDE { @@ -180,9 +182,9 @@ class MockService : public TestExtensionService { return profile_.GetRequestContext(); } - ExtensionPrefs* extension_prefs() { return prefs_.prefs(); } + ExtensionPrefs* extension_prefs() { return prefs_->prefs(); } - PrefService* pref_service() { return prefs_.pref_service(); } + PrefService* pref_service() { return prefs_->pref_service(); } // Creates test extensions and inserts them into list. The name and // version are all based on their index. If |update_url| is non-null, it @@ -201,15 +203,15 @@ class MockService : public TestExtensionService { if (update_url) manifest.SetString(extension_manifest_keys::kUpdateURL, *update_url); scoped_refptr<Extension> e = - prefs_.AddExtensionWithManifest(manifest, location); + prefs_->AddExtensionWithManifest(manifest, location); ASSERT_TRUE(e != NULL); list->push_back(e); } } protected: + TestExtensionPrefs* const prefs_; PendingExtensionManager pending_extension_manager_; - TestExtensionPrefs prefs_; TestingProfile profile_; private: @@ -260,7 +262,9 @@ void SetupPendingExtensionManagerForTest( class ServiceForManifestTests : public MockService { public: - ServiceForManifestTests() {} + explicit ServiceForManifestTests(TestExtensionPrefs* prefs) + : MockService(prefs) { + } virtual ~ServiceForManifestTests() {} @@ -305,8 +309,8 @@ class ServiceForManifestTests : public MockService { class ServiceForDownloadTests : public MockService { public: - ServiceForDownloadTests() - : MockService() { + explicit ServiceForDownloadTests(TestExtensionPrefs* prefs) + : MockService(prefs) { } // Add a fake crx installer to be returned by a call to UpdateExtension() @@ -369,8 +373,8 @@ class ServiceForDownloadTests : public MockService { class ServiceForBlacklistTests : public MockService { public: - ServiceForBlacklistTests() - : MockService(), + explicit ServiceForBlacklistTests(TestExtensionPrefs* prefs) + : MockService(prefs), processed_blacklist_(false) { } virtual void UpdateExtensionBlacklist( @@ -417,16 +421,26 @@ class ExtensionUpdaterTest : public testing::Test { ExtensionUpdaterTest() : ui_thread_(BrowserThread::UI, &loop_), file_thread_(BrowserThread::FILE, &loop_), - io_thread_(BrowserThread::IO, &loop_) {} + io_thread_(BrowserThread::IO, &loop_) { + } + + virtual ~ExtensionUpdaterTest() { + } + + virtual void SetUp() OVERRIDE { + prefs_.reset(new TestExtensionPrefs(loop_.message_loop_proxy())); + } virtual void TearDown() OVERRIDE { // Some tests create URLRequestContextGetters, whose destruction must run // on the IO thread. Make sure the IO loop spins before shutdown so that // those objects are released. - loop_.RunAllPending(); + RunAllPending(); + prefs_.reset(); } void RunAllPending() { + prefs_->pref_service()->CommitPendingWrite(); loop_.RunAllPending(); } @@ -466,7 +480,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestExtensionUpdateCheckRequests(bool pending) { // Create an extension with an update_url. - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); std::string update_url("http://foo.com/bar"); ExtensionList extensions; PendingExtensionManager* pending_extension_manager = @@ -526,7 +540,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestBlacklistUpdateCheckRequests() { // Setup and start the updater. - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); net::TestURLFetcherFactory factory; ExtensionUpdater updater( @@ -608,7 +622,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestUpdateUrlDataFromGallery(const std::string& gallery_url) { net::TestURLFetcherFactory factory; - MockService service; + MockService service(prefs_.get()); MockExtensionDownloaderDelegate delegate; ExtensionDownloader downloader(&delegate, service.request_context()); ExtensionList extensions; @@ -689,7 +703,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestDetermineUpdatesPending() { // Create a set of test extensions - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); PendingExtensionManager* pending_extension_manager = service.pending_extension_manager(); SetupPendingExtensionManagerForTest(3, GURL(), pending_extension_manager); @@ -731,7 +745,7 @@ class ExtensionUpdaterTest : public testing::Test { net::TestURLFetcherFactory factory; net::TestURLFetcher* fetcher = NULL; NotificationsObserver observer; - MockService service; + MockService service(prefs_.get()); MockExtensionDownloaderDelegate delegate; ExtensionDownloader downloader(&delegate, service.request_context()); @@ -843,7 +857,8 @@ class ExtensionUpdaterTest : public testing::Test { void TestSingleExtensionDownloading(bool pending) { net::TestURLFetcherFactory factory; net::TestURLFetcher* fetcher = NULL; - scoped_ptr<ServiceForDownloadTests> service(new ServiceForDownloadTests); + scoped_ptr<ServiceForDownloadTests> service( + new ServiceForDownloadTests(prefs_.get())); ExtensionUpdater updater(service.get(), service->extension_prefs(), service->pref_service(), service->profile(), @@ -899,7 +914,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestBlacklistDownloading() { net::TestURLFetcherFactory factory; net::TestURLFetcher* fetcher = NULL; - ServiceForBlacklistTests service; + ServiceForBlacklistTests service(prefs_.get()); ExtensionUpdater updater( &service, service.extension_prefs(), service.pref_service(), service.profile(), kUpdateFrequencySecs); @@ -947,7 +962,7 @@ class ExtensionUpdaterTest : public testing::Test { void TestMultipleExtensionDownloading(bool updates_start_running) { net::TestURLFetcherFactory factory; net::TestURLFetcher* fetcher = NULL; - ServiceForDownloadTests service; + ServiceForDownloadTests service(prefs_.get()); ExtensionUpdater updater( &service, service.extension_prefs(), service.pref_service(), service.profile(), kUpdateFrequencySecs); @@ -1116,7 +1131,8 @@ class ExtensionUpdaterTest : public testing::Test { // Set up 2 mock extensions, one with a google.com update url and one // without. - ServiceForManifestTests service; + prefs_.reset(new TestExtensionPrefs(loop_.message_loop_proxy())); + ServiceForManifestTests service(prefs_.get()); ExtensionList tmp; GURL url1("http://clients2.google.com/service/update2/crx"); GURL url2("http://www.somewebsite.com"); @@ -1228,6 +1244,8 @@ class ExtensionUpdaterTest : public testing::Test { // queries. EXPECT_TRUE(url1_query.find(brand_string) == std::string::npos); #endif + + RunAllPending(); } // This makes sure that the extension updater properly stores the results @@ -1235,7 +1253,7 @@ class ExtensionUpdaterTest : public testing::Test { // the first time we fetched the extension, or 2) We sent a ping value of // >= 1 day for the extension. void TestHandleManifestResults() { - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); GURL update_url("http://www.google.com/manifest"); ExtensionList tmp; service.CreateTestExtensions(1, 1, &tmp, &update_url.spec(), @@ -1265,6 +1283,9 @@ class ExtensionUpdaterTest : public testing::Test { EXPECT_LT(seconds_diff - results.daystart_elapsed_seconds, 5); } + protected: + scoped_ptr<TestExtensionPrefs> prefs_; + private: MessageLoop loop_; content::TestBrowserThread ui_thread_; @@ -1345,7 +1366,7 @@ TEST_F(ExtensionUpdaterTest, TestHandleManifestResults) { TEST_F(ExtensionUpdaterTest, TestNonAutoUpdateableLocations) { net::TestURLFetcherFactory factory; - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); ExtensionUpdater updater(&service, service.extension_prefs(), service.pref_service(), service.profile(), kUpdateFrequencySecs); @@ -1376,7 +1397,7 @@ TEST_F(ExtensionUpdaterTest, TestNonAutoUpdateableLocations) { TEST_F(ExtensionUpdaterTest, TestUpdatingDisabledExtensions) { net::TestURLFetcherFactory factory; - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); ExtensionUpdater updater(&service, service.extension_prefs(), service.pref_service(), service.profile(), kUpdateFrequencySecs); @@ -1414,7 +1435,7 @@ TEST_F(ExtensionUpdaterTest, TestUpdatingDisabledExtensions) { TEST_F(ExtensionUpdaterTest, TestManifestFetchesBuilderAddExtension) { net::TestURLFetcherFactory factory; - MockService service; + MockService service(prefs_.get()); MockExtensionDownloaderDelegate delegate; scoped_ptr<ExtensionDownloader> downloader( new ExtensionDownloader(&delegate, service.request_context())); @@ -1466,7 +1487,7 @@ TEST_F(ExtensionUpdaterTest, TestManifestFetchesBuilderAddExtension) { TEST_F(ExtensionUpdaterTest, TestStartUpdateCheckMemory) { net::TestURLFetcherFactory factory; - MockService service; + MockService service(prefs_.get()); MockExtensionDownloaderDelegate delegate; ExtensionDownloader downloader(&delegate, service.request_context()); @@ -1480,7 +1501,7 @@ TEST_F(ExtensionUpdaterTest, TestStartUpdateCheckMemory) { } TEST_F(ExtensionUpdaterTest, TestCheckSoon) { - ServiceForManifestTests service; + ServiceForManifestTests service(prefs_.get()); net::TestURLFetcherFactory factory; ExtensionUpdater updater( &service, service.extension_prefs(), service.pref_service(), diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc index 58f398d..680e4a5 100644 --- a/chrome/browser/metrics/metrics_service.cc +++ b/chrome/browser/metrics/metrics_service.cc @@ -1723,27 +1723,11 @@ bool MetricsService::UmaMetricsProperlyShutdown() { return clean_shutdown_status_ == CLEANLY_SHUTDOWN; } -// For use in hack in LogCleanShutdown. -static void Signal(base::WaitableEvent* event) { - event->Signal(); -} - void MetricsService::LogCleanShutdown() { // Redundant hack to write pref ASAP. PrefService* pref = g_browser_process->local_state(); pref->SetBoolean(prefs::kStabilityExitedCleanly, true); pref->CommitPendingWrite(); - // Hack: TBD: Remove this wait. - // We are so concerned that the pref gets written, we are now willing to stall - // the UI thread until we get assurance that a pref-writing task has - // completed. - base::WaitableEvent done_writing(false, false); - BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, - base::Bind(Signal, &done_writing)); - // http://crbug.com/124954 - base::ThreadRestrictions::ScopedAllowWait allow_wait; - done_writing.TimedWait(base::TimeDelta::FromHours(1)); - // Redundant setting to assure that we always reset this value at shutdown // (and that we don't use some alternate path, and not call LogCleanShutdown). clean_shutdown_status_ = CLEANLY_SHUTDOWN; diff --git a/chrome/browser/policy/managed_mode_policy_provider.cc b/chrome/browser/policy/managed_mode_policy_provider.cc index 7fecdb1..c55f87f 100644 --- a/chrome/browser/policy/managed_mode_policy_provider.cc +++ b/chrome/browser/policy/managed_mode_policy_provider.cc @@ -5,6 +5,7 @@ #include "chrome/browser/policy/managed_mode_policy_provider.h" #include "base/prefs/json_pref_store.h" +#include "base/threading/sequenced_worker_pool.h" #include "chrome/browser/policy/policy_bundle.h" #include "chrome/browser/profiles/profile.h" #include "chrome/common/chrome_constants.h" @@ -19,11 +20,11 @@ const char ManagedModePolicyProvider::kPolicies[] = "policies"; // static ManagedModePolicyProvider* ManagedModePolicyProvider::Create(Profile* profile) { - JsonPrefStore* pref_store = - new JsonPrefStore(profile->GetPath().Append( - chrome::kManagedModePolicyFilename), - BrowserThread::GetMessageLoopProxyForThread( - BrowserThread::FILE)); + FilePath path = profile->GetPath().Append(chrome::kManagedModePolicyFilename); + JsonPrefStore* pref_store = new JsonPrefStore( + path, + JsonPrefStore::GetTaskRunnerForFile(path, + BrowserThread::GetBlockingPool())); return new ManagedModePolicyProvider(pref_store); } diff --git a/chrome/browser/prefs/pref_service.cc b/chrome/browser/prefs/pref_service.cc index 8862f7f..eedef9b 100644 --- a/chrome/browser/prefs/pref_service.cc +++ b/chrome/browser/prefs/pref_service.cc @@ -123,6 +123,7 @@ PrefServiceBase* PrefServiceBase::FromBrowserContext(BrowserContext* context) { // static PrefService* PrefService::CreatePrefService( const FilePath& pref_filename, + base::SequencedTaskRunner* pref_io_task_runner, policy::PolicyService* policy_service, PrefStore* extension_prefs, bool async) { @@ -156,8 +157,7 @@ PrefService* PrefService::CreatePrefService( CommandLinePrefStore* command_line = new CommandLinePrefStore(CommandLine::ForCurrentProcess()); JsonPrefStore* user = new JsonPrefStore( - pref_filename, - BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE)); + pref_filename, pref_io_task_runner); DefaultPrefStore* default_pref_store = new DefaultPrefStore(); PrefNotifierImpl* pref_notifier = new PrefNotifierImpl(); diff --git a/chrome/browser/prefs/pref_service.h b/chrome/browser/prefs/pref_service.h index 82b9167..c855ef1 100644 --- a/chrome/browser/prefs/pref_service.h +++ b/chrome/browser/prefs/pref_service.h @@ -31,6 +31,10 @@ class PrefServiceObserver; class PrefStore; class PrefValueStore; +namespace base { + class SequencedTaskRunner; +} + namespace syncer { class SyncableService; } @@ -108,10 +112,12 @@ class PrefService : public PrefServiceBase, public base::NonThreadSafe { // the created PrefService or NULL if creation has failed. Note, it is // guaranteed that in asynchronous version initialization happens after this // function returned. - static PrefService* CreatePrefService(const FilePath& pref_filename, - policy::PolicyService* policy_service, - PrefStore* extension_pref_store, - bool async); + static PrefService* CreatePrefService( + const FilePath& pref_filename, + base::SequencedTaskRunner* pref_io_task_runner, + policy::PolicyService* policy_service, + PrefStore* extension_pref_store, + bool async); // Creates an incognito copy of the pref service that shares most pref stores // but uses a fresh non-persistent overlay for the user pref store and an diff --git a/chrome/browser/prefs/pref_service_mock_builder.cc b/chrome/browser/prefs/pref_service_mock_builder.cc index 2782882..77d8080 100644 --- a/chrome/browser/prefs/pref_service_mock_builder.cc +++ b/chrome/browser/prefs/pref_service_mock_builder.cc @@ -77,16 +77,17 @@ PrefServiceMockBuilder::WithCommandLine(CommandLine* command_line) { PrefServiceMockBuilder& PrefServiceMockBuilder::WithUserFilePrefs(const FilePath& prefs_file) { - return WithUserFilePrefs(prefs_file, - BrowserThread::GetMessageLoopProxyForThread( - BrowserThread::FILE)); + return WithUserFilePrefs( + prefs_file, + JsonPrefStore::GetTaskRunnerForFile(prefs_file, + BrowserThread::GetBlockingPool())); } PrefServiceMockBuilder& PrefServiceMockBuilder::WithUserFilePrefs( const FilePath& prefs_file, - base::MessageLoopProxy* message_loop_proxy) { - user_prefs_ = new JsonPrefStore(prefs_file, message_loop_proxy); + base::SequencedTaskRunner* task_runner) { + user_prefs_ = new JsonPrefStore(prefs_file, task_runner); return *this; } diff --git a/chrome/browser/prefs/pref_service_mock_builder.h b/chrome/browser/prefs/pref_service_mock_builder.h index b939ca6..708b622 100644 --- a/chrome/browser/prefs/pref_service_mock_builder.h +++ b/chrome/browser/prefs/pref_service_mock_builder.h @@ -15,7 +15,7 @@ class FilePath; class PrefService; namespace base { -class MessageLoopProxy; +class SequencedTaskRunner; } namespace policy { @@ -48,10 +48,11 @@ class PrefServiceMockBuilder { PrefServiceMockBuilder& WithCommandLine(CommandLine* command_line); // Specifies to use an actual file-backed user pref store. + // TODO(zelidrag): Remove the first overloaded method below. PrefServiceMockBuilder& WithUserFilePrefs(const FilePath& prefs_file); PrefServiceMockBuilder& WithUserFilePrefs( const FilePath& prefs_file, - base::MessageLoopProxy* message_loop_proxy); + base::SequencedTaskRunner* task_runner); // Creates the PrefService, invalidating the entire builder configuration. PrefService* Create(); diff --git a/chrome/browser/prefs/pref_service_unittest.cc b/chrome/browser/prefs/pref_service_unittest.cc index 2dc90a8..cd046e8 100644 --- a/chrome/browser/prefs/pref_service_unittest.cc +++ b/chrome/browser/prefs/pref_service_unittest.cc @@ -315,7 +315,7 @@ TEST_F(PrefServiceUserFilePrefsTest, PreserveEmptyValue) { pref_file)); PrefServiceMockBuilder builder; - builder.WithUserFilePrefs(pref_file, base::MessageLoopProxy::current()); + builder.WithUserFilePrefs(pref_file, message_loop_.message_loop_proxy()); scoped_ptr<PrefService> prefs(builder.Create()); // Register testing prefs. @@ -344,7 +344,7 @@ TEST_F(PrefServiceUserFilePrefsTest, PreserveEmptyValue) { // Write to file. prefs->CommitPendingWrite(); - MessageLoop::current()->RunAllPending(); + message_loop_.RunAllPending(); // Compare to expected output. FilePath golden_output_file = diff --git a/chrome/browser/profiles/profile_impl.cc b/chrome/browser/profiles/profile_impl.cc index 0766394..1d917aa 100644 --- a/chrome/browser/profiles/profile_impl.cc +++ b/chrome/browser/profiles/profile_impl.cc @@ -12,10 +12,13 @@ #include "base/file_util.h" #include "base/memory/scoped_ptr.h" #include "base/path_service.h" +#include "base/prefs/json_pref_store.h" #include "base/string_number_conversions.h" #include "base/string_tokenizer.h" #include "base/string_util.h" #include "base/stringprintf.h" +#include "base/synchronization/waitable_event.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/utf_string_conversions.h" #include "base/version.h" #include "chrome/browser/autocomplete/autocomplete_classifier.h" @@ -148,6 +151,38 @@ static const char kReadmeText[] = const char* const kPrefExitTypeCrashed = "Crashed"; const char* const kPrefExitTypeSessionEnded = "SessionEnded"; +// Helper method needed because PostTask cannot currently take a Callback +// function with non-void return type. +// TODO(jhawkins): Remove once IgnoreResult is fixed. +void CreateDirectoryAndSignal(const FilePath& path, + base::WaitableEvent* done_creating) { + file_util::CreateDirectory(path); + done_creating->Signal(); +} + +// Task that blocks the FILE thread until CreateDirectoryAndSignal() finishes on +// blocking I/O pool. +void BlockFileThreadOnDirectoryCreate(base::WaitableEvent* done_creating) { + done_creating->Wait(); +} + +// Initiates creation of profile directory on |sequenced_task_runner| and +// ensures that FILE thread is blocked until that operation finishes. +void CreateProfileDirectory(base::SequencedTaskRunner* sequenced_task_runner, + const FilePath& path) { + base::WaitableEvent* done_creating = new base::WaitableEvent(false, false); + sequenced_task_runner->PostTask(FROM_HERE, + base::Bind(&CreateDirectoryAndSignal, + path, + done_creating)); + // Block the FILE thread until directory is created on I/O pool to make sure + // that we don't attempt any operation until that part completes. + BrowserThread::PostTask( + BrowserThread::FILE, FROM_HERE, + base::Bind(&BlockFileThreadOnDirectoryCreate, + base::Owned(done_creating))); +} + FilePath GetCachePath(const FilePath& base) { return base.Append(chrome::kCacheDirname); } @@ -198,12 +233,15 @@ std::string ExitTypeToSessionTypePrefValue(Profile::ExitType type) { Profile* Profile::CreateProfile(const FilePath& path, Delegate* delegate, CreateMode create_mode) { + // Get sequenced task runner for making sure that file operations of + // this profile (defined by |path|) are executed in expected order + // (what was previously assured by the FILE thread). + scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner = + JsonPrefStore::GetTaskRunnerForFile(path, + BrowserThread::GetBlockingPool()); if (create_mode == CREATE_MODE_ASYNCHRONOUS) { DCHECK(delegate); - // This is safe while all file operations are done on the FILE thread. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(base::IgnoreResult(&file_util::CreateDirectory), path)); + CreateProfileDirectory(sequenced_task_runner, path); } else if (create_mode == CREATE_MODE_SYNCHRONOUS) { if (!file_util::PathExists(path)) { // TODO(tc): http://b/1094718 Bad things happen if we can't write to the @@ -216,7 +254,7 @@ Profile* Profile::CreateProfile(const FilePath& path, NOTREACHED(); } - return new ProfileImpl(path, delegate, create_mode); + return new ProfileImpl(path, delegate, create_mode, sequenced_task_runner); } // static @@ -269,9 +307,11 @@ void ProfileImpl::RegisterUserPrefs(PrefService* prefs) { PrefService::SYNCABLE_PREF); } -ProfileImpl::ProfileImpl(const FilePath& path, - Delegate* delegate, - CreateMode create_mode) +ProfileImpl::ProfileImpl( + const FilePath& path, + Delegate* delegate, + CreateMode create_mode, + base::SequencedTaskRunner* sequenced_task_runner) : path_(path), ALLOW_THIS_IN_INITIALIZER_LIST(io_data_(this)), host_content_settings_map_(NULL), @@ -316,6 +356,7 @@ ProfileImpl::ProfileImpl(const FilePath& path, if (create_mode == CREATE_MODE_ASYNCHRONOUS) { prefs_.reset(PrefService::CreatePrefService( GetPrefFilePath(), + sequenced_task_runner, policy_service_.get(), new ExtensionPrefStore( ExtensionPrefValueMapFactory::GetForProfile(this), false), @@ -328,6 +369,7 @@ ProfileImpl::ProfileImpl(const FilePath& path, // Load prefs synchronously. prefs_.reset(PrefService::CreatePrefService( GetPrefFilePath(), + sequenced_task_runner, policy_service_.get(), new ExtensionPrefStore( ExtensionPrefValueMapFactory::GetForProfile(this), false), @@ -351,10 +393,10 @@ void ProfileImpl::DoFinalInit(bool is_new_profile) { // to PathService. chrome::GetUserCacheDirectory(path_, &base_cache_path_); // Always create the cache directory asynchronously. - BrowserThread::PostTask( - BrowserThread::FILE, FROM_HERE, - base::Bind(base::IgnoreResult(&file_util::CreateDirectory), - base_cache_path_)); + scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner = + JsonPrefStore::GetTaskRunnerForFile(base_cache_path_, + BrowserThread::GetBlockingPool()); + CreateProfileDirectory(sequenced_task_runner, base_cache_path_); // Now that the profile is hooked up to receive pref change notifications to // kGoogleServicesUsername, initialize components that depend on it to reflect diff --git a/chrome/browser/profiles/profile_impl.h b/chrome/browser/profiles/profile_impl.h index 533b68b..ad7c17f 100644 --- a/chrome/browser/profiles/profile_impl.h +++ b/chrome/browser/profiles/profile_impl.h @@ -33,6 +33,10 @@ class Preferences; } #endif +namespace base { +class SequencedTaskRunner; +} + namespace content { class SpeechRecognitionPreferences; } @@ -141,7 +145,8 @@ class ProfileImpl : public Profile, ProfileImpl(const FilePath& path, Delegate* delegate, - CreateMode create_mode); + CreateMode create_mode, + base::SequencedTaskRunner* sequenced_task_runner); // Does final initialization. Should be called after prefs were loaded. void DoFinalInit(bool is_new_profile); diff --git a/chrome/browser/sync/credential_cache_service_win.cc b/chrome/browser/sync/credential_cache_service_win.cc index 23ac826..9c938d0 100644 --- a/chrome/browser/sync/credential_cache_service_win.cc +++ b/chrome/browser/sync/credential_cache_service_win.cc @@ -693,8 +693,9 @@ void CredentialCacheService::LookForCachedCredentialsInAlternateProfile() { // Attempt to read cached credentials from the alternate profile. If no file // exists, ReadPrefsAsync() will cause PREF_READ_ERROR_NO_FILE to be returned // after initialization is complete. + FilePath path = GetCredentialPathInAlternateProfile(); alternate_store_ = new JsonPrefStore( - GetCredentialPathInAlternateProfile(), + path, content::BrowserThread::GetMessageLoopProxyForThread( content::BrowserThread::FILE)); alternate_store_observer_ = new AlternateStoreObserver(this, diff --git a/chrome/service/cloud_print/connector_settings_unittest.cc b/chrome/service/cloud_print/connector_settings_unittest.cc index 09ceeef..b86c315 100644 --- a/chrome/service/cloud_print/connector_settings_unittest.cc +++ b/chrome/service/cloud_print/connector_settings_unittest.cc @@ -44,7 +44,7 @@ const char kServiceStateContent[] = class ConnectorSettingsTest : public testing::Test { protected: - virtual void SetUp() { + virtual void SetUp() OVERRIDE { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); message_loop_proxy_ = base::MessageLoopProxy::current(); } @@ -58,7 +58,7 @@ class ConnectorSettingsTest : public testing::Test { file_util::WriteFile(file_name, content.c_str(), content.size()); } ServiceProcessPrefs* prefs = - new ServiceProcessPrefs(file_name, message_loop_proxy_.get()); + new ServiceProcessPrefs(file_name, message_loop_proxy_); prefs->ReadPrefs(); return prefs; } diff --git a/chrome/service/service_process.cc b/chrome/service/service_process.cc index f820f93..40607d9 100644 --- a/chrome/service/service_process.cc +++ b/chrome/service/service_process.cc @@ -13,6 +13,7 @@ #include "base/i18n/rtl.h" #include "base/memory/singleton.h" #include "base/path_service.h" +#include "base/prefs/json_pref_store.h" #include "base/string16.h" #include "base/utf_string_conversions.h" #include "base/values.h" @@ -152,6 +153,7 @@ bool ServiceProcess::Initialize(MessageLoopForUI* message_loop, Teardown(); return false; } + blocking_pool_ = new base::SequencedWorkerPool(3, "ServiceBlocking"); request_context_getter_ = new ServiceURLRequestContextGetter(); @@ -159,7 +161,9 @@ bool ServiceProcess::Initialize(MessageLoopForUI* message_loop, PathService::Get(chrome::DIR_USER_DATA, &user_data_dir); FilePath pref_path = user_data_dir.Append(chrome::kServiceStateFileName); service_prefs_.reset( - new ServiceProcessPrefs(pref_path, file_thread_->message_loop_proxy())); + new ServiceProcessPrefs( + pref_path, + JsonPrefStore::GetTaskRunnerForFile(pref_path, blocking_pool_))); service_prefs_->ReadPrefs(); // Check if a locale override has been specified on the command-line. @@ -218,6 +222,12 @@ bool ServiceProcess::Teardown() { shutdown_event_.Signal(); io_thread_.reset(); file_thread_.reset(); + + if (blocking_pool_.get()) { + blocking_pool_->Shutdown(); + blocking_pool_ = NULL; + } + // The NetworkChangeNotifier must be destroyed after all other threads that // might use it have been shut down. network_change_notifier_.reset(); diff --git a/chrome/service/service_process.h b/chrome/service/service_process.h index 47617ae..af2afb5 100644 --- a/chrome/service/service_process.h +++ b/chrome/service/service_process.h @@ -10,6 +10,7 @@ #include "base/gtest_prod_util.h" #include "base/memory/ref_counted.h" #include "base/memory/scoped_ptr.h" +#include "base/threading/sequenced_worker_pool.h" #include "base/threading/thread.h" #include "base/synchronization/waitable_event.h" #include "chrome/service/cloud_print/cloud_print_proxy.h" @@ -123,6 +124,7 @@ class ServiceProcess : public CloudPrintProxy::Client { scoped_ptr<net::NetworkChangeNotifier> network_change_notifier_; scoped_ptr<base::Thread> io_thread_; scoped_ptr<base::Thread> file_thread_; + scoped_refptr<base::SequencedWorkerPool> blocking_pool_; scoped_ptr<CloudPrintProxy> cloud_print_proxy_; scoped_ptr<ServiceProcessPrefs> service_prefs_; scoped_ptr<ServiceIPCServer> ipc_server_; diff --git a/chrome/service/service_process_prefs.cc b/chrome/service/service_process_prefs.cc index 811647d..e417988 100644 --- a/chrome/service/service_process_prefs.cc +++ b/chrome/service/service_process_prefs.cc @@ -4,12 +4,13 @@ #include "chrome/service/service_process_prefs.h" +#include "base/message_loop_proxy.h" #include "base/values.h" ServiceProcessPrefs::ServiceProcessPrefs( const FilePath& pref_filename, - base::MessageLoopProxy* file_message_loop_proxy) - : prefs_(new JsonPrefStore(pref_filename, file_message_loop_proxy)) { + base::SequencedTaskRunner* task_runner) + : prefs_(new JsonPrefStore(pref_filename, task_runner)) { } ServiceProcessPrefs::~ServiceProcessPrefs() {} diff --git a/chrome/service/service_process_prefs.h b/chrome/service/service_process_prefs.h index 018ce1d..fdbf22e 100644 --- a/chrome/service/service_process_prefs.h +++ b/chrome/service/service_process_prefs.h @@ -12,16 +12,16 @@ namespace base { class DictionaryValue; class ListValue; +class SequencedTaskRunner; } // Manages persistent preferences for the service process. This is basically a // thin wrapper around JsonPrefStore for more comfortable use. class ServiceProcessPrefs { public: - // |file_message_loop_proxy| is the MessageLoopProxy for a thread on which - // file I/O can be done. + // |sequenced_task_runner| must be a shutdown-blocking task runner. ServiceProcessPrefs(const FilePath& pref_filename, - base::MessageLoopProxy* file_message_loop_proxy); + base::SequencedTaskRunner* task_runner); ~ServiceProcessPrefs(); // Read preferences from the backing file. diff --git a/chrome/service/service_process_prefs_unittest.cc b/chrome/service/service_process_prefs_unittest.cc index 0268395..5ce2808 100644 --- a/chrome/service/service_process_prefs_unittest.cc +++ b/chrome/service/service_process_prefs_unittest.cc @@ -5,29 +5,30 @@ #include <string> #include "base/message_loop.h" -#include "base/message_loop_proxy.h" #include "base/scoped_temp_dir.h" +#include "base/sequenced_task_runner.h" #include "chrome/service/service_process_prefs.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" class ServiceProcessPrefsTest : public testing::Test { protected: - virtual void SetUp() { - message_loop_proxy_ = base::MessageLoopProxy::current(); - + virtual void SetUp() OVERRIDE { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); prefs_.reset(new ServiceProcessPrefs( temp_dir_.path().AppendASCII("service_process_prefs.txt"), - message_loop_proxy_.get())); + message_loop_.message_loop_proxy())); + } + + virtual void TearDown() OVERRIDE { + prefs_.reset(); } // The path to temporary directory used to contain the test operations. ScopedTempDir temp_dir_; // A message loop that we can use as the file thread message loop. MessageLoop message_loop_; - scoped_refptr<base::MessageLoopProxy> message_loop_proxy_; scoped_ptr<ServiceProcessPrefs> prefs_; }; @@ -36,7 +37,7 @@ TEST_F(ServiceProcessPrefsTest, RetrievePrefs) { prefs_->SetBoolean("testb", true); prefs_->SetString("tests", "testvalue"); prefs_->WritePrefs(); - MessageLoop::current()->RunAllPending(); + message_loop_.RunAllPending(); prefs_->SetBoolean("testb", false); // overwrite prefs_->SetString("tests", ""); // overwrite prefs_->ReadPrefs(); diff --git a/chrome/test/reliability/page_load_test.cc b/chrome/test/reliability/page_load_test.cc index 9e3909d..9ac520c 100644 --- a/chrome/test/reliability/page_load_test.cc +++ b/chrome/test/reliability/page_load_test.cc @@ -739,7 +739,8 @@ class PageLoadTest : public UITest { // returned PrefService object. PrefService* GetLocalState() { FilePath path = user_data_dir().Append(chrome::kLocalStateFilename); - return PrefServiceMockBuilder().WithUserFilePrefs(path).Create(); + return PrefServiceMockBuilder().WithUserFilePrefs( + path, MessageLoop::current()->message_loop_proxy()).Create(); } void GetStabilityMetrics(NavigationMetrics* metrics) { diff --git a/chrome_frame/test/net/fake_external_tab.cc b/chrome_frame/test/net/fake_external_tab.cc index 71774ee..00e86e7 100644 --- a/chrome_frame/test/net/fake_external_tab.cc +++ b/chrome_frame/test/net/fake_external_tab.cc @@ -18,6 +18,7 @@ #include "base/lazy_instance.h" #include "base/memory/scoped_ptr.h" #include "base/path_service.h" +#include "base/prefs/json_pref_store.h" #include "base/scoped_temp_dir.h" #include "base/string_piece.h" #include "base/string_util.h" @@ -351,8 +352,9 @@ void FilterDisabledTests() { // Same as BrowserProcessImpl, but uses custom profile manager. class FakeBrowserProcessImpl : public BrowserProcessImpl { public: - explicit FakeBrowserProcessImpl(const CommandLine& command_line) - : BrowserProcessImpl(command_line) { + FakeBrowserProcessImpl(base::SequencedTaskRunner* local_state_task_runner, + const CommandLine& command_line) + : BrowserProcessImpl(local_state_task_runner, command_line) { profiles_dir_.CreateUniqueTempDir(); } @@ -493,7 +495,13 @@ void FakeExternalTab::Initialize() { cmd->AppendSwitch(switches::kDisableWebResources); cmd->AppendSwitch(switches::kSingleProcess); - browser_process_.reset(new FakeBrowserProcessImpl(*cmd)); + FilePath local_state_path; + CHECK(PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path)); + scoped_refptr<base::SequencedTaskRunner> local_state_task_runner = + JsonPrefStore::GetTaskRunnerForFile(local_state_path, + BrowserThread::GetBlockingPool()); + browser_process_.reset(new FakeBrowserProcessImpl(local_state_task_runner, + *cmd)); // BrowserProcessImpl's constructor should set g_browser_process. DCHECK(g_browser_process); g_browser_process->SetApplicationLocale("en-US"); |