Don't update safe-browsing database if no updates come down.

For simplicity, the safe-browsing database code ran the full update
cycle on every update.  A significant number of updates make no
changes to the database, in which case re-writing it is cost with no
benefit.

BUG=72216
TEST=unit test.

Review URL: http://codereview.chromium.org/6250211

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@74248 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 69 insertions, 2 deletions

diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index e717655..16e4c55 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -566,6 +566,8 @@ void SafeBrowsingDatabaseNew::InsertChunks(const std::string& list_name,
   SafeBrowsingStore* store = GetStore(list_id);
   if (!store) return;
 
+  change_detected_ = true;
+
   store->BeginChunk();
   if (chunks.front().is_add) {
     InsertAddChunks(list_id, chunks);
@@ -590,6 +592,8 @@ void SafeBrowsingDatabaseNew::DeleteChunks(
   SafeBrowsingStore* store = GetStore(list_id);
   if (!store) return;
 
+  change_detected_ = true;
+
   for (size_t i = 0; i < chunk_deletes.size(); ++i) {
     std::vector<int> chunk_numbers;
     RangesToChunks(chunk_deletes[i].chunk_del, &chunk_numbers);
@@ -677,6 +681,7 @@ bool SafeBrowsingDatabaseNew::UpdateStarted(
   }
 
   corruption_detected_ = false;
+  change_detected_ = false;
   return true;
 }
 
@@ -685,8 +690,10 @@ void SafeBrowsingDatabaseNew::UpdateFinished(bool update_succeeded) {
   if (corruption_detected_)
     return;
 
-  // Unroll any partially-received transaction.
-  if (!update_succeeded) {
+  // Unroll the transaction if there was a protocol error or if the
+  // transaction was empty.  This will leave the bloom filter, the
+  // pending hashes, and the prefix miss cache in place.
+  if (!update_succeeded || !change_detected_) {
     browse_store_->CancelUpdate();
     if (download_store_.get())
       download_store_->CancelUpdate();
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.h b/chrome/browser/safe_browsing/safe_browsing_database.h
index 4bd6db9..4b6890d 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.h
+++ b/chrome/browser/safe_browsing/safe_browsing_database.h
@@ -272,6 +272,10 @@ class SafeBrowsingDatabaseNew : public SafeBrowsingDatabase {
   // Causes the update functions to fail with no side effects, until
   // the next call to |UpdateStarted()|.
   bool corruption_detected_;
+
+  // Set to true if any chunks are added or deleted during an update.
+  // Used to optimize away database update.
+  bool change_detected_;
 };
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_DATABASE_H_
diff --git a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
index 2b754ef..8b0cb76 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database_unittest.cc
@@ -1421,3 +1421,59 @@ TEST_F(SafeBrowsingDatabaseTest, BinHashEntries) {
   // database is available in database.
   database_.reset();
 }
+
+// Test that an empty update doesn't actually update the database.
+// This isn't a functionality requirement, but it is a useful
+// optimization.
+TEST_F(SafeBrowsingDatabaseTest, EmptyUpdate) {
+  SBChunkList chunks;
+  SBChunk chunk;
+
+  FilePath filename = database_->BrowseDBFilename(database_filename_);
+
+  // Prime the database.
+  std::vector<SBListChunkRanges> lists;
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+
+  InsertAddChunkHostPrefixUrl(&chunk, 1, "www.evil.com/",
+                              "www.evil.com/malware.html");
+  chunks.clear();
+  chunks.push_back(chunk);
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+
+  // Inserting another chunk updates the database file.  The sleep is
+  // needed because otherwise the entire test can finish w/in the
+  // resolution of the lastmod time.
+  base::PlatformFileInfo before_info, after_info;
+  base::PlatformThread::Sleep(1500);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &before_info));
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  chunk.hosts.clear();
+  InsertAddChunkHostPrefixUrl(&chunk, 2, "www.foo.com/",
+                              "www.foo.com/malware.html");
+  chunks.clear();
+  chunks.push_back(chunk);
+  database_->InsertChunks(safe_browsing_util::kMalwareList, chunks);
+  database_->UpdateFinished(true);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &after_info));
+  EXPECT_LT(before_info.last_modified, after_info.last_modified);
+
+  // Deleting a chunk updates the database file.
+  base::PlatformThread::Sleep(1500);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &before_info));
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  AddDelChunk(safe_browsing_util::kMalwareList, chunk.chunk_number);
+  database_->UpdateFinished(true);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &after_info));
+  EXPECT_LT(before_info.last_modified, after_info.last_modified);
+
+  // Simply calling |UpdateStarted()| then |UpdateFinished()| does not
+  // update the database file.
+  base::PlatformThread::Sleep(1500);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &before_info));
+  EXPECT_TRUE(database_->UpdateStarted(&lists));
+  database_->UpdateFinished(true);
+  ASSERT_TRUE(file_util::GetFileInfo(filename, &after_info));
+  EXPECT_EQ(before_info.last_modified, after_info.last_modified);
+}