Appcache::OnCorruptionDetected handling

webkit/browser/appcache/appcache_storage_impl.cc

Index: webkit/browser/appcache/appcache_storage_impl.cc
diff --git a/webkit/browser/appcache/appcache_storage_impl.cc b/webkit/browser/appcache/appcache_storage_impl.cc
index e0d02632cf236c45db955c3104eed26f84e11e34..968486e73d8db4dd391c5946c9b1b00e46a0f278 100644
--- a/webkit/browser/appcache/appcache_storage_impl.cc
+++ b/webkit/browser/appcache/appcache_storage_impl.cc
@@ -174,7 +174,7 @@ class AppCacheStorageImpl::DatabaseTask
  private:
   void CallRun(base::TimeTicks schedule_time);
   void CallRunCompleted(base::TimeTicks schedule_time);
-  void CallDisableStorage();
+  void OnFatalError();
 
   scoped_refptr<base::MessageLoopProxy> io_thread_;
 };
@@ -209,10 +209,15 @@ void AppCacheStorageImpl::DatabaseTask::CallRun(
     Run();
     AppCacheHistograms::AddTaskRunTimeSample(
         base::TimeTicks::Now() - run_time);
+
+    if (database_->was_corruption_detected()) {
+      AppCacheHistograms::CountCorruptionDetected();
+      database_->Disable();
+    }
     if (database_->is_disabled()) {
       io_thread_->PostTask(
           FROM_HERE,
-          base::Bind(&DatabaseTask::CallDisableStorage, this));
+          base::Bind(&DatabaseTask::OnFatalError, this));
     }
   }
   io_thread_->PostTask(
@@ -237,10 +242,11 @@ void AppCacheStorageImpl::DatabaseTask::CallRunCompleted(
   }
 }
 
-void AppCacheStorageImpl::DatabaseTask::CallDisableStorage() {
+void AppCacheStorageImpl::DatabaseTask::OnFatalError() {
   if (storage_) {
     DCHECK(io_thread_->BelongsToCurrentThread());
     storage_->Disable();
+    storage_->DeleteAndStartOver();
   }
 }
 
@@ -251,7 +257,14 @@ class AppCacheStorageImpl::InitTask : public DatabaseTask {
   explicit InitTask(AppCacheStorageImpl* storage)
       : DatabaseTask(storage), last_group_id_(0),
         last_cache_id_(0), last_response_id_(0),
-        last_deletable_response_rowid_(0) {}
+        last_deletable_response_rowid_(0) {
+    if (!storage->is_incognito_) {
+      db_file_path_ =
+          storage->cache_directory_.Append(kAppCacheDatabaseName);
+      disk_cache_directory_ =
+          storage->cache_directory_.Append(kDiskCacheDirectoryName);
+    }
+  }
 
   // DatabaseTask:
   virtual void Run() OVERRIDE;
@@ -261,6 +274,8 @@ class AppCacheStorageImpl::InitTask : public DatabaseTask {
   virtual ~InitTask() {}
 
  private:
+  base::FilePath db_file_path_;
+  base::FilePath disk_cache_directory_;
   int64 last_group_id_;
   int64 last_cache_id_;
   int64 last_response_id_;
@@ -269,6 +284,19 @@ class AppCacheStorageImpl::InitTask : public DatabaseTask {
 };
 
 void AppCacheStorageImpl::InitTask::Run() {
+  if (!db_file_path_.empty()) {
+    // If there is no sql database, ensure there is no disk cache either.
+    if (!base::PathExists(db_file_path_)) {
+      if (base::DirectoryExists(disk_cache_directory_)) {

[jsbell, 2014/01/27 21:21:51]

Collapse these 3 if()s into if(a && b && c) to reduce the nesting?

[michaeln, 2014/01/28 22:12:16]

sgtm, done

+        base::DeleteFile(disk_cache_directory_, true);
+        if(base::DirectoryExists(disk_cache_directory_)) {
+          database_->Disable();  // This triggers OnFatalError handling.
+          return;
+        }
+      }
+    }
+  }
+
   database_->FindLastStorageIds(
       &last_group_id_, &last_cache_id_, &last_response_id_,
       &last_deletable_response_rowid_);
@@ -913,8 +941,6 @@ class AppCacheStorageImpl::FindMainResponseTask : public DatabaseTask {
   GURL manifest_url_;
 };
 
-
-
 void AppCacheStorageImpl::FindMainResponseTask::Run() {
   // NOTE: The heuristics around choosing amoungst multiple candidates
   // is underspecified, and just plain not fully understood. This needs
@@ -1793,9 +1819,6 @@ AppCacheDiskCache* AppCacheStorageImpl::disk_cache() {
                      base::Unretained(this)));
     }
 
-    // We should not keep this reference around.
-    cache_thread_ = NULL;
-
     if (rv != net::ERR_IO_PENDING)
       OnDiskCacheInitialized(rv);
   }
@@ -1811,21 +1834,46 @@ void AppCacheStorageImpl::OnDiskCacheInitialized(int rv) {
     // really recover from. We handle it by temporarily disabling the appcache
     // deleting the directory on disk and reinitializing the appcache system.
     Disable();
-    if (!is_incognito_ && rv != net::ERR_ABORTED) {
-      VLOG(1) << "Deleting existing appcache data and starting over.";
-      db_thread_->PostTaskAndReply(
-          FROM_HERE,
-          base::Bind(base::IgnoreResult(&base::DeleteFile),
-                     cache_directory_, true),
-          base::Bind(&AppCacheStorageImpl::CallReinitialize,
-                     weak_factory_.GetWeakPtr()));
-    }
+    if (rv != net::ERR_ABORTED)
+      DeleteAndStartOver();
+  }
+}
+
+namespace {
+
+void Noop() {
+  // Function body is intentionally empty.
+}
+
+}
+
+void AppCacheStorageImpl::DeleteAndStartOver() {
+  DCHECK(is_disabled_);
+  if (!is_incognito_) {
+    VLOG(1) << "Deleting existing appcache data and starting over.";
+    // We can have tasks in flight to close file handles on both the db
+    // and cache threads, we need to allow those tasks to cycle thru
+    // prior to deleting the files and calling reinit.
+    cache_thread_->PostTaskAndReply(
+        FROM_HERE,
+        base::Bind(&Noop),
+        base::Bind(&AppCacheStorageImpl::DeleteAndStartOverPart2,
+                   weak_factory_.GetWeakPtr()));
   }
 }
 
-void AppCacheStorageImpl::CallReinitialize() {
-  service_->Reinitialize();
-  // note: 'this' may be deleted during reinit.
+void AppCacheStorageImpl::DeleteAndStartOverPart2() {
+  db_thread_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(base::IgnoreResult(&base::DeleteFile),
+                 cache_directory_, true),
+      base::Bind(&AppCacheStorageImpl::CallScheduleReinitialize,
+                  weak_factory_.GetWeakPtr()));
+}
+
+void AppCacheStorageImpl::CallScheduleReinitialize() {
+  service_->ScheduleReinitialize();
+  // note: 'this' may be deleted at this point.
 }
 
 }  // namespace appcache