diff options
26 files changed, 453 insertions, 200 deletions
diff --git a/base/file_util.cc b/base/file_util.cc index f7a0411..8eec3ac 100644 --- a/base/file_util.cc +++ b/base/file_util.cc @@ -188,6 +188,14 @@ bool ReadFileToString(const FilePath& path, std::string* contents) { return true; } +FILE* CreateAndOpenTemporaryFile(FilePath* path) { + FilePath directory; + if (!GetTempDir(&directory)) + return false; + + return CreateAndOpenTemporaryFileInDir(directory, path); +} + bool GetFileSize(const FilePath& file_path, int64* file_size) { FileInfo info; if (!GetFileInfo(file_path, &info)) diff --git a/base/file_util.h b/base/file_util.h index 489cbcc..47254ab 100644 --- a/base/file_util.h +++ b/base/file_util.h @@ -296,6 +296,9 @@ FILE* CreateAndOpenTemporaryFile(FilePath* path); // Like above but for shmem files. Only useful for POSIX. FILE* CreateAndOpenTemporaryShmemFile(FilePath* path); +// Similar to CreateAndOpenTemporaryFile, but the file is created in |dir|. +FILE* CreateAndOpenTemporaryFileInDir(const FilePath& dir, FilePath* path); + // Same as CreateTemporaryFileName but the file is created in |dir|. bool CreateTemporaryFileNameInDir(const std::wstring& dir, std::wstring* temp_file); diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc index f218460..1835d3a 100644 --- a/base/file_util_posix.cc +++ b/base/file_util_posix.cc @@ -369,30 +369,20 @@ bool CreateTemporaryFileName(FilePath* path) { return true; } -FILE* CreateAndOpenTemporaryFile(FilePath* path) { - FilePath directory; - if (!GetTempDir(&directory)) - return false; - - int fd = CreateAndOpenFdForTemporaryFile(directory, path); - if (fd < 0) - return NULL; - - FILE *fp = fdopen(fd, "a+"); - return fp; -} - FILE* CreateAndOpenTemporaryShmemFile(FilePath* path) { FilePath directory; if (!GetShmemTempDir(&directory)) return false; - int fd = CreateAndOpenFdForTemporaryFile(directory, path); + return CreateAndOpenTemporaryFileInDir(directory, path); +} + +FILE* CreateAndOpenTemporaryFileInDir(const FilePath& dir, FilePath* path) { + int fd = CreateAndOpenFdForTemporaryFile(dir, path); if (fd < 0) return NULL; - FILE *fp = fdopen(fd, "a+"); - return fp; + return fdopen(fd, "a+"); } bool CreateTemporaryFileNameInDir(const std::wstring& dir, diff --git a/base/file_util_win.cc b/base/file_util_win.cc index 8b3d4f5..8964fbf 100644 --- a/base/file_util_win.cc +++ b/base/file_util_win.cc @@ -446,20 +446,24 @@ bool CreateTemporaryFileName(FilePath* path) { return false; } +FILE* CreateAndOpenTemporaryShmemFile(FilePath* path) { + return CreateAndOpenTemporaryFile(path); +} + // On POSIX we have semantics to create and open a temporary file // atomically. // TODO(jrg): is there equivalent call to use on Windows instead of // going 2-step? -FILE* CreateAndOpenTemporaryFile(FilePath* path) { - - if (!CreateTemporaryFileName(path)) { +FILE* CreateAndOpenTemporaryFileInDir(const FilePath& dir, FilePath* path) { + std::wstring wstring_path; + if (!CreateTemporaryFileNameInDir(dir.value(), &wstring_path)) { return NULL; } - return OpenFile(*path, "w+"); -} - -FILE* CreateAndOpenTemporaryShmemFile(FilePath* path) { - return CreateAndOpenTemporaryFile(path); + *path = FilePath(wstring_path); + // Open file in binary mode, to avoid problems with fwrite. On Windows + // it replaces \n's with \r\n's, which may surprise you. + // Reference: http://msdn.microsoft.com/en-us/library/h9t88zwz(VS.71).aspx + return OpenFile(*path, "wb+"); } bool CreateTemporaryFileNameInDir(const std::wstring& dir, diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc index e22911f..e20e545 100644 --- a/chrome/browser/bookmarks/bookmark_utils.cc +++ b/chrome/browser/bookmarks/bookmark_utils.cc @@ -538,7 +538,7 @@ void ToggleWhenVisible(Profile* profile) { // The user changed when the bookmark bar is shown, update the preferences. prefs->SetBoolean(prefs::kShowBookmarkBar, always_show); - prefs->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs->ScheduleSavePersistentPrefs(); // And notify the notification service. Source<Profile> source(profile); diff --git a/chrome/browser/browser_main.cc b/chrome/browser/browser_main.cc index abd1d0b..fd9ee3d 100644 --- a/chrome/browser/browser_main.cc +++ b/chrome/browser/browser_main.cc @@ -298,7 +298,8 @@ int BrowserMain(const MainFunctionParams& parameters) { parsed_command_line.HasSwitch(switches::kParentProfile)) { FilePath parent_profile = FilePath::FromWStringHack( parsed_command_line.GetSwitchValue(switches::kParentProfile)); - PrefService parent_local_state(parent_profile); + PrefService parent_local_state(parent_profile, + g_browser_process->file_thread()); parent_local_state.RegisterStringPref(prefs::kApplicationLocale, std::wstring()); // Right now, we only inherit the locale setting from the parent profile. diff --git a/chrome/browser/browser_process_impl.cc b/chrome/browser/browser_process_impl.cc index 5b7b6b6..085f773 100644 --- a/chrome/browser/browser_process_impl.cc +++ b/chrome/browser/browser_process_impl.cc @@ -233,7 +233,7 @@ void BrowserProcessImpl::EndSession() { metrics->RecordStartOfSessionEnd(); // MetricsService lazily writes to prefs, force it to write now. - local_state()->SavePersistentPrefs(file_thread()); + local_state()->SavePersistentPrefs(); } // We must write that the profile and metrics service shutdown cleanly, @@ -351,7 +351,7 @@ void BrowserProcessImpl::CreateLocalState() { FilePath local_state_path; PathService::Get(chrome::FILE_LOCAL_STATE, &local_state_path); - local_state_.reset(new PrefService(local_state_path)); + local_state_.reset(new PrefService(local_state_path, file_thread())); } void BrowserProcessImpl::InitBrokerServices( diff --git a/chrome/browser/browser_shutdown.cc b/chrome/browser/browser_shutdown.cc index 6c2e0fb..c37a975 100644 --- a/chrome/browser/browser_shutdown.cc +++ b/chrome/browser/browser_shutdown.cc @@ -124,7 +124,7 @@ void Shutdown() { shutdown_num_processes_slow_); } - prefs->SavePersistentPrefs(g_browser_process->file_thread()); + prefs->SavePersistentPrefs(); // Cleanup any statics created by RLZ. Must be done before NotificationService // is destroyed. diff --git a/chrome/browser/importer/importer.cc b/chrome/browser/importer/importer.cc index 370ec9e..70e98a8 100644 --- a/chrome/browser/importer/importer.cc +++ b/chrome/browser/importer/importer.cc @@ -87,7 +87,7 @@ void ProfileWriter::AddHomepage(const GURL& home_page) { PrefService* prefs = profile_->GetPrefs(); // NOTE: We set the kHomePage value, but keep the NewTab page as the homepage. prefs->SetString(prefs::kHomePage, ASCIIToWide(home_page.spec())); - prefs->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs->ScheduleSavePersistentPrefs(); } void ProfileWriter::AddBookmarkEntry( @@ -295,7 +295,7 @@ void ProfileWriter::ShowBookmarkBar() { if (!prefs->GetBoolean(prefs::kShowBookmarkBar)) { // Set the pref and notify the notification service. prefs->SetBoolean(prefs::kShowBookmarkBar, true); - prefs->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs->ScheduleSavePersistentPrefs(); Source<Profile> source(profile_); NotificationService::current()->Notify( NotificationType::BOOKMARK_BAR_VISIBILITY_PREF_CHANGED, source, diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc index 6cdbb8b..c54b36e 100644 --- a/chrome/browser/metrics/metrics_service.cc +++ b/chrome/browser/metrics/metrics_service.cc @@ -728,7 +728,7 @@ void MetricsService::SaveLocalState() { } RecordCurrentState(pref); - pref->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + pref->ScheduleSavePersistentPrefs(); // TODO(jar): Does this run down the batteries???? ScheduleNextStateSave(); @@ -1298,8 +1298,7 @@ void MetricsService::OnURLFetchComplete(const URLFetcher* source, PrefService* local_state = g_browser_process->local_state(); DCHECK(local_state); if (local_state) - local_state->ScheduleSavePersistentPrefs( - g_browser_process->file_thread()); + local_state->ScheduleSavePersistentPrefs(); // Provide a default (free of exponetial backoff, other varances) in case // the server does not specify a value. diff --git a/chrome/browser/metrics/metrics_service_uitest.cc b/chrome/browser/metrics/metrics_service_uitest.cc index 880d648..9f208f0 100644 --- a/chrome/browser/metrics/metrics_service_uitest.cc +++ b/chrome/browser/metrics/metrics_service_uitest.cc @@ -52,7 +52,7 @@ class MetricsServiceTest : public UITest { FilePath local_state_path = user_data_dir() .Append(chrome::kLocalStateFilename); - PrefService* local_state(new PrefService(local_state_path)); + PrefService* local_state(new PrefService(local_state_path, NULL)); return local_state; } diff --git a/chrome/browser/profile.cc b/chrome/browser/profile.cc index a234ac5..2a95f9e 100644 --- a/chrome/browser/profile.cc +++ b/chrome/browser/profile.cc @@ -573,7 +573,8 @@ SSLHostState* ProfileImpl::GetSSLHostState() { PrefService* ProfileImpl::GetPrefs() { if (!prefs_.get()) { - prefs_.reset(new PrefService(GetPrefFilePath())); + prefs_.reset(new PrefService(GetPrefFilePath(), + g_browser_process->file_thread())); // The Profile class and ProfileManager class may read some prefs so // register known prefs as soon as possible. @@ -587,7 +588,7 @@ PrefService* ProfileImpl::GetPrefs() { // Mark the session as open. prefs_->SetBoolean(prefs::kSessionExitedCleanly, false); // Make sure we save to disk that the session has opened. - prefs_->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs_->ScheduleSavePersistentPrefs(); } return prefs_.get(); @@ -879,7 +880,7 @@ void ProfileImpl::MarkAsCleanShutdown() { // NOTE: If you change what thread this writes on, be sure and update // ChromeFrame::EndSession(). - prefs_->SavePersistentPrefs(g_browser_process->file_thread()); + prefs_->SavePersistentPrefs(); } } diff --git a/chrome/browser/search_engines/template_url_model.cc b/chrome/browser/search_engines/template_url_model.cc index 982de9d..9399a95 100644 --- a/chrome/browser/search_engines/template_url_model.cc +++ b/chrome/browser/search_engines/template_url_model.cc @@ -795,7 +795,7 @@ void TemplateURLModel::SaveDefaultSearchProviderToPrefs( t_url ? Int64ToWString(t_url->id()) : std::wstring(); prefs->SetString(prefs::kDefaultSearchProviderID, id_string); - prefs->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs->ScheduleSavePersistentPrefs(); } bool TemplateURLModel::LoadDefaultSearchProviderFromPrefs( diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc index 37d49ee..1a95044 100644 --- a/chrome/browser/tab_contents/web_contents_unittest.cc +++ b/chrome/browser/tab_contents/web_contents_unittest.cc @@ -47,7 +47,7 @@ class WebContentsTestingProfile : public TestingProfile { source_path = source_path.AppendASCII("profiles") .AppendASCII("chrome_prefs").AppendASCII("Preferences"); - prefs_.reset(new PrefService(source_path)); + prefs_.reset(new PrefService(source_path, NULL)); Profile::RegisterUserPrefs(prefs_.get()); browser::RegisterAllPrefs(prefs_.get(), prefs_.get()); } diff --git a/chrome/browser/views/options/options_page_view.cc b/chrome/browser/views/options/options_page_view.cc index 9011bac..ed303ad 100644 --- a/chrome/browser/views/options/options_page_view.cc +++ b/chrome/browser/views/options/options_page_view.cc @@ -25,7 +25,7 @@ void OptionsPageView::UserMetricsRecordAction(const wchar_t* action, PrefService* prefs) { UserMetrics::RecordComputedAction(action, profile()); if (prefs) - prefs->ScheduleSavePersistentPrefs(g_browser_process->file_thread()); + prefs->ScheduleSavePersistentPrefs(); } /////////////////////////////////////////////////////////////////////////////// diff --git a/chrome/chrome.gyp b/chrome/chrome.gyp index d85abc6b..6108e42 100644 --- a/chrome/chrome.gyp +++ b/chrome/chrome.gyp @@ -182,6 +182,8 @@ 'common/file_descriptor_set_posix.h', 'common/filter_policy.h', 'common/gears_api.h', + 'common/important_file_writer.cc', + 'common/important_file_writer.h', 'common/ipc_channel.h', 'common/ipc_channel_posix.cc', 'common/ipc_channel_posix.h', @@ -2352,6 +2354,7 @@ 'common/gfx/emf_unittest.cc', 'common/gfx/icon_util_unittest.cc', 'common/gfx/text_elider_unittest.cc', + 'common/important_file_writer_unittest.cc', 'common/ipc_message_unittest.cc', 'common/ipc_sync_channel_unittest.cc', 'common/ipc_sync_message_unittest.cc', diff --git a/chrome/common/common.vcproj b/chrome/common/common.vcproj index b09e53d..22258ed 100644 --- a/chrome/common/common.vcproj +++ b/chrome/common/common.vcproj @@ -522,6 +522,14 @@ > </File> <File + RelativePath=".\important_file_writer.cc" + > + </File> + <File + RelativePath=".\important_file_writer.h" + > + </File> + <File RelativePath=".\json_value_serializer.cc" > </File> diff --git a/chrome/common/important_file_writer.cc b/chrome/common/important_file_writer.cc new file mode 100644 index 0000000..5707744 --- /dev/null +++ b/chrome/common/important_file_writer.cc @@ -0,0 +1,128 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/common/important_file_writer.h" + +#include <stdio.h> + +#include <ostream> +#include <string> + +#include "base/file_path.h" +#include "base/file_util.h" +#include "base/logging.h" +#include "base/string_util.h" +#include "base/task.h" +#include "base/thread.h" +#include "base/time.h" + +using base::TimeDelta; + +namespace { + +const int kDefaultCommitIntervalMs = 10000; + +class WriteToDiskTask : public Task { + public: + WriteToDiskTask(const FilePath& path, const std::string& data) + : path_(path), + data_(data) { + } + + virtual void Run() { + // Write the data to a temp file then rename to avoid data loss if we crash + // while writing the file. Ensure that the temp file is on the same volume + // as target file, so it can be moved in one step, and that the temp file + // is securely created. + FilePath tmp_file_path; + FILE* tmp_file = file_util::CreateAndOpenTemporaryFileInDir( + path_.DirName(), &tmp_file_path); + if (!tmp_file) { + LogFailure("could not create temporary file"); + return; + } + + size_t bytes_written = fwrite(data_.data(), 1, data_.length(), tmp_file); + if (!file_util::CloseFile(tmp_file)) { + file_util::Delete(tmp_file_path, false); + LogFailure("failed to close temporary file"); + return; + } + if (bytes_written < data_.length()) { + file_util::Delete(tmp_file_path, false); + LogFailure("error writing, bytes_written=" + UintToString(bytes_written)); + return; + } + + if (file_util::Move(tmp_file_path, path_)) { + LogSuccess(); + return; + } + + file_util::Delete(tmp_file_path, false); + LogFailure("could not rename temporary file"); + } + + private: + void LogSuccess() { + LOG(INFO) << "successfully saved " << path_.value(); + } + + void LogFailure(const std::string& message) { + LOG(WARNING) << "failed to write " << path_.value() + << ": " << message; + } + + const FilePath path_; + const std::string data_; + + DISALLOW_COPY_AND_ASSIGN(WriteToDiskTask); +}; + +} // namespace + +ImportantFileWriter::ImportantFileWriter(const FilePath& path, + const base::Thread* backend_thread) + : path_(path), + backend_thread_(backend_thread), + commit_interval_(TimeDelta::FromMilliseconds(kDefaultCommitIntervalMs)) { + DCHECK(CalledOnValidThread()); +} + +ImportantFileWriter::~ImportantFileWriter() { + DCHECK(CalledOnValidThread()); + if (timer_.IsRunning()) { + timer_.Stop(); + CommitPendingData(); + } +} + +void ImportantFileWriter::WriteNow(const std::string& data) { + DCHECK(CalledOnValidThread()); + + if (timer_.IsRunning()) + timer_.Stop(); + + Task* task = new WriteToDiskTask(path_, data); + if (backend_thread_) { + backend_thread_->message_loop()->PostTask(FROM_HERE, task); + } else { + task->Run(); + delete task; + } +} + +void ImportantFileWriter::ScheduleWrite(const std::string& data) { + DCHECK(CalledOnValidThread()); + + data_ = data; + if (!timer_.IsRunning()) { + timer_.Start(commit_interval_, this, + &ImportantFileWriter::CommitPendingData); + } +} + +void ImportantFileWriter::CommitPendingData() { + WriteNow(data_); +} diff --git a/chrome/common/important_file_writer.h b/chrome/common/important_file_writer.h new file mode 100644 index 0000000..6a854bb --- /dev/null +++ b/chrome/common/important_file_writer.h @@ -0,0 +1,90 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef CHROME_COMMON_IMPORTANT_FILE_WRITER_H_ +#define CHROME_COMMON_IMPORTANT_FILE_WRITER_H_ + +#include <string> + +#include "base/basictypes.h" +#include "base/file_path.h" +#include "base/non_thread_safe.h" +#include "base/time.h" +#include "base/timer.h" + +namespace base { +class Thread; +} + +// Helper to ensure that a file won't be corrupted by the write (for example on +// application crash). Consider a naive way to save an important file F: +// +// 1. Open F for writing, truncating it. +// 2. Write new data to F. +// +// It's good when it works, but it gets very bad if step 2. doesn't complete. +// It can be caused by a crash, a computer hang, or a weird I/O error. And you +// end up with a broken file. +// +// To be safe, we don't start with writing directly to F. Instead, we write to +// to a temporary file. Only after that write is successful, we rename the +// temporary file to target filename. +// +// If you want to know more about this approach and ext3/ext4 fsync issues, see +// http://valhenson.livejournal.com/37921.html +class ImportantFileWriter : public NonThreadSafe { + public: + // Initialize the writer. + // |path| is the name of file to write. Disk operations will be executed on + // |backend_thread|, or current thread if |backend_thread| is NULL. + // + // All non-const methods, ctor and dtor must be called on the same thread. + ImportantFileWriter(const FilePath& path, const base::Thread* backend_thread); + + // On destruction all pending writes are executed on |backend_thread|. + ~ImportantFileWriter(); + + FilePath path() const { return path_; } + + // Save |data| to target filename. Does not block. If there is a pending write + // scheduled by ScheduleWrite, it is cancelled. + void WriteNow(const std::string& data); + + // Schedule saving |data| to target filename. Data will be saved to disk after + // the commit interval. If an other ScheduleWrite is issued before that, only + // one write to disk will happen - with the more recent data. This operation + // does not block. + void ScheduleWrite(const std::string& data); + + base::TimeDelta commit_interval() const { + return commit_interval_; + } + + void set_commit_interval(const base::TimeDelta& interval) { + commit_interval_ = interval; + } + + private: + // If there is a data scheduled to write, issue a disk operation. + void CommitPendingData(); + + // Path being written to. + const FilePath path_; + + // Thread on which disk operation run. NULL means no separate thread is used. + const base::Thread* backend_thread_; + + // Timer used to schedule commit after ScheduleWrite. + base::OneShotTimer<ImportantFileWriter> timer_; + + // Data scheduled to save. + std::string data_; + + // Time delta after which scheduled data will be written to disk. + base::TimeDelta commit_interval_; + + DISALLOW_COPY_AND_ASSIGN(ImportantFileWriter); +}; + +#endif // CHROME_COMMON_IMPORTANT_FILE_WRITER_H_ diff --git a/chrome/common/important_file_writer_unittest.cc b/chrome/common/important_file_writer_unittest.cc new file mode 100644 index 0000000..09703a5 --- /dev/null +++ b/chrome/common/important_file_writer_unittest.cc @@ -0,0 +1,96 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "chrome/common/important_file_writer.h" + +#include "base/compiler_specific.h" +#include "base/file_path.h" +#include "base/file_util.h" +#include "base/logging.h" +#include "base/message_loop.h" +#include "base/scoped_temp_dir.h" +#include "base/thread.h" +#include "base/time.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace { + +std::string GetFileContent(const FilePath& path) { + std::string content; + if (!file_util::ReadFileToString(path, &content)) { + NOTREACHED(); + } + return content; +} + +} // namespace + +class ImportantFileWriterTest : public testing::Test { + public: + virtual void SetUp() { + ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); + file_ = temp_dir_.path().AppendASCII("test-file"); + } + + protected: + FilePath file_; + + private: + MessageLoop loop_; + ScopedTempDir temp_dir_; +}; + +TEST_F(ImportantFileWriterTest, WithoutBackendThread) { + ImportantFileWriter writer(file_, NULL); + EXPECT_FALSE(file_util::PathExists(writer.path())); + writer.WriteNow("foo"); + ASSERT_TRUE(file_util::PathExists(writer.path())); + EXPECT_EQ("foo", GetFileContent(writer.path())); +} + +TEST_F(ImportantFileWriterTest, WithBackendThread) { + base::Thread thread("file_writer_test"); + ASSERT_TRUE(thread.Start()); + + ImportantFileWriter writer(file_, &thread); + EXPECT_FALSE(file_util::PathExists(writer.path())); + writer.WriteNow("foo"); + thread.Stop(); // Blocks until all tasks are executed. + + ASSERT_TRUE(file_util::PathExists(writer.path())); + EXPECT_EQ("foo", GetFileContent(writer.path())); +} + +TEST_F(ImportantFileWriterTest, ScheduleWrite) { + ImportantFileWriter writer(file_, NULL); + writer.set_commit_interval(base::TimeDelta::FromMilliseconds(25)); + writer.ScheduleWrite("foo"); + MessageLoop::current()->PostDelayedTask(FROM_HERE, + new MessageLoop::QuitTask(), 100); + MessageLoop::current()->Run(); + ASSERT_TRUE(file_util::PathExists(writer.path())); + EXPECT_EQ("foo", GetFileContent(writer.path())); +} + +TEST_F(ImportantFileWriterTest, BatchingWrites) { + ImportantFileWriter writer(file_, NULL); + writer.set_commit_interval(base::TimeDelta::FromMilliseconds(25)); + writer.ScheduleWrite("foo"); + writer.ScheduleWrite("bar"); + writer.ScheduleWrite("baz"); + MessageLoop::current()->PostDelayedTask(FROM_HERE, + new MessageLoop::QuitTask(), 100); + MessageLoop::current()->Run(); + ASSERT_TRUE(file_util::PathExists(writer.path())); + EXPECT_EQ("baz", GetFileContent(writer.path())); +} + +TEST_F(ImportantFileWriterTest, WriteOnDestruction) { + { + ImportantFileWriter writer(file_, NULL); + writer.ScheduleWrite("foo"); + } + ASSERT_TRUE(file_util::PathExists(file_)); + EXPECT_EQ("foo", GetFileContent(file_)); +} diff --git a/chrome/common/pref_member_unittest.cc b/chrome/common/pref_member_unittest.cc index 0e87978..7fe9c6d 100644 --- a/chrome/common/pref_member_unittest.cc +++ b/chrome/common/pref_member_unittest.cc @@ -2,6 +2,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "base/file_path.h" #include "chrome/common/notification_service.h" #include "chrome/common/pref_member.h" #include "chrome/common/pref_service.h" @@ -49,7 +50,7 @@ class PrefMemberTestClass : public NotificationObserver { } // anonymous namespace TEST(PrefMemberTest, BasicGetAndSet) { - PrefService prefs; + PrefService prefs(FilePath(), NULL); RegisterTestPrefs(&prefs); // Test bool @@ -139,7 +140,7 @@ TEST(PrefMemberTest, BasicGetAndSet) { TEST(PrefMemberTest, TwoPrefs) { // Make sure two RealPrefMembers stay in sync. - PrefService prefs; + PrefService prefs(FilePath(), NULL); RegisterTestPrefs(&prefs); RealPrefMember pref1; @@ -159,7 +160,7 @@ TEST(PrefMemberTest, TwoPrefs) { } TEST(PrefMemberTest, Observer) { - PrefService prefs; + PrefService prefs(FilePath(), NULL); RegisterTestPrefs(&prefs); PrefMemberTestClass test_obj(&prefs); diff --git a/chrome/common/pref_service.cc b/chrome/common/pref_service.cc index 12ed13e..74d8805 100644 --- a/chrome/common/pref_service.cc +++ b/chrome/common/pref_service.cc @@ -4,12 +4,9 @@ #include "chrome/common/pref_service.h" -#include "base/compiler_specific.h" -#include "base/file_util.h" #include "base/logging.h" #include "base/message_loop.h" #include "base/string_util.h" -#include "base/task.h" #include "base/thread.h" #include "chrome/common/json_value_serializer.h" #include "chrome/common/l10n_util.h" @@ -19,43 +16,6 @@ namespace { -// The number of milliseconds we'll wait to do a write of chrome prefs to disk. -// This lets us batch together write operations. -static const int kCommitIntervalMs = 10000; - -// Replaces the given file's content with the given data. This allows the -// preferences to be written to disk on a background thread. -class SaveLaterTask : public Task { - public: - SaveLaterTask(const FilePath& file_name, - const std::string& data) - : file_name_(file_name), - data_(data) { - } - - void Run() { - // Write the data to a temp file then rename to avoid data loss if we crash - // while writing the file. - FilePath tmp_file_name(file_name_.value() + FILE_PATH_LITERAL(".tmp")); - int bytes_written = file_util::WriteFile(tmp_file_name, data_.c_str(), - static_cast<int>(data_.length())); - if (bytes_written != -1) { - if (!file_util::Move(tmp_file_name, file_name_)) { - // Rename failed. Try again on the off chance someone has locked either - // file and hope we're successful the second time through. - bool move_result = file_util::Move(tmp_file_name, file_name_); - DCHECK(move_result); - } - } - } - - private: - FilePath file_name_; - std::string data_; - - DISALLOW_COPY_AND_ASSIGN(SaveLaterTask); -}; - // A helper function for RegisterLocalized*Pref that creates a Value* based on // the string value in the locale dll. Because we control the values in a // locale dll, this should always return a Value of the appropriate type. @@ -99,18 +59,12 @@ Value* CreateLocaleDefaultValue(Value::ValueType type, int message_id) { } // namespace -PrefService::PrefService() +PrefService::PrefService(const FilePath& pref_filename, + const base::Thread* backend_thread) : persistent_(new DictionaryValue), transient_(new DictionaryValue), - save_preferences_factory_(NULL) { -} - -PrefService::PrefService(const FilePath& pref_filename) - : persistent_(new DictionaryValue), - transient_(new DictionaryValue), - pref_filename_(pref_filename), - ALLOW_THIS_IN_INITIALIZER_LIST(save_preferences_factory_(this)) { - LoadPersistentPrefs(pref_filename_); + writer_(pref_filename, backend_thread) { + ReloadPersistentPrefs(); } PrefService::~PrefService() { @@ -132,11 +86,10 @@ PrefService::~PrefService() { pref_observers_.clear(); } -bool PrefService::LoadPersistentPrefs(const FilePath& file_path) { - DCHECK(!file_path.empty()); +bool PrefService::ReloadPersistentPrefs() { DCHECK(CalledOnValidThread()); - JSONFileValueSerializer serializer(file_path); + JSONFileValueSerializer serializer(writer_.path()); scoped_ptr<Value> root(serializer.Deserialize(NULL)); if (!root.get()) return false; @@ -146,60 +99,34 @@ bool PrefService::LoadPersistentPrefs(const FilePath& file_path) { return false; persistent_.reset(static_cast<DictionaryValue*>(root.release())); - return true; -} - -void PrefService::ReloadPersistentPrefs() { - DCHECK(CalledOnValidThread()); - - JSONFileValueSerializer serializer(pref_filename_); - scoped_ptr<Value> root(serializer.Deserialize(NULL)); - if (!root.get()) - return; - - // Preferences should always have a dictionary root. - if (!root->IsType(Value::TYPE_DICTIONARY)) - return; - - persistent_.reset(static_cast<DictionaryValue*>(root.release())); for (PreferenceSet::iterator it = prefs_.begin(); it != prefs_.end(); ++it) { (*it)->root_pref_ = persistent_.get(); } + + return true; } -bool PrefService::SavePersistentPrefs(base::Thread* thread) const { - DCHECK(!pref_filename_.empty()); +bool PrefService::SavePersistentPrefs() { DCHECK(CalledOnValidThread()); - // TODO(tc): Do we want to prune webkit preferences that match the default - // value? std::string data; - JSONStringValueSerializer serializer(&data); - serializer.set_pretty_print(true); - if (!serializer.Serialize(*(persistent_.get()))) + if (!SerializePrefData(&data)) return false; - SaveLaterTask* task = new SaveLaterTask(pref_filename_, data); - if (thread != NULL) { - // We can use the background thread, it will take ownership of the task. - thread->message_loop()->PostTask(FROM_HERE, task); - } else { - // In unit test mode, we have no background thread, just execute. - task->Run(); - delete task; - } + writer_.WriteNow(data); return true; } -void PrefService::ScheduleSavePersistentPrefs(base::Thread* thread) { - if (!save_preferences_factory_.empty()) - return; +bool PrefService::ScheduleSavePersistentPrefs() { + DCHECK(CalledOnValidThread()); - MessageLoop::current()->PostDelayedTask(FROM_HERE, - save_preferences_factory_.NewRunnableMethod( - &PrefService::SavePersistentPrefs, thread), - kCommitIntervalMs); + std::string data; + if (!SerializePrefData(&data)) + return false; + + writer_.ScheduleWrite(data); + return true; } void PrefService::RegisterBooleanPref(const wchar_t* path, @@ -717,6 +644,14 @@ void PrefService::FireObservers(const wchar_t* path) { } } +bool PrefService::SerializePrefData(std::string* output) const { + // TODO(tc): Do we want to prune webkit preferences that match the default + // value? + JSONStringValueSerializer serializer(output); + serializer.set_pretty_print(true); + return serializer.Serialize(*(persistent_.get())); +} + /////////////////////////////////////////////////////////////////////////////// // PrefService::Preference diff --git a/chrome/common/pref_service.h b/chrome/common/pref_service.h index 3ab6965..b60f497 100644 --- a/chrome/common/pref_service.h +++ b/chrome/common/pref_service.h @@ -22,9 +22,8 @@ #include "base/non_thread_safe.h" #include "base/observer_list.h" #include "base/scoped_ptr.h" -#include "base/task.h" #include "base/values.h" -#include "testing/gtest/include/gtest/gtest_prod.h" +#include "chrome/common/important_file_writer.h" class NotificationObserver; class Preference; @@ -77,27 +76,27 @@ class PrefService : public NonThreadSafe { }; // |pref_filename| is the path to the prefs file we will try to load or save - // to. - explicit PrefService(const FilePath& pref_filename); + // to. Saves will be executed on |backend_thread|. It should be the file + // thread in Chrome. You can pass NULL for unit tests, and then no separate + // thread will be used. + PrefService(const FilePath& pref_filename, + const base::Thread* backend_thread); ~PrefService(); // Reloads the data from file. This should only be called when the importer // is running during first run, and the main process may not change pref - // values while the importer process is running. - void ReloadPersistentPrefs(); - - // Writes the data to disk on the provided thread. In Chrome, |thread| should - // be the file thread. The return value only reflects whether serialization - // was successful; we don't know whether the data actually made it on disk - // (since it's on a different thread). This should only be used if we need - // to save immediately (basically, during shutdown). Otherwise, you should - // use ScheduleSavePersistentPrefs. - bool SavePersistentPrefs(base::Thread* thread) const; - - // Starts a timer that ends up saving the preferences. This helps to batch - // together save requests that happen in a close time frame so we don't write - // to disk too frequently. - void ScheduleSavePersistentPrefs(base::Thread* thread); + // values while the importer process is running. Returns true on success. + bool ReloadPersistentPrefs(); + + // Writes the data to disk. The return value only reflects whether + // serialization was successful; we don't know whether the data actually made + // it on disk (since it's on a different thread). This should only be used if + // we need to save immediately (basically, during shutdown). Otherwise, you + // should use ScheduleSavePersistentPrefs. + bool SavePersistentPrefs(); + + // Serializes the data and schedules save using ImportantFileWriter. + bool ScheduleSavePersistentPrefs(); DictionaryValue* transient() { return transient_.get(); } @@ -193,24 +192,6 @@ class PrefService : public NonThreadSafe { const Preference* FindPreference(const wchar_t* pref_name) const; private: - FRIEND_TEST(PrefServiceTest, Basic); - FRIEND_TEST(PrefServiceTest, Overlay); - FRIEND_TEST(PrefServiceTest, Observers); - FRIEND_TEST(PrefServiceTest, LocalizedPrefs); - FRIEND_TEST(PrefServiceTest, NoObserverFire); - FRIEND_TEST(PrefServiceTest, HasPrefPath); - - FRIEND_TEST(PrefMemberTest, BasicGetAndSet); - FRIEND_TEST(PrefMemberTest, TwoPrefs); - FRIEND_TEST(PrefMemberTest, Observer); - - // This constructor is used only for some unittests. It doesn't try to load - // any existing prefs from a file. - PrefService(); - - // Reads the data from the given file, returning true on success. - bool LoadPersistentPrefs(const FilePath& file_path); - // Add a preference to the PreferenceMap. If the pref already exists, return // false. This method takes ownership of |pref|. void RegisterPreference(Preference* pref); @@ -227,14 +208,15 @@ class PrefService : public NonThreadSafe { void FireObserversIfChanged(const wchar_t* pref_name, const Value* old_value); + // Serializes stored data to string. |output| is modified only + // if serialization was successful. Returns true on success. + bool SerializePrefData(std::string* output) const; + scoped_ptr<DictionaryValue> persistent_; scoped_ptr<DictionaryValue> transient_; - // The filename that we're loading/saving the prefs to. - FilePath pref_filename_; - - // Task used by ScheduleSavePersistentPrefs to avoid lots of little saves. - ScopedRunnableMethodFactory<PrefService> save_preferences_factory_; + // Helper for safe writing pref data. + ImportantFileWriter writer_; // A set of all the registered Preference objects. PreferenceSet prefs_; diff --git a/chrome/common/pref_service_unittest.cc b/chrome/common/pref_service_unittest.cc index c6c39aa..a7e75e0 100644 --- a/chrome/common/pref_service_unittest.cc +++ b/chrome/common/pref_service_unittest.cc @@ -77,18 +77,22 @@ class TestPrefObserver : public NotificationObserver { std::wstring new_pref_value_; }; -// This test is disabled. See issue 8339. TEST_F(PrefServiceTest, Basic) { - PrefService prefs; + { + // Test that it fails on nonexistent file. + FilePath bogus_input_file = data_dir_.AppendASCII("read.txt"); + PrefService prefs(bogus_input_file, NULL); + EXPECT_FALSE(prefs.ReloadPersistentPrefs()); + } - // Test that it fails on nonexistent file. - FilePath bogus_input_file = data_dir_.AppendASCII("read.txt"); - EXPECT_FALSE(prefs.LoadPersistentPrefs(bogus_input_file)); + ASSERT_TRUE(file_util::CopyFile(data_dir_.AppendASCII("read.json"), + test_dir_.AppendASCII("write.json"))); // Test that the persistent value can be loaded. - FilePath input_file = data_dir_.AppendASCII("read.json"); + FilePath input_file = test_dir_.AppendASCII("write.json"); ASSERT_TRUE(file_util::PathExists(input_file)); - ASSERT_TRUE(prefs.LoadPersistentPrefs(input_file)); + PrefService prefs(input_file, NULL); + ASSERT_TRUE(prefs.ReloadPersistentPrefs()); // Register test prefs. const wchar_t kNewWindowsInTabs[] = L"tabs.new_windows_in_tabs"; @@ -138,11 +142,11 @@ TEST_F(PrefServiceTest, Basic) { // Serialize and compare to expected output. FilePath output_file = test_dir_.AppendASCII("write.json"); - prefs.pref_filename_ = output_file; - ASSERT_TRUE(prefs.SavePersistentPrefs(NULL)); FilePath golden_output_file = data_dir_.AppendASCII("write.golden.json"); ASSERT_TRUE(file_util::PathExists(golden_output_file)); - ASSERT_TRUE(file_util::ContentsEqual(golden_output_file, output_file)); + ASSERT_TRUE(prefs.SavePersistentPrefs()); + EXPECT_TRUE(file_util::ContentsEqual(golden_output_file, output_file)); + ASSERT_TRUE(file_util::Delete(output_file, false)); } TEST_F(PrefServiceTest, Overlay) { @@ -154,10 +158,9 @@ TEST_F(PrefServiceTest, Overlay) { std::wstring persistent_string(L"persistent"); std::wstring transient_string(L"transient"); - PrefService prefs; - FilePath persistent_file = data_dir_.AppendASCII("overlay.json"); - EXPECT_TRUE(prefs.LoadPersistentPrefs(persistent_file)); + PrefService prefs(persistent_file, NULL); + EXPECT_TRUE(prefs.ReloadPersistentPrefs()); Value* transient_value; { @@ -280,11 +283,12 @@ TEST_F(PrefServiceTest, Overlay) { } TEST_F(PrefServiceTest, Observers) { - PrefService prefs; - FilePath input_file = data_dir_.AppendASCII("read.json"); EXPECT_TRUE(file_util::PathExists(input_file)); - EXPECT_TRUE(prefs.LoadPersistentPrefs(input_file)); + + PrefService prefs(input_file, NULL); + + EXPECT_TRUE(prefs.ReloadPersistentPrefs()); const wchar_t pref_name[] = L"homepage"; prefs.RegisterStringPref(pref_name, L""); @@ -325,7 +329,7 @@ TEST_F(PrefServiceTest, Observers) { // TODO(port): port this test to POSIX. #if defined(OS_WIN) TEST_F(PrefServiceTest, LocalizedPrefs) { - PrefService prefs; + PrefService prefs(FilePath(), NULL); const wchar_t kBoolean[] = L"boolean"; const wchar_t kInteger[] = L"integer"; const wchar_t kString[] = L"string"; @@ -348,7 +352,7 @@ TEST_F(PrefServiceTest, LocalizedPrefs) { #endif TEST_F(PrefServiceTest, NoObserverFire) { - PrefService prefs; + PrefService prefs(FilePath(), NULL); const wchar_t pref_name[] = L"homepage"; prefs.RegisterStringPref(pref_name, L""); @@ -383,7 +387,7 @@ TEST_F(PrefServiceTest, NoObserverFire) { } TEST_F(PrefServiceTest, HasPrefPath) { - PrefService prefs; + PrefService prefs(FilePath(), NULL); const wchar_t path[] = L"fake.path"; @@ -396,6 +400,6 @@ TEST_F(PrefServiceTest, HasPrefPath) { EXPECT_FALSE(prefs.HasPrefPath(path)); // Set a value and make sure we have a path. - prefs.persistent_->SetString(path, L"blah"); + prefs.SetString(path, L"blah"); EXPECT_TRUE(prefs.HasPrefPath(path)); } diff --git a/chrome/test/reliability/page_load_test.cc b/chrome/test/reliability/page_load_test.cc index 6134c81..6bb0631 100644 --- a/chrome/test/reliability/page_load_test.cc +++ b/chrome/test/reliability/page_load_test.cc @@ -525,7 +525,7 @@ class PageLoadTest : public UITest { FilePath local_state_path = user_data_dir() .Append(chrome::kLocalStateFilename); - PrefService* local_state(new PrefService(local_state_path)); + PrefService* local_state(new PrefService(local_state_path, NULL)); return local_state; } diff --git a/chrome/test/testing_profile.h b/chrome/test/testing_profile.h index 792402a..9f4d3ca 100644 --- a/chrome/test/testing_profile.h +++ b/chrome/test/testing_profile.h @@ -98,7 +98,7 @@ class TestingProfile : public Profile { prefs_filename = prefs_filename.Append(FILE_PATH_LITERAL("TestPreferences")); if (!prefs_.get()) { - prefs_.reset(new PrefService(prefs_filename)); + prefs_.reset(new PrefService(prefs_filename, NULL)); Profile::RegisterUserPrefs(prefs_.get()); browser::RegisterAllPrefs(prefs_.get(), prefs_.get()); } |