[Merge] ServiceWorker: Directly delete database directory instead of leveldb::DestroyDB

When a database directory contains files not related to LevelDB, DestroyDB()
does not delete the directory and leaves those files. This non-empty database
directory infinitely fails ServiceWorkerDatabase::LazyOpen during a session.
(See my comment #24 in the issue for more details).

To avoid that, this CL directly deletes the directory instead of DestroyDB.

BUG=468926
TEST=content_unittests --gtest_filter=ServiceWorkerResourceStorageDiskTest.DeleteAndStartOver*
TBR=falken@chromium.org

Review URL: https://codereview.chromium.org/1176203007

Cr-Commit-Position: refs/heads/master@{#335893}
(cherry picked from commit 0bd5fd70714ca1ae846be5b657fd3a21f49884c1)

Review URL: https://codereview.chromium.org/1214793006.

Cr-Commit-Position: refs/branch-heads/2403@{#413}
Cr-Branched-From: f54b8097a9c45ed4ad308133d49f05325d6c5070-refs/heads/master@{#330231}

6 files changed, 206 insertions, 101 deletions

diff --git a/content/browser/service_worker/service_worker_context_unittest.cc b/content/browser/service_worker/service_worker_context_unittest.cc
index 16a3233..ae52dee 100644
--- a/content/browser/service_worker/service_worker_context_unittest.cc
+++ b/content/browser/service_worker/service_worker_context_unittest.cc
@@ -520,16 +520,105 @@ TEST_F(ServiceWorkerContextTest, RegisterDuplicateScript) {
   EXPECT_EQ(old_registration_id, notifications_[1].registration_id);
 }
 
-TEST_F(ServiceWorkerContextTest, DeleteAndStartOver) {
+TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
+  const int kRenderProcessId1 = 1;
+  const int kRenderProcessId2 = 2;
+  const GURL kOrigin1 = GURL("http://www.example.com/");
+  const GURL kOrigin2 = GURL("https://www.example.com/");
+  int provider_id = 1;
+
+  // Host1 (provider_id=1): process_id=1, origin1.
+  ServiceWorkerProviderHost* host1(new ServiceWorkerProviderHost(
+      kRenderProcessId1, MSG_ROUTING_NONE, provider_id++,
+      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
+  host1->SetDocumentUrl(kOrigin1);
+
+  // Host2 (provider_id=2): process_id=2, origin2.
+  ServiceWorkerProviderHost* host2(new ServiceWorkerProviderHost(
+      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
+      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
+  host2->SetDocumentUrl(kOrigin2);
+
+  // Host3 (provider_id=3): process_id=2, origin1.
+  ServiceWorkerProviderHost* host3(new ServiceWorkerProviderHost(
+      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
+      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
+  host3->SetDocumentUrl(kOrigin1);
+
+  // Host4 (provider_id=4): process_id=2, origin2, for ServiceWorker.
+  ServiceWorkerProviderHost* host4(new ServiceWorkerProviderHost(
+      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
+      SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, context()->AsWeakPtr(), nullptr));
+  host4->SetDocumentUrl(kOrigin2);
+
+  context()->AddProviderHost(make_scoped_ptr(host1));
+  context()->AddProviderHost(make_scoped_ptr(host2));
+  context()->AddProviderHost(make_scoped_ptr(host3));
+  context()->AddProviderHost(make_scoped_ptr(host4));
+
+  // Iterate over all provider hosts.
+  std::set<ServiceWorkerProviderHost*> results;
+  for (auto it = context()->GetProviderHostIterator(); !it->IsAtEnd();
+       it->Advance()) {
+    results.insert(it->GetProviderHost());
+  }
+  EXPECT_EQ(4u, results.size());
+  EXPECT_TRUE(ContainsKey(results, host1));
+  EXPECT_TRUE(ContainsKey(results, host2));
+  EXPECT_TRUE(ContainsKey(results, host3));
+  EXPECT_TRUE(ContainsKey(results, host4));
+
+  // Iterate over the client provider hosts that belong to kOrigin1.
+  results.clear();
+  for (auto it = context()->GetClientProviderHostIterator(kOrigin1);
+       !it->IsAtEnd(); it->Advance()) {
+    results.insert(it->GetProviderHost());
+  }
+  EXPECT_EQ(2u, results.size());
+  EXPECT_TRUE(ContainsKey(results, host1));
+  EXPECT_TRUE(ContainsKey(results, host3));
+
+  // Iterate over the provider hosts that belong to kOrigin2.
+  // (This should not include host4 as it's not for controllee.)
+  results.clear();
+  for (auto it = context()->GetClientProviderHostIterator(kOrigin2);
+       !it->IsAtEnd(); it->Advance()) {
+    results.insert(it->GetProviderHost());
+  }
+  EXPECT_EQ(1u, results.size());
+  EXPECT_TRUE(ContainsKey(results, host2));
+
+  context()->RemoveProviderHost(kRenderProcessId1, 1);
+  context()->RemoveProviderHost(kRenderProcessId2, 2);
+  context()->RemoveProviderHost(kRenderProcessId2, 3);
+  context()->RemoveProviderHost(kRenderProcessId2, 4);
+}
+
+class ServiceWorkerContextRecoveryTest
+    : public ServiceWorkerContextTest,
+      public testing::WithParamInterface<bool> {
+ public:
+  ServiceWorkerContextRecoveryTest() {}
+  virtual ~ServiceWorkerContextRecoveryTest() {}
+};
+
+INSTANTIATE_TEST_CASE_P(ServiceWorkerContextRecoveryTest,
+                        ServiceWorkerContextRecoveryTest,
+                        testing::Values(true, false));
+
+TEST_P(ServiceWorkerContextRecoveryTest, DeleteAndStartOver) {
   GURL pattern("http://www.example.com/");
   GURL script_url("http://www.example.com/service_worker.js");
 
-  // Reinitialize the helper to test on-disk storage.
-  base::ScopedTempDir user_data_directory;
-  ASSERT_TRUE(user_data_directory.CreateUniqueTempDir());
-  helper_.reset(new EmbeddedWorkerTestHelper(user_data_directory.path(),
-                                             render_process_id_));
-  helper_->context_wrapper()->AddObserver(this);
+  bool is_storage_on_disk = GetParam();
+  if (is_storage_on_disk) {
+    // Reinitialize the helper to test on-disk storage.
+    base::ScopedTempDir user_data_directory;
+    ASSERT_TRUE(user_data_directory.CreateUniqueTempDir());
+    helper_.reset(new EmbeddedWorkerTestHelper(user_data_directory.path(),
+                                               render_process_id_));
+    helper_->context_wrapper()->AddObserver(this);
+  }
 
   int64 registration_id = kInvalidServiceWorkerRegistrationId;
   bool called = false;
@@ -614,78 +703,5 @@ TEST_F(ServiceWorkerContextTest, DeleteAndStartOver) {
   EXPECT_EQ(registration_id, notifications_[2].registration_id);
 }
 
-TEST_F(ServiceWorkerContextTest, ProviderHostIterator) {
-  const int kRenderProcessId1 = 1;
-  const int kRenderProcessId2 = 2;
-  const GURL kOrigin1 = GURL("http://www.example.com/");
-  const GURL kOrigin2 = GURL("https://www.example.com/");
-  int provider_id = 1;
-
-  // Host1 (provider_id=1): process_id=1, origin1.
-  ServiceWorkerProviderHost* host1(new ServiceWorkerProviderHost(
-      kRenderProcessId1, MSG_ROUTING_NONE, provider_id++,
-      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
-  host1->SetDocumentUrl(kOrigin1);
-
-  // Host2 (provider_id=2): process_id=2, origin2.
-  ServiceWorkerProviderHost* host2(new ServiceWorkerProviderHost(
-      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
-      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
-  host2->SetDocumentUrl(kOrigin2);
-
-  // Host3 (provider_id=3): process_id=2, origin1.
-  ServiceWorkerProviderHost* host3(new ServiceWorkerProviderHost(
-      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
-      SERVICE_WORKER_PROVIDER_FOR_WINDOW, context()->AsWeakPtr(), nullptr));
-  host3->SetDocumentUrl(kOrigin1);
-
-  // Host4 (provider_id=4): process_id=2, origin2, for ServiceWorker.
-  ServiceWorkerProviderHost* host4(new ServiceWorkerProviderHost(
-      kRenderProcessId2, MSG_ROUTING_NONE, provider_id++,
-      SERVICE_WORKER_PROVIDER_FOR_CONTROLLER, context()->AsWeakPtr(), nullptr));
-  host4->SetDocumentUrl(kOrigin2);
-
-  context()->AddProviderHost(make_scoped_ptr(host1));
-  context()->AddProviderHost(make_scoped_ptr(host2));
-  context()->AddProviderHost(make_scoped_ptr(host3));
-  context()->AddProviderHost(make_scoped_ptr(host4));
-
-  // Iterate over all provider hosts.
-  std::set<ServiceWorkerProviderHost*> results;
-  for (auto it = context()->GetProviderHostIterator(); !it->IsAtEnd();
-       it->Advance()) {
-    results.insert(it->GetProviderHost());
-  }
-  EXPECT_EQ(4u, results.size());
-  EXPECT_TRUE(ContainsKey(results, host1));
-  EXPECT_TRUE(ContainsKey(results, host2));
-  EXPECT_TRUE(ContainsKey(results, host3));
-  EXPECT_TRUE(ContainsKey(results, host4));
-
-  // Iterate over the client provider hosts that belong to kOrigin1.
-  results.clear();
-  for (auto it = context()->GetClientProviderHostIterator(kOrigin1);
-       !it->IsAtEnd(); it->Advance()) {
-    results.insert(it->GetProviderHost());
-  }
-  EXPECT_EQ(2u, results.size());
-  EXPECT_TRUE(ContainsKey(results, host1));
-  EXPECT_TRUE(ContainsKey(results, host3));
-
-  // Iterate over the provider hosts that belong to kOrigin2.
-  // (This should not include host4 as it's not for controllee.)
-  results.clear();
-  for (auto it = context()->GetClientProviderHostIterator(kOrigin2);
-       !it->IsAtEnd(); it->Advance()) {
-    results.insert(it->GetProviderHost());
-  }
-  EXPECT_EQ(1u, results.size());
-  EXPECT_TRUE(ContainsKey(results, host2));
-
-  context()->RemoveProviderHost(kRenderProcessId1, 1);
-  context()->RemoveProviderHost(kRenderProcessId2, 2);
-  context()->RemoveProviderHost(kRenderProcessId2, 3);
-  context()->RemoveProviderHost(kRenderProcessId2, 4);
-}
 
 }  // namespace content
diff --git a/content/browser/service_worker/service_worker_database.cc b/content/browser/service_worker/service_worker_database.cc
index c928c38..921bf77 100644
--- a/content/browser/service_worker/service_worker_database.cc
+++ b/content/browser/service_worker/service_worker_database.cc
@@ -970,18 +970,17 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::DestroyDatabase() {
   DCHECK(sequence_checker_.CalledOnValidSequencedThread());
   Disable(FROM_HERE, STATUS_OK);
 
-  leveldb::Options options;
-  if (path_.empty()) {
-    if (env_) {
-      options.env = env_.get();
-    } else {
-      // In-memory database not initialized.
-      return STATUS_OK;
-    }
+  if (IsDatabaseInMemory()) {
+    env_.reset();
+    return STATUS_OK;
   }
 
-  Status status =
-      LevelDBStatusToStatus(leveldb::DestroyDB(path_.AsUTF8Unsafe(), options));
+  // Directly delete the database directory instead of leveldb::DestroyDB()
+  // because the API does not delete the directory if there are unrelated files.
+  // (https://code.google.com/p/chromium/issues/detail?id=468926#c24)
+  Status status = base::DeleteFile(path_, true /* recursive */)
+                      ? STATUS_OK
+                      : STATUS_ERROR_FAILED;
   ServiceWorkerMetrics::RecordDestroyDatabaseResult(status);
   return status;
 }
@@ -996,13 +995,9 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
   if (IsOpen())
     return STATUS_OK;
 
-  // When |path_| is empty, open a database in-memory.
-  bool use_in_memory_db = path_.empty();
-
   if (!create_if_missing) {
     // Avoid opening a database if it does not exist at the |path_|.
-    if (use_in_memory_db ||
-        !base::PathExists(path_) ||
+    if (IsDatabaseInMemory() || !base::PathExists(path_) ||
         base::IsDirectoryEmpty(path_)) {
       return STATUS_ERROR_NOT_FOUND;
     }
@@ -1011,7 +1006,7 @@ ServiceWorkerDatabase::Status ServiceWorkerDatabase::LazyOpen(
   leveldb::Options options;
   options.create_if_missing = create_if_missing;
   options.reuse_logs = leveldb_env::kDefaultLogReuseOptionValue;
-  if (use_in_memory_db) {
+  if (IsDatabaseInMemory()) {
     env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
     options.env = env_.get();
   }
@@ -1463,4 +1458,8 @@ void ServiceWorkerDatabase::HandleWriteResult(
   ServiceWorkerMetrics::CountWriteDatabaseResult(status);
 }
 
+bool ServiceWorkerDatabase::IsDatabaseInMemory() const {
+  return path_.empty();
+}
+
 }  // namespace content
diff --git a/content/browser/service_worker/service_worker_database.h b/content/browser/service_worker/service_worker_database.h
index 13c0ed4..241a8b1 100644
--- a/content/browser/service_worker/service_worker_database.h
+++ b/content/browser/service_worker/service_worker_database.h
@@ -350,7 +350,7 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
       const tracked_objects::Location& from_here,
       Status status);
 
-  base::FilePath path_;
+  const base::FilePath path_;
   scoped_ptr<leveldb::Env> env_;
   scoped_ptr<leveldb::DB> db_;
 
@@ -365,6 +365,8 @@ class CONTENT_EXPORT ServiceWorkerDatabase {
   };
   State state_;
 
+  bool IsDatabaseInMemory() const;
+
   base::SequenceChecker sequence_checker_;
 
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerDatabaseTest, OpenDatabase);
diff --git a/content/browser/service_worker/service_worker_storage.cc b/content/browser/service_worker/service_worker_storage.cc
index c5c6157..6671523 100644
--- a/content/browser/service_worker/service_worker_storage.cc
+++ b/content/browser/service_worker/service_worker_storage.cc
@@ -825,13 +825,11 @@ void ServiceWorkerStorage::DeleteAndStartOver(const StatusCallback& callback) {
 
   // Delete the database on the database thread.
   PostTaskAndReplyWithResult(
-      database_task_manager_->GetTaskRunner(),
-      FROM_HERE,
+      database_task_manager_->GetTaskRunner(), FROM_HERE,
       base::Bind(&ServiceWorkerDatabase::DestroyDatabase,
                  base::Unretained(database_.get())),
       base::Bind(&ServiceWorkerStorage::DidDeleteDatabase,
-                 weak_factory_.GetWeakPtr(),
-                 callback));
+                 weak_factory_.GetWeakPtr(), callback));
 }
 
 int64 ServiceWorkerStorage::NewRegistrationId() {
diff --git a/content/browser/service_worker/service_worker_storage.h b/content/browser/service_worker/service_worker_storage.h
index 1bfa737..b175d42 100644
--- a/content/browser/service_worker/service_worker_storage.h
+++ b/content/browser/service_worker/service_worker_storage.h
@@ -231,6 +231,12 @@ class CONTENT_EXPORT ServiceWorkerStorage
                            CleanupOnRestart);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
                            ClearOnExit);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
+                           DeleteAndStartOver);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
+                           DeleteAndStartOver_UnrelatedFileExists);
+  FRIEND_TEST_ALL_PREFIXES(ServiceWorkerResourceStorageDiskTest,
+                           DeleteAndStartOver_OpenedFileExists);
 
   struct InitialData {
     int64 next_registration_id;
diff --git a/content/browser/service_worker/service_worker_storage_unittest.cc b/content/browser/service_worker/service_worker_storage_unittest.cc
index aa2ceb5..bdfbe18 100644
--- a/content/browser/service_worker/service_worker_storage_unittest.cc
+++ b/content/browser/service_worker/service_worker_storage_unittest.cc
@@ -4,6 +4,7 @@
 
 #include <string>
 
+#include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/run_loop.h"
@@ -35,6 +36,13 @@ namespace {
 typedef ServiceWorkerDatabase::RegistrationData RegistrationData;
 typedef ServiceWorkerDatabase::ResourceRecord ResourceRecord;
 
+void StatusAndQuitCallback(ServiceWorkerStatusCode* result,
+                           const base::Closure& quit_closure,
+                           ServiceWorkerStatusCode status) {
+  *result = status;
+  quit_closure.Run();
+}
+
 void StatusCallback(bool* was_called,
                     ServiceWorkerStatusCode* result,
                     ServiceWorkerStatusCode status) {
@@ -1241,6 +1249,82 @@ TEST_F(ServiceWorkerResourceStorageDiskTest, CleanupOnRestart) {
   EXPECT_TRUE(VerifyBasicResponse(storage(), kNewResourceId, true));
 }
 
+TEST_F(ServiceWorkerResourceStorageDiskTest, DeleteAndStartOver) {
+  EXPECT_FALSE(storage()->IsDisabled());
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDatabasePath()));
+
+  base::RunLoop run_loop;
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_ABORT;
+  storage()->DeleteAndStartOver(
+      base::Bind(&StatusAndQuitCallback, &status, run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_TRUE(storage()->IsDisabled());
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDatabasePath()));
+}
+
+TEST_F(ServiceWorkerResourceStorageDiskTest,
+       DeleteAndStartOver_UnrelatedFileExists) {
+  EXPECT_FALSE(storage()->IsDisabled());
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDatabasePath()));
+
+  // Create an unrelated file in the database directory to make sure such a file
+  // does not prevent DeleteAndStartOver.
+  base::FilePath file_path;
+  ASSERT_TRUE(
+      base::CreateTemporaryFileInDir(storage()->GetDatabasePath(), &file_path));
+  ASSERT_TRUE(base::PathExists(file_path));
+
+  base::RunLoop run_loop;
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_ABORT;
+  storage()->DeleteAndStartOver(
+      base::Bind(&StatusAndQuitCallback, &status, run_loop.QuitClosure()));
+  run_loop.Run();
+
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_TRUE(storage()->IsDisabled());
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDatabasePath()));
+}
+
+TEST_F(ServiceWorkerResourceStorageDiskTest,
+       DeleteAndStartOver_OpenedFileExists) {
+  EXPECT_FALSE(storage()->IsDisabled());
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  ASSERT_TRUE(base::DirectoryExists(storage()->GetDatabasePath()));
+
+  // Create an unrelated opened file in the database directory to make sure such
+  // a file does not prevent DeleteAndStartOver on non-Windows platforms.
+  base::FilePath file_path;
+  base::ScopedFILE file(base::CreateAndOpenTemporaryFileInDir(
+      storage()->GetDatabasePath(), &file_path));
+  ASSERT_TRUE(file);
+  ASSERT_TRUE(base::PathExists(file_path));
+
+  base::RunLoop run_loop;
+  ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_ABORT;
+  storage()->DeleteAndStartOver(
+      base::Bind(&StatusAndQuitCallback, &status, run_loop.QuitClosure()));
+  run_loop.Run();
+
+#if defined(OS_WIN)
+  // On Windows, deleting the directory containing an opened file should fail.
+  EXPECT_EQ(SERVICE_WORKER_ERROR_FAILED, status);
+  EXPECT_TRUE(storage()->IsDisabled());
+  EXPECT_TRUE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  EXPECT_TRUE(base::DirectoryExists(storage()->GetDatabasePath()));
+#else
+  EXPECT_EQ(SERVICE_WORKER_OK, status);
+  EXPECT_TRUE(storage()->IsDisabled());
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDiskCachePath()));
+  EXPECT_FALSE(base::DirectoryExists(storage()->GetDatabasePath()));
+#endif
+}
+
 TEST_F(ServiceWorkerResourceStorageTest, UpdateRegistration) {
   // Promote the worker to active worker and add a controllee.
   registration_->SetActiveVersion(registration_->waiting_version());