ServiceWorker: Migrate the script cache backend from BlockFile to Simple

content/browser/service_worker/service_worker_storage.cc

Index: content/browser/service_worker/service_worker_storage.cc
diff --git a/content/browser/service_worker/service_worker_storage.cc b/content/browser/service_worker/service_worker_storage.cc
index 4f714195ce0ea688daffbebcf25093034b944c16..840eecebe29897b4bd29ef2a7bd0248b374bf454 100644
--- a/content/browser/service_worker/service_worker_storage.cc
+++ b/content/browser/service_worker/service_worker_storage.cc
@@ -4,6 +4,7 @@
 
 #include "content/browser/service_worker/service_worker_storage.h"
 
+#include "base/barrier_closure.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
@@ -14,8 +15,8 @@
 #include "base/trace_event/trace_event.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_disk_cache.h"
+#include "content/browser/service_worker/service_worker_disk_cache_migrator.h"
 #include "content/browser/service_worker/service_worker_info.h"
-#include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_utils.h"
 #include "content/browser/service_worker/service_worker_version.h"
@@ -60,7 +61,8 @@ void CompleteFindSoon(
 const base::FilePath::CharType kDatabaseName[] =
     FILE_PATH_LITERAL("Database");
 const base::FilePath::CharType kDiskCacheName[] =
-    FILE_PATH_LITERAL("Cache");
+    FILE_PATH_LITERAL("ScriptCache");
+const base::FilePath::CharType kOldDiskCacheName[] = FILE_PATH_LITERAL("Cache");
 
 const int kMaxMemDiskCacheSize = 10 * 1024 * 1024;
 const int kMaxDiskCacheSize = 250 * 1024 * 1024;
@@ -929,6 +931,7 @@ ServiceWorkerStorage::ServiceWorkerStorage(
       disk_cache_thread_(disk_cache_thread),
       quota_manager_proxy_(quota_manager_proxy),
       special_storage_policy_(special_storage_policy),
+      needs_disk_cache_migration_(false),
       is_purge_pending_(false),
       has_checked_for_stale_resources_(false),
       weak_factory_(this) {
@@ -949,6 +952,13 @@ base::FilePath ServiceWorkerStorage::GetDiskCachePath() {
       .Append(kDiskCacheName);
 }
 
+base::FilePath ServiceWorkerStorage::GetOldDiskCachePath() {
+  if (path_.empty())
+    return base::FilePath();
+  return path_.Append(ServiceWorkerContextCore::kServiceWorkerDirectory)
+      .Append(kOldDiskCacheName);
+}
+
 bool ServiceWorkerStorage::LazyInitialize(const base::Closure& callback) {
   if (!context_)
     return false;
@@ -969,9 +979,8 @@ bool ServiceWorkerStorage::LazyInitialize(const base::Closure& callback) {
   state_ = INITIALIZING;
   database_task_manager_->GetTaskRunner()->PostTask(
       FROM_HERE,
-      base::Bind(&ReadInitialDataFromDB,
-                 database_.get(),
-                 base::ThreadTaskRunnerHandle::Get(),
+      base::Bind(&ReadInitialDataFromDB, database_.get(),
+                 base::ThreadTaskRunnerHandle::Get(), GetOldDiskCachePath(),
                  base::Bind(&ServiceWorkerStorage::DidReadInitialData,
                             weak_factory_.GetWeakPtr())));
   return false;
@@ -979,6 +988,7 @@ bool ServiceWorkerStorage::LazyInitialize(const base::Closure& callback) {
 
 void ServiceWorkerStorage::DidReadInitialData(
     InitialData* data,
+    bool needs_disk_cache_migration,
     ServiceWorkerDatabase::Status status) {
   DCHECK(data);
   DCHECK_EQ(INITIALIZING, state_);
@@ -989,6 +999,7 @@ void ServiceWorkerStorage::DidReadInitialData(
     next_resource_id_ = data->next_resource_id;
     registered_origins_.swap(data->origins);
     state_ = INITIALIZED;
+    needs_disk_cache_migration_ = needs_disk_cache_migration;

[michaeln, 2015/06/08 21:36:33]

Curiosity: I wish we didn't have to migrate at all because too many thing can go
wrong. What is it that is forced us to migrate instead of going with the
comparatively simple starting with a clean slate?

[nhiroki, 2015/06/09 15:41:56]

Yeah... it's definitely simpler and easier to achieve..., but it'd be preferable
to avoid losing (push) registrations:
https://code.google.com/p/chromium/issues/detail?id=487482#c13

   } else {
     DVLOG(2) << "Failed to initialize: "
              << ServiceWorkerDatabase::StatusToString(status);
@@ -1368,30 +1379,160 @@ ServiceWorkerStorage::FindInstallingRegistrationForId(
 }
 
 ServiceWorkerDiskCache* ServiceWorkerStorage::disk_cache() {
+  DCHECK_EQ(INITIALIZED, state_);
   if (disk_cache_)
     return disk_cache_.get();
 
-  disk_cache_ = ServiceWorkerDiskCache::CreateWithBlockFileBackend();
+  disk_cache_ = ServiceWorkerDiskCache::CreateWithSimpleBackend();
 
-  base::FilePath path = GetDiskCachePath();
-  if (path.empty()) {
+  if (GetDiskCachePath().empty()) {
     int rv = disk_cache_->InitWithMemBackend(kMaxMemDiskCacheSize,
                                              net::CompletionCallback());
     DCHECK_EQ(net::OK, rv);
     return disk_cache_.get();
   }
 
+  if (needs_disk_cache_migration_) {
+    // Defer the start of initialization until the migration is complete.
+    disk_cache_->set_is_waiting_to_initialize(true);
+    PrepareDiskCacheForMigration();
+    return disk_cache_.get();
+  }
+
+  // DiskCache has already been migrated.
+  ServiceWorkerMetrics::RecordDiskCacheMigrationResult(
+      ServiceWorkerMetrics::MIGRATION_NOT_NECESSARY);
+  InitializeDiskCache();
+
+  return disk_cache_.get();
+}
+
+void ServiceWorkerStorage::PrepareDiskCacheForMigration() {
+  scoped_ptr<ServiceWorkerDiskCache> src_disk_cache =
+      ServiceWorkerDiskCache::CreateWithBlockFileBackend();
+  ServiceWorkerDiskCache* src_disk_cache_ptr = src_disk_cache.get();
+  scoped_ptr<ServiceWorkerDiskCache> dest_disk_cache =
+      ServiceWorkerDiskCache::CreateWithSimpleBackend();
+  ServiceWorkerDiskCache* dest_disk_cache_ptr = dest_disk_cache.get();
+  bool* is_failed = new bool(false);
+
+  // This closure is called when both diskcaches are initialized.
+  base::Closure barrier_closure = base::BarrierClosure(
+      2, base::Bind(
+             &ServiceWorkerStorage::DidPrepareDiskCacheForMigration,
+             weak_factory_.GetWeakPtr(), base::Passed(src_disk_cache.Pass()),
+             base::Passed(dest_disk_cache.Pass()), base::Owned(is_failed)));
+
+  // Initialize the src DiskCache.
+  net::CompletionCallback src_callback =
+      base::Bind(&ServiceWorkerStorage::DidInitializeDiskCacheForMigration,
+                 weak_factory_.GetWeakPtr(), is_failed, barrier_closure);
+  int result = src_disk_cache_ptr->InitWithDiskBackend(
+      GetOldDiskCachePath(), kMaxDiskCacheSize, false /* force */,
+      disk_cache_thread_, src_callback);
+  if (result != net::ERR_IO_PENDING)
+    src_callback.Run(result);
+
+  // Initialize the dest DiskCache.

[michaeln, 2015/06/08 21:36:33]

Since a migration attempt can be started multiple times, should the first step
be to delete any partial results from an earlier attempt?

[nhiroki, 2015/06/09 15:41:56]

Done.

+  net::CompletionCallback dest_callback =
+      base::Bind(&ServiceWorkerStorage::DidInitializeDiskCacheForMigration,
+                 weak_factory_.GetWeakPtr(), is_failed, barrier_closure);
+  result = dest_disk_cache_ptr->InitWithDiskBackend(
+      GetDiskCachePath(), kMaxDiskCacheSize, false /* force */,
+      disk_cache_thread_, dest_callback);
+  if (result != net::ERR_IO_PENDING)
+    dest_callback.Run(result);
+}
+
+void ServiceWorkerStorage::DidInitializeDiskCacheForMigration(
+    bool* is_failed,
+    const base::Closure& barrier_closure,
+    int result) {
+  if (result != net::OK)
+    *is_failed = true;
+  barrier_closure.Run();
+}
+
+void ServiceWorkerStorage::DidPrepareDiskCacheForMigration(
+    scoped_ptr<ServiceWorkerDiskCache> src_disk_cache,
+    scoped_ptr<ServiceWorkerDiskCache> dest_disk_cache,
+    bool* is_failed) {
+  if (*is_failed) {
+    LOG(ERROR) << "Failed to initialize the diskcache.";
+    src_disk_cache.reset();
+    dest_disk_cache.reset();
+    AbortDiskCacheMigration(ServiceWorkerMetrics::MIGRATION_ERROR_INITIALIZE);
+    return;
+  }
+
+  // Start migrating resources stored in the src DiskCache.
+  scoped_ptr<ServiceWorkerDiskCacheMigrator> migrator(
+      new ServiceWorkerDiskCacheMigrator(src_disk_cache.get(),
+                                         dest_disk_cache.get()));
+  ServiceWorkerDiskCacheMigrator* migrator_ptr = migrator.get();
+  migrator_ptr->Start(base::Bind(

[michaeln, 2015/06/08 21:36:33]

The migrator will continue to run even after ServiceWorkerStorage has been
deleted, yet the results of a migrator left running detached from its storage
will never be used. Seems like we should abort the migrator in this case to
avoid it doing a bunch of unwanted io during shutdown.

[nhiroki, 2015/06/09 15:41:56]

I changed the storage to own the migrator. When the storage is destroyed, the
migrator is also destroyed and running/pending migration tasks bound with
weakptr<migrator> are aborted.

+      &ServiceWorkerStorage::DidMigrateDiskCache, weak_factory_.GetWeakPtr(),
+      base::Passed(migrator.Pass()), base::Passed(src_disk_cache.Pass()),
+      base::Passed(dest_disk_cache.Pass())));

[michaeln, 2015/06/08 21:36:33]

having ownership of src and dst bound into the closure works, but having the
migrator own them outright seems easier to grok

[nhiroki, 2015/06/09 15:41:57]

Moved the initialization code from storage to migrator.

+}
+
+void ServiceWorkerStorage::DidMigrateDiskCache(
+    scoped_ptr<ServiceWorkerDiskCacheMigrator> migrator,
+    scoped_ptr<ServiceWorkerDiskCache> src_disk_cache,
+    scoped_ptr<ServiceWorkerDiskCache> dest_disk_cache,
+    ServiceWorkerStatusCode status) {
+  migrator.reset();
+  src_disk_cache.reset();
+  dest_disk_cache.reset();
+
+  if (status != SERVICE_WORKER_OK) {
+    LOG(ERROR) << "Failed to migrate the diskcache: "
+               << ServiceWorkerStatusToString(status);
+    AbortDiskCacheMigration(ServiceWorkerMetrics::MIGRATION_ERROR_FAILED);
+    return;
+  }
+
+  // Delete the old DiskCache directory.
+  PostTaskAndReplyWithResult(
+      disk_cache_thread_.get(), FROM_HERE,
+      base::Bind(&base::DeleteFile, GetOldDiskCachePath(), true),
+      base::Bind(&ServiceWorkerStorage::DidDeleteOldDiskCache,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void ServiceWorkerStorage::DidDeleteOldDiskCache(bool deleted) {
+  if (!deleted) {
+    LOG(ERROR) << "Failed to delete the old diskcache directory";
+    AbortDiskCacheMigration(

[michaeln, 2015/06/08 21:36:33]

Seems unfortunate to let this error prevent migration completion since the dest
cache is successfully written and ready to go? Likely the call in line 1523 will
also fail to delete the old. Hmmm, if we're unable to delete the old dir we'll
be wedged forever trying to migrate?

[nhiroki, 2015/06/09 15:41:57]

Changed this sequence. The latest patchset records the old diskcache directory
in the database (INITDATA_PURGEABLE_FILE) and tries to delete it independently
from the migration attempt.

+        ServiceWorkerMetrics::MIGRATION_ERROR_DELETE_DISK_CACHE);
+    return;
+  }
+  ServiceWorkerMetrics::RecordDiskCacheMigrationResult(
+      ServiceWorkerMetrics::MIGRATION_OK);
+  InitializeDiskCache();
+}
+
+void ServiceWorkerStorage::AbortDiskCacheMigration(

[michaeln, 2015/06/08 21:36:33]

This method doesn't actually abort anything, it handles a migration failure.
Maybe rename the method?

[nhiroki, 2015/06/09 15:41:57]

(This function is gone)

+    ServiceWorkerMetrics::DiskCacheMigrationResult result) {
+  ServiceWorkerMetrics::RecordDiskCacheMigrationResult(result);
+
+  // Give up initialization and recreate the whole storage.
+  ScheduleDeleteAndStartOver();
+
+  // Delete the old disk directory because DeleteAndStartOver doesn't.
+  disk_cache_thread_->PostTask(FROM_HERE,
+                               base::Bind(base::IgnoreResult(&base::DeleteFile),
+                                          GetOldDiskCachePath(), true));
+}
+
+void ServiceWorkerStorage::InitializeDiskCache() {
+  disk_cache_->set_is_waiting_to_initialize(false);
   int rv = disk_cache_->InitWithDiskBackend(
-      path,
-      kMaxDiskCacheSize,
-      false,
-      disk_cache_thread_,
+      GetDiskCachePath(), kMaxDiskCacheSize, false, disk_cache_thread_,
       base::Bind(&ServiceWorkerStorage::OnDiskCacheInitialized,
                  weak_factory_.GetWeakPtr()));
   if (rv != net::ERR_IO_PENDING)
     OnDiskCacheInitialized(rv);
-
-  return disk_cache_.get();
 }
 
 void ServiceWorkerStorage::OnDiskCacheInitialized(int rv) {
@@ -1534,6 +1675,7 @@ void ServiceWorkerStorage::CollectStaleResourcesFromDB(
 void ServiceWorkerStorage::ReadInitialDataFromDB(
     ServiceWorkerDatabase* database,
     scoped_refptr<base::SequencedTaskRunner> original_task_runner,
+    const base::FilePath& old_disk_cache_path,
     const InitializeCallback& callback) {
   DCHECK(database);
   scoped_ptr<ServiceWorkerStorage::InitialData> data(
@@ -1545,13 +1687,19 @@ void ServiceWorkerStorage::ReadInitialDataFromDB(
                                     &data->next_resource_id);
   if (status != ServiceWorkerDatabase::STATUS_OK) {
     original_task_runner->PostTask(
-        FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
+        FROM_HERE, base::Bind(callback, base::Owned(data.release()),
+                              false /* needs_disk_cache_migration */, status));
     return;
   }
 
+  bool needs_disk_cache_migration = false;
+  if (!old_disk_cache_path.empty())
+    needs_disk_cache_migration = base::DirectoryExists(old_disk_cache_path);

[michaeln, 2015/06/08 21:36:33]

I'm wondering about pathological cases where migration fails? How do we handle
different failure modes. As coded looks like we could repeatedly try to migrate
during a browsing session. I'm back to wondering if we should store whether
migration has happened or is needed in the db proper to handle unable to delete
the old case? Also wondering if the name of the scriptcache directory should be
part of the db schema to handle being unable to delete partial results from an
earlier migration attempt?

[nhiroki, 2015/06/09 15:41:57]

Added INITDATA_DISKCACHE_VERSION field in the database. We should be able to
distinguish the migration state by the version number.
In the latest CL, if the migrator is still running on the dtor of the storage,
the new diskcache directory which contains partial results is recorded as a
purgeable file in the database. Purgeable files are removed on the next system
startup.

(We might not have to do this if we delete partial results immediately before
the migration as you commented at line 1436?)

+
   status = database->GetOriginsWithRegistrations(&data->origins);
   original_task_runner->PostTask(
-      FROM_HERE, base::Bind(callback, base::Owned(data.release()), status));
+      FROM_HERE, base::Bind(callback, base::Owned(data.release()),
+                            needs_disk_cache_migration, status));
 }
 
 void ServiceWorkerStorage::DeleteRegistrationFromDB(