Change store group and newest cache API so that storage failure does not require storage clients to revert changes to group and so it's not racey.TEST=existing tests updatedBUG=none

Review URL: http://codereview.chromium.org/384029

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@31730 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 18 insertions, 45 deletions

diff --git a/webkit/appcache/appcache_group.cc b/webkit/appcache/appcache_group.cc
index 9efd8ca2..5107b12 100644
--- a/webkit/appcache/appcache_group.cc
+++ b/webkit/appcache/appcache_group.cc
@@ -89,18 +89,6 @@ void AppCacheGroup::RemoveCache(AppCache* cache) {
   }
 }
 
-void AppCacheGroup::RestoreCacheAsNewest(AppCache* former_newest_cache) {
-  newest_complete_cache_->set_owning_group(NULL);
-  newest_complete_cache_ = former_newest_cache;
-  if (former_newest_cache) {
-    DCHECK(former_newest_cache->owning_group() == this);
-    Caches::iterator it =
-        std::find(old_caches_.begin(), old_caches_.end(), former_newest_cache);
-    DCHECK(it != old_caches_.end());
-    old_caches_.erase(it);
-  }
-}
-
 void AppCacheGroup::StartUpdateWithNewMasterEntry(
     AppCacheHost* host, const GURL& new_master_resource) {
   if (!update_job_)
diff --git a/webkit/appcache/appcache_group.h b/webkit/appcache/appcache_group.h
index fa0f7da..166ec21 100644
--- a/webkit/appcache/appcache_group.h
+++ b/webkit/appcache/appcache_group.h
@@ -88,10 +88,6 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
 
   const Caches& old_caches() const { return old_caches_; }
 
-  // Used by update process to restore the group's newest cache if storage
-  // fails to store the newly created cache.
-  void RestoreCacheAsNewest(AppCache* cache);
-
   GURL manifest_url_;
   UpdateStatus update_status_;
   bool is_obsolete_;
diff --git a/webkit/appcache/appcache_storage.h b/webkit/appcache/appcache_storage.h
index 67717a5..02b6884 100644
--- a/webkit/appcache/appcache_storage.h
+++ b/webkit/appcache/appcache_storage.h
@@ -88,12 +88,14 @@ class AppCacheStorage {
   // Schedules a group and its newest complete cache to be initially stored or
   // incrementally updated with new changes. Upon completion the delegate
   // will be called back. A group without a newest cache cannot be stored.
-  // It's a programming error to call this method with such a group. A
+  // It's a programming error to call this method without a newest cache. A
   // side effect of storing a new newest cache is the removal of the group's
   // old caches and responses from persistent storage (although they may still
-  // linger in the in-memory working set until no longer needed).
+  // linger in the in-memory working set until no longer needed). The new
+  // cache will be added as the group's newest complete cache only if storage
+  // succeeds.
   virtual void StoreGroupAndNewestCache(
-      AppCacheGroup* group, Delegate* delegate) = 0;
+      AppCacheGroup* group, AppCache* newest_cache, Delegate* delegate) = 0;
 
   // Schedules a query to identify a response for a main request. Upon
   // completion the delegate will be called back.
diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index b1cf778..7887ee1 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -557,17 +557,15 @@ void AppCacheUpdateJob::OnManifestDataWriteComplete(int result) {
 void AppCacheUpdateJob::CompleteInprogressCache() {
   inprogress_cache_->set_update_time(base::TimeTicks::Now());
   inprogress_cache_->set_complete(true);
-
-  protect_former_newest_cache_ = group_->newest_complete_cache();
-  group_->AddCache(inprogress_cache_);
+  service_->storage()->StoreGroupAndNewestCache(group_, inprogress_cache_,
+                                                this);  // async
   protect_new_cache_.swap(inprogress_cache_);
-
-  service_->storage()->StoreGroupAndNewestCache(group_, this);  // async
 }
 
 void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
                                                     bool success) {
   if (success) {
+    DCHECK_EQ(protect_new_cache_, group->newest_complete_cache());
     if (update_type_ == CACHE_ATTEMPT)
       NotifyAllAssociatedHosts(CACHED_EVENT);
     else
@@ -575,15 +573,11 @@ void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
     internal_state_ = COMPLETED;
     MaybeCompleteUpdate();  // will definitely complete
   } else {
-    // TODO(jennb): Change storage so clients won't need to revert group state?
-    // Change group back to reflect former newest group.
-    group_->RestoreCacheAsNewest(protect_former_newest_cache_);
     protect_new_cache_ = NULL;
 
     // Treat storage failure as if manifest refetch failed.
     HandleManifestRefetchFailure();
   }
-  protect_former_newest_cache_ = NULL;
 }
 
 void AppCacheUpdateJob::HandleManifestRefetchFailure() {
diff --git a/webkit/appcache/appcache_update_job.h b/webkit/appcache/appcache_update_job.h
index 7b9bd51..b65ecd5 100644
--- a/webkit/appcache/appcache_update_job.h
+++ b/webkit/appcache/appcache_update_job.h
@@ -168,10 +168,6 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   // the notification when its status is set to IDLE in ~AppCacheUpdateJob.
   scoped_refptr<AppCache> protect_new_cache_;
 
-  // Hold a reference to the group's newest cache (prior to update) in order
-  // to restore the group's newest cache if storage fails.
-  scoped_refptr<AppCache> protect_former_newest_cache_;
-
   AppCacheGroup* group_;
 
   UpdateType update_type_;
diff --git a/webkit/appcache/mock_appcache_storage.cc b/webkit/appcache/mock_appcache_storage.cc
index d9c63b0..919e5f1 100644
--- a/webkit/appcache/mock_appcache_storage.cc
+++ b/webkit/appcache/mock_appcache_storage.cc
@@ -71,15 +71,14 @@ void MockAppCacheStorage::LoadOrCreateGroup(
 }
 
 void MockAppCacheStorage::StoreGroupAndNewestCache(
-    AppCacheGroup* group, Delegate* delegate) {
+    AppCacheGroup* group, AppCache* newest_cache, Delegate* delegate) {
   DCHECK(group && delegate);
-  DCHECK(group->newest_complete_cache());
+  DCHECK(newest_cache && newest_cache->is_complete());
 
   // Always make this operation look async.
   ScheduleTask(method_factory_.NewRunnableMethod(
       &MockAppCacheStorage::ProcessStoreGroupAndNewestCache,
-      group, group->newest_complete_cache(),
-      GetOrCreateDelegateReference(delegate)));
+      group, newest_cache, GetOrCreateDelegateReference(delegate)));
 }
 
 void MockAppCacheStorage::FindResponseForMainRequest(
@@ -179,8 +178,6 @@ void MockAppCacheStorage::ProcessStoreGroupAndNewestCache(
     scoped_refptr<AppCacheGroup> group,
     scoped_refptr<AppCache> newest_cache,
     scoped_refptr<DelegateReference> delegate_ref) {
-  DCHECK(group->newest_complete_cache() == newest_cache.get());
-
   if (simulate_store_group_and_newest_cache_failure_) {
     if (delegate_ref->delegate)
       delegate_ref->delegate->OnGroupAndNewestCacheStored(group, false);
@@ -188,7 +185,8 @@ void MockAppCacheStorage::ProcessStoreGroupAndNewestCache(
   }
 
   AddStoredGroup(group);
-  AddStoredCache(group->newest_complete_cache());
+  AddStoredCache(newest_cache);
+  group->AddCache(newest_cache);
 
   // Copy the collection prior to removal, on final release
   // of a cache the group's collection will change.
diff --git a/webkit/appcache/mock_appcache_storage.h b/webkit/appcache/mock_appcache_storage.h
index c29959a..9f352bd 100644
--- a/webkit/appcache/mock_appcache_storage.h
+++ b/webkit/appcache/mock_appcache_storage.h
@@ -30,7 +30,7 @@ class MockAppCacheStorage : public AppCacheStorage {
   virtual void LoadCache(int64 id, Delegate* delegate);
   virtual void LoadOrCreateGroup(const GURL& manifest_url, Delegate* delegate);
   virtual void StoreGroupAndNewestCache(
-      AppCacheGroup* group, Delegate* delegate);
+      AppCacheGroup* group, AppCache* newest_cache, Delegate* delegate);
   virtual void FindResponseForMainRequest(const GURL& url, Delegate* delegate);
   virtual void FindResponseForSubRequest(
       AppCache* cache, const GURL& url,
diff --git a/webkit/appcache/mock_appcache_storage_unittest.cc b/webkit/appcache/mock_appcache_storage_unittest.cc
index 911ec7c..404df50 100644
--- a/webkit/appcache/mock_appcache_storage_unittest.cc
+++ b/webkit/appcache/mock_appcache_storage_unittest.cc
@@ -210,7 +210,6 @@ TEST_F(MockAppCacheStorageTest, StoreNewGroup) {
   int64 cache_id = storage->NewCacheId();
   scoped_refptr<AppCache> cache = new AppCache(&service, cache_id);
   cache->set_complete(true);
-  group->AddCache(cache);
   // Hold a ref to the cache simulate the UpdateJob holding that ref,
   // and hold a ref to the group to simulate the CacheHost holding that ref.
 
@@ -218,7 +217,7 @@ TEST_F(MockAppCacheStorageTest, StoreNewGroup) {
   MockStorageDelegate delegate;
   EXPECT_TRUE(storage->stored_caches_.empty());
   EXPECT_TRUE(storage->stored_groups_.empty());
-  storage->StoreGroupAndNewestCache(group, &delegate);
+  storage->StoreGroupAndNewestCache(group, cache, &delegate);
   EXPECT_FALSE(delegate.stored_group_success_);
   EXPECT_TRUE(storage->stored_caches_.empty());
   EXPECT_TRUE(storage->stored_groups_.empty());
@@ -226,6 +225,7 @@ TEST_F(MockAppCacheStorageTest, StoreNewGroup) {
   EXPECT_TRUE(delegate.stored_group_success_);
   EXPECT_FALSE(storage->stored_caches_.empty());
   EXPECT_FALSE(storage->stored_groups_.empty());
+  EXPECT_EQ(cache, group->newest_complete_cache());
 }
 
 TEST_F(MockAppCacheStorageTest, StoreExistingGroup) {
@@ -248,8 +248,6 @@ TEST_F(MockAppCacheStorageTest, StoreExistingGroup) {
   int64 new_cache_id = storage->NewCacheId();
   scoped_refptr<AppCache> new_cache = new AppCache(&service, new_cache_id);
   new_cache->set_complete(true);
-  group->AddCache(new_cache);
-  EXPECT_EQ(new_cache.get(), group->newest_complete_cache());
   // Hold our refs to simulate the UpdateJob holding these refs.
 
   // Conduct the test.
@@ -258,7 +256,7 @@ TEST_F(MockAppCacheStorageTest, StoreExistingGroup) {
   EXPECT_EQ(size_t(1), storage->stored_groups_.size());
   EXPECT_TRUE(storage->IsCacheStored(old_cache));
   EXPECT_FALSE(storage->IsCacheStored(new_cache));
-  storage->StoreGroupAndNewestCache(group, &delegate);
+  storage->StoreGroupAndNewestCache(group, new_cache, &delegate);
   EXPECT_FALSE(delegate.stored_group_success_);
   EXPECT_EQ(size_t(1), storage->stored_caches_.size());
   EXPECT_EQ(size_t(1), storage->stored_groups_.size());
@@ -270,6 +268,7 @@ TEST_F(MockAppCacheStorageTest, StoreExistingGroup) {
   EXPECT_EQ(size_t(1), storage->stored_groups_.size());
   EXPECT_FALSE(storage->IsCacheStored(old_cache));
   EXPECT_TRUE(storage->IsCacheStored(new_cache));
+  EXPECT_EQ(new_cache.get(), group->newest_complete_cache());
 }
 
 TEST_F(MockAppCacheStorageTest, MakeGroupObsolete) {