Delete empty dom storage databases on destruction.

webkit/dom_storage/dom_storage_database.cc

 // Copyright (c) 2012 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 "webkit/dom_storage/dom_storage_database.h"
 
 #include "base/file_util.h"
 #include "base/logging.h"
 #include "sql/diagnostic_error_delegate.h"
 #include "sql/statement.h"
@@ skipping 12 matching lines @@
 }
 
 }  // anon namespace
 
 namespace dom_storage {
 
 DomStorageDatabase::DomStorageDatabase(const FilePath& file_path)
     : file_path_(file_path),
       db_(NULL),
       failed_to_open_(false),
-      tried_to_recreate_(false) {
+      tried_to_recreate_(false),
+      known_to_be_empty_(false) {
   // Note: in normal use we should never get an empty backing path here.
   // However, the unit test for this class defines another constructor
   // that will bypass this check to allow an empty path that signifies
   // we should operate on an in-memory database for performance/reliability
   // reasons.
   DCHECK(!file_path_.empty());
 }
 
 DomStorageDatabase::~DomStorageDatabase() {
+  if (known_to_be_empty_ && !file_path_.empty()) {
+    // Delete the db from disk, it's empty.
+    Close();
+    file_util::Delete(file_path_, false);
+  }
 }
 
 void DomStorageDatabase::ReadAllValues(ValuesMap* result) {
   if (!LazyOpen(false))
     return;
 
   sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
                                                    "SELECT * from ItemTable"));
   DCHECK(statement.is_valid());
 
   while (statement.Step()) {
     string16 key = statement.ColumnString16(0);
     string16 value;
     statement.ColumnBlobAsString16(1, &value);
     (*result)[key] = NullableString16(value, false);
   }

[michaeln, 2012/02/27 19:28:48]

i think we should set known_to_be_empty_ here

since chrome can crash, the reasons to assume that an existing database is not
empty are not valid... also there's the matter of the pre-existing db files what
state are they in?

[benm (inactive), 2012/02/27 19:36:15]

Got it.

 }
 
 bool DomStorageDatabase::CommitChanges(bool clear_all_first,
                                        const ValuesMap& changes) {
   if (!LazyOpen(!changes.empty()))
     return false;
 
+  bool old_known_to_be_empty = known_to_be_empty_;
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return false;
 
   if (clear_all_first) {
     if (!db_->Execute("DELETE FROM ItemTable"))
       return false;
+    known_to_be_empty_ = true;
   }
 
+  bool did_delete = false;
+  bool did_insert = false;
   ValuesMap::const_iterator it = changes.begin();
   for(; it != changes.end(); ++it) {
     sql::Statement statement;
     string16 key = it->first;
     NullableString16 value = it->second;
     if (value.is_null()) {
       statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
          "DELETE FROM ItemTable WHERE key=?"));
       statement.BindString16(0, key);
+      did_delete = true;
     } else {
       statement.Assign(db_->GetCachedStatement(SQL_FROM_HERE,
           "INSERT INTO ItemTable VALUES (?,?)"));
       statement.BindString16(0, key);
       statement.BindBlob(1, value.string().data(),
                          value.string().length() * sizeof(char16));
+      known_to_be_empty_ = false;
+      did_insert = true;
     }
     DCHECK(statement.is_valid());
     statement.Run();
   }
-  return transaction.Commit();
+
+  if (!known_to_be_empty_ && did_delete && !did_insert) {
+    // See if the database is empty.

[michaeln, 2012/02/27 19:28:48]

consider removing the comment, the code is clear enough

[benm (inactive), 2012/02/27 19:36:15]

Done.

+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
+        "SELECT count(key) from ItemTable"));
+    if (statement.Step())
+      known_to_be_empty_ = statement.ColumnInt(0) == 0;

[michaeln, 2012/02/27 19:28:48]

if you think it's more readable, consider...

known_to_be_empty_ = Step() &&
                    (ColumnInt(0) == 0);

[benm (inactive), 2012/02/27 19:36:15]

I think I prefer the if () then approach like it is now.

+  }
+
+  bool success = transaction.Commit();
+  if (!success)
+    known_to_be_empty_ = old_known_to_be_empty;
+  return success;
 }
 
 bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
   if (failed_to_open_) {
     // Don't try to open a database that we know has failed
     // already.
     return false;
   }
 
   if (IsOpen())
@@ skipping 37 matching lines @@
 
   if (!database_exists) {
     // This is a new database, create the table and we're done!
     if (CreateTableV2())
       return true;
   } else {
     // The database exists already - check if we need to upgrade
     // and whether it's usable (i.e. not corrupted).
     SchemaVersion current_version = DetectSchemaVersion();
 
+    bool valid_database = false;
     if (current_version == V2) {
-      return true;
+      valid_database = true;
     } else if (current_version == V1) {
       if (UpgradeVersion1To2())
-        return true;
+        valid_database = true;
+    }
+
+    if (!valid_database) {
+      // Try and recover by attempting to delete the file and start again.
+      Close();
+      return DeleteFileAndRecreate();
     }
   }
 
-  // This is the exceptional case - to try and recover we'll attempt
-  // to delete the file and start again.
-  Close();
-  return DeleteFileAndRecreate();
+  return true;

[michaeln, 2012/02/27 19:28:48]

looks like this introduces a bug in the 'new database' case (up above), if table
creation fails this method returns true.

in the spirit of extreme programming principles, i don't see what the changes in
this method have to do with deleting empties on exit and suggest undoing them

(having said that... imo, an improvement would be to make the 'new database'
case return early always instead of only sometimes, because that would make the
type of error being introduced here less likely to be introduced, i think i
tried to make that point in an earlier code review:)

[benm (inactive), 2012/02/27 19:36:15]

Good spot.
Probably for the best. :)
Will do it in another CL :)

 }
 
 DomStorageDatabase::SchemaVersion DomStorageDatabase::DetectSchemaVersion() {
   DCHECK(IsOpen());
 
   // Connection::Open() may succeed even if the file we try and open is not a
   // database, however in the case that the database is corrupted to the point
   // that SQLite doesn't actually think it's a database,
   // sql::Connection::GetCachedStatement will DCHECK when we later try and
   // run statements. So we run a query here that will not DCHECK but fail
@@ skipping 77 matching lines @@
       CreateTableV2() &&
       CommitChanges(false, values) &&
       migration.Commit();
 }
 
 void DomStorageDatabase::Close() {
   db_.reset(NULL);
 }
 
 }  // namespace dom_storage