diff options
author | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-04 06:45:43 +0000 |
---|---|---|
committer | shess@chromium.org <shess@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2010-03-04 06:45:43 +0000 |
commit | b0544c44da65ac1a2b0654f97ec473c76176aea2 (patch) | |
tree | baa67ac64a2be7bd3e0bdae932ada206ee9d5991 /chrome/browser/safe_browsing | |
parent | 7af6806afc92098dbe8194195fa0607a9e6c7490 (diff) | |
download | chromium_src-b0544c44da65ac1a2b0654f97ec473c76176aea2.zip chromium_src-b0544c44da65ac1a2b0654f97ec473c76176aea2.tar.gz chromium_src-b0544c44da65ac1a2b0654f97ec473c76176aea2.tar.bz2 |
Revert "On-the-fly migration from SafeBrowsingStoreSqlite -> *File."
Broke the build. Sorry.
BUG=none
TEST=none
Review URL: http://codereview.chromium.org/669043
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@40607 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'chrome/browser/safe_browsing')
4 files changed, 9 insertions, 242 deletions
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file.cc b/chrome/browser/safe_browsing/safe_browsing_store_file.cc index 75c7ad0..7186388 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file.cc +++ b/chrome/browser/safe_browsing/safe_browsing_store_file.cc @@ -7,9 +7,6 @@ #include "base/callback.h" #include "base/md5.h" -// TODO(shess): Remove after migration. -#include "chrome/browser/safe_browsing/safe_browsing_store_sqlite.h" - namespace { // NOTE(shess): kFileMagic should not be a byte-wise palindrome, so @@ -209,17 +206,6 @@ bool SafeBrowsingStoreFile::Delete() { return false; } - // Also make sure any SQLite data is deleted. This should only be - // needed if a journal file is left from a crash and the database is - // reset before SQLite gets a chance to straighten things out. - // TODO(shess): Remove after migration. - SafeBrowsingStoreSqlite old_store; - old_store.Init( - filename_, - NewCallback(this, &SafeBrowsingStoreFile::HandleCorruptDatabase)); - if (!old_store.Delete()) - return false; - return true; } @@ -243,12 +229,11 @@ bool SafeBrowsingStoreFile::Close() { // Make sure the files are closed. file_.reset(); new_file_.reset(); - old_store_.reset(); return true; } bool SafeBrowsingStoreFile::BeginUpdate() { - DCHECK(!file_.get() && !new_file_.get() && !old_store_.get()); + DCHECK(!file_.get() && !new_file_.get()); // Structures should all be clear unless something bad happened. DCHECK(add_chunks_cache_.empty()); @@ -282,30 +267,8 @@ bool SafeBrowsingStoreFile::BeginUpdate() { if (!ReadArray(&header, 1, file.get(), NULL)) return OnCorruptDatabase(); - if (header.magic != kFileMagic || header.version != kFileVersion) { - // Magic numbers didn't match, maybe it's a SQLite database. - scoped_ptr<SafeBrowsingStoreSqlite> - sqlite_store(new SafeBrowsingStoreSqlite()); - sqlite_store->Init( - filename_, - NewCallback(this, &SafeBrowsingStoreFile::HandleCorruptDatabase)); - if (!sqlite_store->BeginUpdate()) - return OnCorruptDatabase(); - - // Pull chunks-seen data into local structures, rather than - // optionally wiring various calls through to the SQLite store. - std::vector<int32> chunks; - sqlite_store->GetAddChunks(&chunks); - add_chunks_cache_.insert(chunks.begin(), chunks.end()); - - sqlite_store->GetSubChunks(&chunks); - sub_chunks_cache_.insert(chunks.begin(), chunks.end()); - - new_file_.swap(new_file); - old_store_.swap(sqlite_store); - - return true; - } + if (header.magic != kFileMagic || header.version != kFileVersion) + return OnCorruptDatabase(); // Check that the file size makes sense given the header. This is a // cheap way to protect against header corruption while deferring @@ -370,7 +333,7 @@ bool SafeBrowsingStoreFile::DoUpdate( const std::vector<SBAddFullHash>& pending_adds, std::vector<SBAddPrefix>* add_prefixes_result, std::vector<SBAddFullHash>* add_full_hashes_result) { - DCHECK(old_store_.get() || file_.get() || empty_); + DCHECK(file_.get() || empty_); DCHECK(new_file_.get()); std::vector<SBAddPrefix> add_prefixes; @@ -378,30 +341,8 @@ bool SafeBrowsingStoreFile::DoUpdate( std::vector<SBAddFullHash> add_full_hashes; std::vector<SBSubFullHash> sub_full_hashes; - // Read |old_store_| into the vectors. - if (old_store_.get()) { - // Push deletions to |old_store_| so they can be applied to the - // data being read. - for (base::hash_set<int32>::const_iterator iter = add_del_cache_.begin(); - iter != add_del_cache_.end(); ++iter) { - old_store_->DeleteAddChunk(*iter); - } - for (base::hash_set<int32>::const_iterator iter = sub_del_cache_.begin(); - iter != sub_del_cache_.end(); ++iter) { - old_store_->DeleteSubChunk(*iter); - } - - if (!old_store_->ReadAddPrefixes(&add_prefixes) || - !old_store_->ReadSubPrefixes(&sub_prefixes) || - !old_store_->ReadAddHashes(&add_full_hashes) || - !old_store_->ReadSubHashes(&sub_full_hashes)) - return OnCorruptDatabase(); - - // Do not actually update the old store. - if (!old_store_->CancelUpdate()) - return OnCorruptDatabase(); - } else if (!empty_) { - // Read |file_| into the vectors. + // Read |file_| into the vectors. + if (!empty_) { DCHECK(file_.get()); if (!FileRewind(file_.get())) @@ -538,16 +479,9 @@ bool SafeBrowsingStoreFile::DoUpdate( // Close the file handle and swizzle the file into place. new_file_.reset(); - if (old_store_.get()) { - const bool deleted = old_store_->Delete(); - old_store_.reset(); - if (!deleted) - return false; - } else { - if (!file_util::Delete(filename_, false) && - file_util::PathExists(filename_)) - return false; - } + if (!file_util::Delete(filename_, false) && + file_util::PathExists(filename_)) + return false; const FilePath new_filename = TemporaryFileForFilename(filename_); if (!file_util::Move(new_filename, filename_)) @@ -574,12 +508,10 @@ bool SafeBrowsingStoreFile::FinishUpdate( DCHECK(!new_file_.get()); DCHECK(!file_.get()); - DCHECK(!old_store_.get()); return Close(); } bool SafeBrowsingStoreFile::CancelUpdate() { - old_store_.reset(); return Close(); } diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file.h b/chrome/browser/safe_browsing/safe_browsing_store_file.h index 3996442..4e0cda9 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file.h +++ b/chrome/browser/safe_browsing/safe_browsing_store_file.h @@ -13,12 +13,6 @@ #include "base/callback.h" #include "base/file_util.h" -// TODO(shess): Data is migrated from SafeBrowsingStoreSqlite as part -// of the next update. That way everyone doesn't pull a new database -// when the code rolls out (and the migration code isn't gnarly). -// After substantially everyone has migrated, the migration code can -// be removed. Make sure that it deletes any journal file. - // Implement SafeBrowsingStore in terms of a flat file. The file // format is pretty literal: // @@ -109,9 +103,6 @@ // often, consider retaining the last known-good file for recovery // purposes, rather than deleting it. -// TODO(shess): Remove after migration. -class SafeBrowsingStoreSqlite; - class SafeBrowsingStoreFile : public SafeBrowsingStore { public: SafeBrowsingStoreFile(); @@ -198,12 +189,6 @@ class SafeBrowsingStoreFile : public SafeBrowsingStore { // a convenience to the caller. bool OnCorruptDatabase(); - // Helper for creating a corruption callback for |old_store_|. - // TODO(shess): Remove after migration. - void HandleCorruptDatabase() { - OnCorruptDatabase(); - } - // Clear temporary buffers used to accumulate chunk data. bool ClearChunkBuffers() { // NOTE: .clear() doesn't release memory. @@ -246,11 +231,6 @@ class SafeBrowsingStoreFile : public SafeBrowsingStore { file_util::ScopedFILE new_file_; bool empty_; - // If the main file existed, but was an SQLite store, this is a - // handle to it. - // TODO(shess): Remove this (and all references) after migration. - scoped_ptr<SafeBrowsingStoreSqlite> old_store_; - // Cache of chunks which have been seen. Loaded from the database // on BeginUpdate() so that it can be queried during the // transaction. diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc index 411ce8a..8e6ee61 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc +++ b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc @@ -3,7 +3,6 @@ // found in the LICENSE file. #include "chrome/browser/safe_browsing/safe_browsing_store_file.h" -#include "chrome/browser/safe_browsing/safe_browsing_store_sqlite.h" #include "base/callback.h" #include "chrome/browser/safe_browsing/safe_browsing_store_unittest_helper.h" @@ -145,144 +144,4 @@ TEST_F(SafeBrowsingStoreFileTest, DetectsCorruption) { EXPECT_TRUE(corruption_detected_); } -// Info to build a trivial store for migration testing. -const int kAddChunk1 = 1; -const int kAddChunk2 = 3; -const int kAddChunk3 = 5; -const int kSubChunk1 = 2; -const int kSubChunk2 = 4; -const SBFullHash kHash1 = SBFullHashFromString("one"); -const SBFullHash kHash2 = SBFullHashFromString("two"); -const SBPrefix kPrefix1 = kHash1.prefix; -const SBPrefix kPrefix2 = kHash2.prefix; -const SBPrefix kPrefix3 = SBFullHashFromString("three").prefix; -const SBPrefix kPrefix4 = SBFullHashFromString("four").prefix; -const SBPrefix kPrefix5 = SBFullHashFromString("five").prefix; -const base::Time kNow = base::Time::Now(); - -// Load the store with some data. -void LoadStore(SafeBrowsingStore* store) { - EXPECT_TRUE(store->BeginUpdate()); - EXPECT_TRUE(store->BeginChunk()); - - store->SetAddChunk(kAddChunk1); - store->SetSubChunk(kSubChunk1); - store->SetAddChunk(kAddChunk2); - - EXPECT_TRUE(store->WriteAddPrefix(kAddChunk1, kPrefix1)); - EXPECT_TRUE(store->WriteAddPrefix(kAddChunk1, kPrefix2)); - EXPECT_TRUE(store->WriteAddPrefix(kAddChunk2, kPrefix3)); - EXPECT_TRUE(store->WriteSubPrefix(kSubChunk1, kAddChunk3, kPrefix4)); - EXPECT_TRUE(store->WriteAddHash(kAddChunk1, kNow, kHash1)); - EXPECT_TRUE(store->WriteSubHash(kSubChunk1, kAddChunk1, kHash2)); - - EXPECT_TRUE(store->FinishChunk()); - - std::vector<SBAddFullHash> pending_adds; - std::vector<SBAddPrefix> add_prefixes; - std::vector<SBAddFullHash> add_hashes; - EXPECT_TRUE(store->FinishUpdate(pending_adds, &add_prefixes, &add_hashes)); - EXPECT_EQ(3U, add_prefixes.size()); -} - -// Verify that the store looks like what results from LoadStore(), and -// update it. -void UpdateStore(SafeBrowsingStore* store) { - EXPECT_TRUE(store->BeginUpdate()); - EXPECT_TRUE(store->BeginChunk()); - - // The chunks in the database should be the same. - std::vector<int> add_chunks; - store->GetAddChunks(&add_chunks); - ASSERT_EQ(2U, add_chunks.size()); - EXPECT_EQ(kAddChunk1, add_chunks[0]); - EXPECT_EQ(kAddChunk2, add_chunks[1]); - - std::vector<int> sub_chunks; - store->GetSubChunks(&sub_chunks); - ASSERT_EQ(1U, sub_chunks.size()); - EXPECT_EQ(kSubChunk1, sub_chunks[0]); - - EXPECT_TRUE(store->CheckAddChunk(kAddChunk1)); - EXPECT_TRUE(store->CheckSubChunk(kSubChunk1)); - EXPECT_TRUE(store->CheckAddChunk(kAddChunk2)); - - EXPECT_FALSE(store->CheckAddChunk(kAddChunk3)); - store->SetAddChunk(kAddChunk3); - // This one already has a sub. - EXPECT_TRUE(store->WriteAddPrefix(kAddChunk3, kPrefix4)); - EXPECT_TRUE(store->WriteAddPrefix(kAddChunk3, kPrefix5)); - - EXPECT_FALSE(store->CheckSubChunk(kSubChunk2)); - store->SetSubChunk(kSubChunk2); - EXPECT_TRUE(store->WriteSubPrefix(kSubChunk2, kAddChunk1, kPrefix1)); - - EXPECT_TRUE(store->FinishChunk()); - - store->DeleteAddChunk(kAddChunk2); - - std::vector<SBAddFullHash> pending_adds; - std::vector<SBAddPrefix> add_prefixes; - std::vector<SBAddFullHash> add_hashes; - EXPECT_TRUE(store->FinishUpdate(pending_adds, &add_prefixes, &add_hashes)); - EXPECT_EQ(2U, add_prefixes.size()); -} - -// Verify that the expected UpdateStore() data is present. -void CheckStore(SafeBrowsingStore* store) { - EXPECT_TRUE(store->BeginUpdate()); - - // The chunks in the database should be the same. - std::vector<int> add_chunks; - store->GetAddChunks(&add_chunks); - ASSERT_EQ(2U, add_chunks.size()); - EXPECT_EQ(kAddChunk1, add_chunks[0]); - EXPECT_EQ(kAddChunk3, add_chunks[1]); - - std::vector<int> sub_chunks; - store->GetSubChunks(&sub_chunks); - ASSERT_EQ(2U, sub_chunks.size()); - EXPECT_EQ(kSubChunk1, sub_chunks[0]); - EXPECT_EQ(kSubChunk2, sub_chunks[1]); - - EXPECT_TRUE(store->CancelUpdate()); -} - -// Verify that the migration sequence works as expected in the -// non-migration cases. -TEST_F(SafeBrowsingStoreFileTest, MigrateBaselineFile) { - LoadStore(store_.get()); - UpdateStore(store_.get()); - CheckStore(store_.get()); -} -TEST_F(SafeBrowsingStoreFileTest, MigrateBaselineSqlite) { - SafeBrowsingStoreSqlite sqlite_store; - sqlite_store.Init(filename_, NULL); - - LoadStore(&sqlite_store); - UpdateStore(&sqlite_store); - CheckStore(&sqlite_store); -} - -// The sequence should work exactly the same when we migrate from -// SQLite to file. -TEST_F(SafeBrowsingStoreFileTest, Migrate) { - // No existing store. - EXPECT_FALSE(file_util::PathExists(filename_)); - - { - SafeBrowsingStoreSqlite sqlite_store; - sqlite_store.Init(filename_, NULL); - - LoadStore(&sqlite_store); - } - - // At this point |filename_| references a SQLite store. - EXPECT_TRUE(file_util::PathExists(filename_)); - - // Update and check using a file store. - UpdateStore(store_.get()); - CheckStore(store_.get()); -} - } // namespace diff --git a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h index e72073b..5e03d50 100644 --- a/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h +++ b/chrome/browser/safe_browsing/safe_browsing_store_sqlite.h @@ -102,10 +102,6 @@ class SafeBrowsingStoreSqlite : public SafeBrowsingStore { } private: - // For on-the-fly migration. - // TODO(shess): Remove (entire class) after migration. - friend class SafeBrowsingStoreFile; - // The following routines return true on success, or false on // failure. Failure is presumed to be persistent, so the caller // should stop trying and unwind the transaction. |