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),
+      values_stored_(0) {
   // 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 (IsOpen() && values_stored_ == 0) {
+    // Delete the db from disk, it's empty.
+    Close();
+    file_util::Delete(file_path_, false);

[michaeln, 2012/02/25 02:55:55]

maybe skip this if file_path.empty()?

if (known_to_be_empty_ && !file_path.empty()) {
  Close();
  file_util::Delete(...);
}

[benm (inactive), 2012/02/27 12:06:25]

Done.

+  }
 }
 
 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/25 02:55:55]

known_to_be_empty_ = result->empty();

[benm (inactive), 2012/02/27 12:06:25]

Do we need to do this if we init known_to_be_empty_(false) in the ctor?

When the database is first created, it's empty and we won't write anything on
disk until we get the first CommitChanges. If ReadAllValues gets called on a db
in this state then we'll have already bailed out as LazyOpen(false) would've
returned false. The flag may be false when it could be true in this case, but
the side effect of that is that we won't try and delete the non-existent file in
the dtor.

If the file already exists, with this patch we'll never leave an empty database
on disk so it must have content. So as ReadAllValues does not change the
database, we can assume the database non-empty here if CommitChanges hasn't been
executed. And if CommitChanges has been executed, then we will have updated
known_to_be_empty_ at that point.

 }
 
 bool DomStorageDatabase::CommitChanges(bool clear_all_first,
                                        const ValuesMap& changes) {
   if (!LazyOpen(!changes.empty()))
     return false;
 
   sql::Transaction transaction(db_.get());
   if (!transaction.Begin())
     return false;
 
   if (clear_all_first) {
     if (!db_->Execute("DELETE FROM ItemTable"))
       return false;
+    values_stored_ = 0;
   }
 
   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);
+      values_stored_--;
     } 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));
+      values_stored_++;

[michaeln, 2012/02/25 02:55:55]

This count will not be accurate, consider this is just an update.

Keeping count is overkill. All we need is 'known_to_be_empty_' at dtor time, not
a count. I think we can do this in a lighter weight way given a bool data member
to that effect.

- init to false in the ctor
- dont modify LazyOpen in any way (value remains unmodified upon return)
  no extra queries on open
  (its ok to not determine on open because open only happens via the public
   ReadAllValues and CommitChanges methods which will determine the truth
   themselves)
- set to true in the if clear_all_first clause
- set to false here in the INSERT clause, and set a local to 'did_insert'
- set a local to 'did_delete' in the DELETE clause

after the for loop where we INSERT &| DELETE items, and before the COMMIT

if (!known_to_be_empty_ && did_delete && !did_insert) {
  knowns_to_be_empty_ = (select count == 0);
}

COMMIT

wdyt?

[benm (inactive), 2012/02/27 12:06:25]

Good catch, i made the test a bit more robust.
sg. I kept the small refactoring in LazyOpen, but removed the query.
Do we need the did_insert local? We can just set known_to_be_empty_ false on
insert (as if it's an add or replace we still won't have an empty db). I guess
it would optimise the case that we have a bunch of inserts/deletes in
changed_values_, so that we don't have to run the SELECT COUNT (as there was an
insert which we know will leave the db non-empty)

     }
     DCHECK(statement.is_valid());
     statement.Run();
   }
   return transaction.Commit();
 }
 
 bool DomStorageDatabase::LazyOpen(bool create_if_needed) {
   if (failed_to_open_) {
     // Don't try to open a database that we know has failed
@@ skipping 42 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) {
+      // This is the exceptional case - to try and recover we'll attempt
+      // to delete the file and start again.
+      Close();
+      return DeleteFileAndRecreate();
+    }
+
+    sql::Statement statement(db_->GetCachedStatement(SQL_FROM_HERE,
+                             "SELECT count(key) from ItemTable"));
+    if (statement.Step())
+      values_stored_ = statement.ColumnInt(0);
   }
 
-  // This is the exceptional case - to try and recover we'll attempt
-  // to delete the file and start again.
-  Close();
-  return DeleteFileAndRecreate();
+  return true;
 }
 
 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