Move code to clear web databases on shutdown to the database tracker

webkit/database/database_tracker.cc

Index: webkit/database/database_tracker.cc
diff --git a/webkit/database/database_tracker.cc b/webkit/database/database_tracker.cc
index 7497c3814c7262279eeac8a242b42b1c921505d1..61753e44f1147088982ef4a44de92186344903c6 100644
--- a/webkit/database/database_tracker.cc
+++ b/webkit/database/database_tracker.cc
@@ -15,6 +15,7 @@
 #include "base/basictypes.h"
 #include "base/file_util.h"
 #include "base/message_loop_proxy.h"
+#include "base/platform_file.h"
 #include "base/string_number_conversions.h"
 #include "base/utf_string_conversions.h"
 #include "net/base/net_errors.h"
@@ -35,6 +36,37 @@ sql::ErrorDelegate* GetErrorHandlerForTrackerDb() {
   return new sql::DiagnosticErrorDelegate<HistogramUniquifier>();
 }
 
+void ClearLocalState(webkit_database::DatabaseTracker* tracker) {

[michaeln, 2011/07/01 03:02:34]

What guarantees are there that the tracker has not been deleted here? Did you
mean to make the param be a scoped_refptr<>? Also, see comments below about
making this a class method instead of a separate function.

[jochen (gone - plz use gerrit), 2011/07/06 09:24:53]

Done.

+  std::vector<string16> origin_identifiers;
+  tracker->GetAllOriginIdentifiers(&origin_identifiers);
+
+  for (std::vector<string16>::iterator origin = origin_identifiers.begin();
+       origin != origin_identifiers.end(); ++origin) {
+    if (tracker->special_storage_policy() &&
+        tracker->special_storage_policy()->IsStorageProtected(
+            webkit_database::DatabaseUtil::GetOriginFromIdentifier(*origin))) {
+      continue;
+    }
+    webkit_database::OriginInfo origin_info;
+    std::vector<string16> databases;
+    tracker->GetOriginInfo(*origin, &origin_info);
+    origin_info.GetAllDatabaseNames(&databases);
+
+    for (std::vector<string16>::iterator database = databases.begin();
+         database != databases.end(); ++database) {
+      base::PlatformFile file_handle = base::CreatePlatformFile(
+          tracker->GetFullDBFilePath(*origin, *database),
+          base::PLATFORM_FILE_OPEN_ALWAYS |
+          base::PLATFORM_FILE_SHARE_DELETE |
+          base::PLATFORM_FILE_DELETE_ON_CLOSE |
+          base::PLATFORM_FILE_READ,
+          NULL, NULL);
+      base::ClosePlatformFile(file_handle);
+    }
+    tracker->DeleteOrigin(*origin, true);
+  }
+}
+
 }  // anon namespace
 
 namespace webkit_database {
@@ -47,7 +79,11 @@ const FilePath::CharType kTrackerDatabaseFileName[] =
     FILE_PATH_LITERAL("Databases.db");
 static const int kCurrentVersion = 2;
 static const int kCompatibleVersion = 1;
-static const char* kExtensionOriginIdentifierPrefix = "chrome-extension_";
+
+const FilePath::CharType kTemporaryDirectoryPrefix[] =
+    FILE_PATH_LITERAL("DeleteMe");
+const FilePath::CharType kTemporaryDirectoryPattern[] =
+    FILE_PATH_LITERAL("DeleteMe*");
 
 OriginInfo::OriginInfo()
     : total_size_(0) {}
@@ -87,11 +123,13 @@ OriginInfo::OriginInfo(const string16& origin, int64 total_size)
 DatabaseTracker::DatabaseTracker(
     const FilePath& profile_path,
     bool is_incognito,
+    bool clear_local_state_on_exit,
     quota::SpecialStoragePolicy* special_storage_policy,
     quota::QuotaManagerProxy* quota_manager_proxy,
     base::MessageLoopProxy* db_tracker_thread)
     : is_initialized_(false),
       is_incognito_(is_incognito),
+      clear_local_state_on_exit_(clear_local_state_on_exit),
       shutting_down_(false),
       profile_path_(profile_path),
       db_dir_(is_incognito_ ?
@@ -102,6 +140,7 @@ DatabaseTracker::DatabaseTracker(
       meta_table_(NULL),
       special_storage_policy_(special_storage_policy),
       quota_manager_proxy_(quota_manager_proxy),
+      db_tracker_thread_(db_tracker_thread),
       incognito_origin_directories_generator_(0) {
   if (quota_manager_proxy) {
     quota_manager_proxy->RegisterClient(
@@ -119,7 +158,7 @@ void DatabaseTracker::DatabaseOpened(const string16& origin_identifier,
                                      const string16& database_description,
                                      int64 estimated_size,
                                      int64* database_size) {
-  if (!LazyInit()) {
+  if (shutting_down_ || !LazyInit()) {
     *database_size = 0;
     return;
   }
@@ -363,17 +402,18 @@ bool DatabaseTracker::DeleteClosedDatabase(const string16& origin_identifier,
   if (databases_table_->GetAllDatabaseDetailsForOrigin(
           origin_identifier, &details) && details.empty()) {
     // Try to delete the origin in case this was the last database.
-    DeleteOrigin(origin_identifier);
+    DeleteOrigin(origin_identifier, false);
   }
   return true;
 }
 
-bool DatabaseTracker::DeleteOrigin(const string16& origin_identifier) {
+bool DatabaseTracker::DeleteOrigin(const string16& origin_identifier,
+                                   bool force) {
   if (!LazyInit())
     return false;
 
   // Check if any database in this origin is opened by any renderer.
-  if (database_connections_.IsOriginUsed(origin_identifier))
+  if (database_connections_.IsOriginUsed(origin_identifier) && !force)
     return false;
 
   int64 deleted_size = 0;
@@ -389,8 +429,18 @@ bool DatabaseTracker::DeleteOrigin(const string16& origin_identifier) {
   origins_info_map_.erase(origin_identifier);
   FilePath origin_dir = db_dir_.Append(FilePath::FromWStringHack(
       UTF16ToWide(origin_identifier)));
-  if (!file_util::Delete(origin_dir, true))
-    return false;
+  if (!file_util::Delete(origin_dir, true)) {
+    if (!force)
+      return false;
+
+    // Rename the directory as its name leaks the origin's name.
+    FilePath new_origin_dir;
+    file_util::CreateTemporaryDirInDir(db_dir_,
+                                       kTemporaryDirectoryPrefix,
+                                       &new_origin_dir);
+    FilePath temp_dir = db_dir_.Append(FILE_PATH_LITERAL("0"));
+    file_util::Move(origin_dir, temp_dir);
+  }
 
   databases_table_->DeleteOrigin(origin_identifier);
 
@@ -422,6 +472,20 @@ bool DatabaseTracker::LazyInit() {
     DCHECK(!databases_table_.get());
     DCHECK(!meta_table_.get());
 
+    // If there are left-over directoryfrom failed deletion attempts, clean

[michaeln, 2011/07/01 03:02:34]

s/b directories from

[jochen (gone - plz use gerrit), 2011/07/06 09:24:53]

Done.

+    // them up.
+    if (file_util::DirectoryExists(db_dir_)) {
+      file_util::FileEnumerator directories(
+          db_dir_,
+          false,
+          file_util::FileEnumerator::DIRECTORIES,
+          kTemporaryDirectoryPattern);
+      for (FilePath directory = directories.Next(); !directory.empty();
+           directory = directories.Next()) {
+        file_util::Delete(directory, true);
+      }
+    }
+
     // If the tracker database exists, but it's corrupt or doesn't
     // have a meta table, delete the database directory.
     const FilePath kTrackerDatabaseFullPath =
@@ -747,44 +811,30 @@ void DatabaseTracker::DeleteIncognitoDBDirectory() {
     file_util::Delete(incognito_db_dir, true);
 }
 
-// static
-void DatabaseTracker::ClearLocalState(const FilePath& profile_path) {
-  // TODO(michaeln): use SpecialStoragePolicy instead of kExtensionOriginPrefix
-  FilePath db_dir = profile_path.Append(FilePath(kDatabaseDirectoryName));
-  FilePath db_tracker = db_dir.Append(FilePath(kTrackerDatabaseFileName));
-  if (file_util::DirectoryExists(db_dir) &&
-      file_util::PathExists(db_tracker)) {
-    scoped_ptr<sql::Connection> db_(new sql::Connection);
-    if (!db_->Open(db_tracker) ||
-        !db_->DoesTableExist("Databases")) {
-      db_->Close();
-      file_util::Delete(db_dir, true);
+void DatabaseTracker::Shutdown() {
+  if (shutting_down_ || is_incognito_) {
+    NOTREACHED();
+    return;
+  }
+  if (clear_local_state_on_exit_ && LazyInit()) {

[michaeln, 2011/07/01 03:02:34]

We shouldn't call LazyInit() on the UI thread. It'd be safer to invoke this
method on the FILE thread.

The OffTheRecordProfileImpl does something very similar to what this CL is
doing, but it calls DeleteIncognitoDBDirectory() (being careful to schedule the
call on the FILE thread) in it's dtor instead of calling Shutdown().

Maybe we can use the Shutdown() method for both purposes?

void Shutdown(bool clear_local_state) {
  DCHECK(db_tracker_thread_->BelongsToCurrentThread());
  shutting_down_ = true;
  is_initialized_ = false;
  if (is_incognito_)
    DeleteIncognitoDBDirectory();
  else if (clear_local_state)
    ClearLocalState();
}

wdyt?

[jochen (gone - plz use gerrit), 2011/07/06 09:24:53]

Done.

+    shutting_down_ = true;
+
+    if (!db_tracker_thread_.get()) {
+      NOTREACHED();
       return;
-    } else {
-      sql::Statement delete_statement(db_->GetCachedStatement(
-            SQL_FROM_HERE, "DELETE FROM Databases WHERE origin NOT LIKE ?"));
-      std::string filter(kExtensionOriginIdentifierPrefix);
-      filter += "%";
-      delete_statement.BindString(0, filter);
-      if (!delete_statement.Run()) {
-        db_->Close();
-        file_util::Delete(db_dir, true);
-        return;
-      }
     }
+    scoped_refptr<DatabaseTracker> tracker(this);
+    db_tracker_thread_->PostTask(FROM_HERE,
+                                 NewRunnableFunction(ClearLocalState, tracker));
   }
-  file_util::FileEnumerator file_enumerator(db_dir, false,
-      file_util::FileEnumerator::DIRECTORIES);
-  for (FilePath file_path = file_enumerator.Next(); !file_path.empty();
-       file_path = file_enumerator.Next()) {
-    if (file_path.BaseName() != FilePath(kTrackerDatabaseFileName)) {
-      std::string basename = file_path.BaseName().MaybeAsASCII();
-      if (!basename.empty() &&
-          !StartsWithASCII(basename, kExtensionOriginIdentifierPrefix, true)) {
-        file_util::Delete(file_path, true);
-      }
-    }
+}
+
+void DatabaseTracker::SetClearLocalStateOnExit(bool clear_local_state_on_exit) {

[michaeln, 2011/07/01 03:02:34]

It'd be safer to test and set these data members exclusively on the FILE thread.

 if (!db_tracker_thread_->BelongsToCurrentThread()) {
    db_tracker_thread_->PostTask(FROM_HERE,
        NewRunnableMethod(this,
                          &DatabaseTracker::SetClearLocalStateOnExit,
                          clear_local_state_on_exit));
    return;
  }
  clear_local_state_on_exit_ = clear_local_state_on_exit;

[jochen (gone - plz use gerrit), 2011/07/06 09:24:53]

Done.

+  if (shutting_down_) {
+    NOTREACHED();
+    return;
   }
+  clear_local_state_on_exit_ = clear_local_state_on_exit;
 }
 
 }  // namespace webkit_database