Fix for a big appcache storage bug. Now stores a record of the new master entry in the database even when there is no manifest update.

BUG=none
TEST=manual and unittest

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

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

9 files changed, 85 insertions, 50 deletions

diff --git a/webkit/appcache/appcache.h b/webkit/appcache/appcache.h
index d99393f..45e2238 100644
--- a/webkit/appcache/appcache.h
+++ b/webkit/appcache/appcache.h
@@ -69,9 +69,8 @@ class AppCache : public base::RefCounted<AppCache> {
     return false;
   }
 
-  void set_update_time(base::TimeTicks ticks) {
-    update_time_ = ticks;
-  }
+  base::TimeTicks update_time() const { return update_time_; }
+  void set_update_time(base::TimeTicks ticks) { update_time_ = ticks; }
 
   // Initializes the cache with information in the manifest.
   // Do not use the manifest after this call.
diff --git a/webkit/appcache/appcache_storage.h b/webkit/appcache/appcache_storage.h
index 8f10b78..cfa27a5 100644
--- a/webkit/appcache/appcache_storage.h
+++ b/webkit/appcache/appcache_storage.h
@@ -41,7 +41,7 @@ class AppCacheStorage {
 
     // If successfully stored 'success' will be true.
     virtual void OnGroupAndNewestCacheStored(
-        AppCacheGroup* group, bool success) {}
+        AppCacheGroup* group, AppCache* newest_cache, bool success) {}
 
     // If the operation fails, success will be false.
     virtual void OnGroupMadeObsolete(AppCacheGroup* group, bool success) {}
diff --git a/webkit/appcache/appcache_storage_impl.cc b/webkit/appcache/appcache_storage_impl.cc
index 50d2aef..ed893c1 100644
--- a/webkit/appcache/appcache_storage_impl.cc
+++ b/webkit/appcache/appcache_storage_impl.cc
@@ -422,7 +422,8 @@ void AppCacheStorageImpl::StoreGroupAndCacheTask::RunCompleted() {
     }
     group_->AddNewlyDeletableResponseIds(&newly_deletable_response_ids_);
   }
-  FOR_EACH_DELEGATE(delegates_, OnGroupAndNewestCacheStored(group_, success_));
+  FOR_EACH_DELEGATE(delegates_,
+                    OnGroupAndNewestCacheStored(group_, cache_, success_));
   group_ = NULL;
   cache_ = NULL;
 }
diff --git a/webkit/appcache/appcache_storage_impl_unittest.cc b/webkit/appcache/appcache_storage_impl_unittest.cc
index fac6319..01120a5 100644
--- a/webkit/appcache/appcache_storage_impl_unittest.cc
+++ b/webkit/appcache/appcache_storage_impl_unittest.cc
@@ -87,7 +87,8 @@ class AppCacheStorageImplTest : public testing::Test {
       test_->ScheduleNextTask();
     }
 
-    void OnGroupAndNewestCacheStored(AppCacheGroup* group, bool success) {
+    void OnGroupAndNewestCacheStored(
+        AppCacheGroup* group, AppCache* newest_cache, bool success) {
       stored_group_ = group;
       stored_group_success_ = success;
       test_->ScheduleNextTask();
diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index fa65f71..cab10ea 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -106,6 +106,7 @@ AppCacheUpdateJob::AppCacheUpdateJob(AppCacheService* service,
       master_entries_completed_(0),
       url_fetches_completed_(0),
       manifest_url_request_(NULL),
+      stored_state_(UNSTORED),
       ALLOW_THIS_IN_INITIALIZER_LIST(manifest_info_write_callback_(
           this, &AppCacheUpdateJob::OnManifestInfoWriteComplete)),
       ALLOW_THIS_IN_INITIALIZER_LIST(manifest_data_write_callback_(
@@ -613,6 +614,7 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(URLRequest* request) {
 
     // In no-update case, associate host with the newest cache.
     if (!inprogress_cache_) {
+      // TODO(michaeln): defer until the updated cache has been stored
       DCHECK(cache == group_->newest_complete_cache());
       for (PendingHosts::iterator host_it = hosts.begin();
            host_it != hosts.end(); ++host_it) {
@@ -627,9 +629,8 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(URLRequest* request) {
       host_notifier.AddHost(host);
 
       // In downloading case, disassociate host from inprogress cache.
-      if (inprogress_cache_) {
+      if (inprogress_cache_)
         host->AssociateCache(NULL);
-      }
 
       host->RemoveObserver(this);
     }
@@ -669,7 +670,7 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
     AppCacheEntry* entry = inprogress_cache_->GetEntry(manifest_url_);
     if (entry) {
       entry->add_types(AppCacheEntry::MANIFEST);
-      CompleteInprogressCache();
+      StoreGroupAndCache();
     } else {
       manifest_response_writer_.reset(
           service_->storage()->CreateResponseWriter(manifest_url_));
@@ -683,7 +684,9 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
     LOG(INFO) << "Request status: " << request->status().status()
         << " os_error: " << request->status().os_error()
         << " response code: " << response_code;
-    HandleManifestRefetchFailure();
+    ScheduleUpdateRetry(kRerunDelayMs);
+    internal_state_ = CACHE_FAILURE;
+    MaybeCompleteUpdate();  // will definitely complete
   }
 }
 
@@ -694,8 +697,8 @@ void AppCacheUpdateJob::OnManifestInfoWriteComplete(int result) {
     manifest_response_writer_->WriteData(io_buffer, manifest_data_.length(),
                                          &manifest_data_write_callback_);
   } else {
-    // Treat storage failure as if refetch of manifest failed.
-    HandleManifestRefetchFailure();
+    internal_state_ = CACHE_FAILURE;
+    MaybeCompleteUpdate();  // will definitely complete
   }
 }
 
@@ -706,36 +709,34 @@ void AppCacheUpdateJob::OnManifestDataWriteComplete(int result) {
         manifest_response_writer_->amount_written());
     if (!inprogress_cache_->AddOrModifyEntry(manifest_url_, entry))
       duplicate_response_ids_.push_back(entry.response_id());
-    CompleteInprogressCache();
+    StoreGroupAndCache();
   } else {
-    // Treat storage failure as if refetch of manifest failed.
-    HandleManifestRefetchFailure();
+    internal_state_ = CACHE_FAILURE;
+    MaybeCompleteUpdate();  // will definitely complete
   }
 }
 
-void AppCacheUpdateJob::CompleteInprogressCache() {
-  inprogress_cache_->set_update_time(base::TimeTicks::Now());
-  service_->storage()->StoreGroupAndNewestCache(group_, inprogress_cache_,
+void AppCacheUpdateJob::StoreGroupAndCache() {
+  DCHECK(stored_state_ == UNSTORED);
+  stored_state_ = STORING;
+  scoped_refptr<AppCache> newest_cache;
+  if (inprogress_cache_)
+    newest_cache.swap(inprogress_cache_);
+  else
+    newest_cache = group_->newest_complete_cache();
+  newest_cache->set_update_time(base::TimeTicks::Now());
+  service_->storage()->StoreGroupAndNewestCache(group_, newest_cache,
                                                 this);  // async
-  protect_new_cache_.swap(inprogress_cache_);
 }
 
 void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
+                                                    AppCache* newest_cache,
                                                     bool success) {
-  if (success) {
-    DCHECK_EQ(protect_new_cache_, group->newest_complete_cache());
-    MaybeCompleteUpdate();  // will definitely complete
-  } else {
-    protect_new_cache_ = NULL;
-
-    // Treat storage failure as if manifest refetch failed.
-    HandleManifestRefetchFailure();
-  }
-}
-
-void AppCacheUpdateJob::HandleManifestRefetchFailure() {
-  ScheduleUpdateRetry(kRerunDelayMs);
-  internal_state_ = CACHE_FAILURE;
+  DCHECK(stored_state_ == STORING);
+  if (success)
+    stored_state_ = STORED;
+  else
+    internal_state_ = CACHE_FAILURE;
   MaybeCompleteUpdate();  // will definitely complete
 }
 
@@ -985,6 +986,7 @@ void AppCacheUpdateJob::FetchMasterEntries() {
       // In no update case, associate hosts to newest cache in group
       // now that master entry has been "successfully downloaded".
       if (internal_state_ == NO_UPDATE) {
+        // TODO(michaeln): defer until the updated cache has been stored.
         DCHECK(!inprogress_cache_.get());
         AppCache* cache = group_->newest_complete_cache();
         PendingMasters::iterator found = pending_master_entries_.find(url);
@@ -1136,6 +1138,17 @@ void AppCacheUpdateJob::MaybeCompleteUpdate() {
 
   switch (internal_state_) {
     case NO_UPDATE:
+      if (master_entries_completed_ > 0) {
+        switch (stored_state_) {
+          case UNSTORED:
+            StoreGroupAndCache();
+            return;
+          case STORING:
+            return;
+          case STORED:
+            break;
+        }
+      }
       // 6.9.4 steps 7.3-7.7.
       NotifyAllAssociatedHosts(NO_UPDATE_EVENT);
       DiscardDuplicateResponses();
@@ -1146,6 +1159,7 @@ void AppCacheUpdateJob::MaybeCompleteUpdate() {
       FetchManifest(false);
       break;
     case REFETCH_MANIFEST:
+      DCHECK(stored_state_ == STORED);
       if (update_type_ == CACHE_ATTEMPT)
         NotifyAllAssociatedHosts(CACHED_EVENT);
       else
@@ -1246,7 +1260,6 @@ void AppCacheUpdateJob::DeleteSoon() {
   // Break the connection with the group so the group cannot call delete
   // on this object after we've posted a task to delete ourselves.
   group_->SetUpdateStatus(AppCacheGroup::IDLE);
-  protect_new_cache_ = NULL;
   group_ = NULL;
 
   MessageLoop::current()->DeleteSoon(FROM_HERE, this);
diff --git a/webkit/appcache/appcache_update_job.h b/webkit/appcache/appcache_update_job.h
index 1eec1a4..2580ff8 100644
--- a/webkit/appcache/appcache_update_job.h
+++ b/webkit/appcache/appcache_update_job.h
@@ -44,7 +44,6 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
 
   // Master entries have multiple hosts, for example, the same page is opened
   // in different tabs.
-  // TODO(jennb): detect when hosts are deleted
   typedef std::vector<AppCacheHost*> PendingHosts;
   typedef std::map<GURL, PendingHosts> PendingMasters;
   typedef std::map<GURL, URLRequest*> PendingUrlFetches;
@@ -53,6 +52,10 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
 
   static const int kRerunDelayMs = 1000;
 
+  // TODO(michaeln): Rework the set of states vs update types vs stored states.
+  // The NO_UPDATE state is really more of an update type. For all update types
+  // storing the results is relevant.
+
   enum UpdateType {
     UNKNOWN_TYPE,
     UPGRADE_ATTEMPT,
@@ -71,6 +74,12 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
     COMPLETED,
   };
 
+  enum StoredState {
+    UNSTORED,
+    STORING,
+    STORED,
+  };
+
   // Methods for URLRequest::Delegate.
   void OnResponseStarted(URLRequest* request);
   void OnReadCompleted(URLRequest* request, int bytes_read);
@@ -82,7 +91,8 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   // Methods for AppCacheStorage::Delegate.
   void OnResponseInfoLoaded(AppCacheResponseInfo* response_info,
                             int64 response_id);
-  void OnGroupAndNewestCacheStored(AppCacheGroup* group, bool success);
+  void OnGroupAndNewestCacheStored(AppCacheGroup* group, AppCache* newest_cache,
+                                   bool success);
   void OnGroupMadeObsolete(AppCacheGroup* group, bool success);
 
   // Methods for AppCacheHost::Observer.
@@ -122,8 +132,8 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   void HandleManifestRefetchCompleted(URLRequest* request);
   void OnManifestInfoWriteComplete(int result);
   void OnManifestDataWriteComplete(int result);
-  void CompleteInprogressCache();
-  void HandleManifestRefetchFailure();
+
+  void StoreGroupAndCache();
 
   void NotifySingleHost(AppCacheHost* host, EventID event_id);
   void NotifyAllPendingMasterHosts(EventID event_id);
@@ -179,7 +189,8 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   // Deletes this object after letting the stack unwind.
   void DeleteSoon();
 
-  bool IsTerminating() { return internal_state_ >= REFETCH_MANIFEST; }
+  bool IsTerminating() { return internal_state_ >= REFETCH_MANIFEST ||
+                                stored_state_ != UNSTORED; }
 
   // This factory will be used to schedule invocations of various methods.
   ScopedRunnableMethodFactory<AppCacheUpdateJob> method_factory_;
@@ -189,12 +200,6 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
 
   scoped_refptr<AppCache> inprogress_cache_;
 
-  // Hold a reference to the newly created cache until this update is
-  // destroyed. The host that started the update will add a reference to the
-  // new cache when notified that the update is complete. AppCacheGroup sends
-  // the notification when its status is set to IDLE in ~AppCacheUpdateJob.
-  scoped_refptr<AppCache> protect_new_cache_;
-
   AppCacheGroup* group_;
 
   UpdateType update_type_;
@@ -248,6 +253,9 @@ class AppCacheUpdateJob : public URLRequest::Delegate,
   // TODO(michaeln): Rework when we no longer fetches master entries directly.
   std::vector<int64> duplicate_response_ids_;
 
+  // Whether we've stored the resulting group/cache yet.
+  StoredState stored_state_;
+
   net::CompletionCallbackImpl<AppCacheUpdateJob> manifest_info_write_callback_;
   net::CompletionCallbackImpl<AppCacheUpdateJob> manifest_data_write_callback_;
   net::CompletionCallbackImpl<AppCacheUpdateJob> manifest_data_read_callback_;
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index b789710..9285689 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -282,6 +282,7 @@ class AppCacheUpdateJobTest : public testing::Test,
         expect_group_has_cache_(false),
         expect_old_cache_(NULL),
         expect_newest_cache_(NULL),
+        expect_non_null_update_time_(false),
         tested_manifest_(NONE),
         registered_factory_(false),
         old_factory_(NULL) {
@@ -1864,6 +1865,10 @@ class AppCacheUpdateJobTest : public testing::Test,
     cache->AddEntry(http_server_->TestServerPage("files/explicit2"),
                     AppCacheEntry(AppCacheEntry::EXPLICIT, 222));
 
+    // Reset the update time to null so we can verify it gets
+    // modified in this test case by the UpdateJob.
+    cache->set_update_time(base::TimeTicks());
+
     MockFrontend* frontend2 = MakeMockFrontend();
     AppCacheHost* host2 = MakeHost(2, frontend2);
     host2->new_master_entry_url_ =
@@ -1880,6 +1885,7 @@ class AppCacheUpdateJobTest : public testing::Test,
     expect_group_obsolete_ = false;
     expect_group_has_cache_ = true;
     expect_newest_cache_ = cache;  // newest cache still the same cache
+    expect_non_null_update_time_ = true;
     tested_manifest_ = PENDING_MASTER_NO_UPDATE;
     MockFrontend::HostIds ids1(1, host1->host_id());
     frontend1->AddExpectedEvent(ids1, CHECKING_EVENT);
@@ -2510,6 +2516,10 @@ class AppCacheUpdateJobTest : public testing::Test,
 
     if (expect_group_has_cache_) {
       EXPECT_TRUE(group_->newest_complete_cache() != NULL);
+
+      if (expect_non_null_update_time_)
+        EXPECT_TRUE(!group_->newest_complete_cache()->update_time().is_null());
+
       if (expect_old_cache_) {
         EXPECT_NE(expect_old_cache_, group_->newest_complete_cache());
         EXPECT_TRUE(group_->old_caches().end() !=
@@ -2776,6 +2786,7 @@ class AppCacheUpdateJobTest : public testing::Test,
   bool expect_group_has_cache_;
   AppCache* expect_old_cache_;
   AppCache* expect_newest_cache_;
+  bool expect_non_null_update_time_;
   std::vector<MockFrontend*> frontends_;  // to check expected events
   TestedManifest tested_manifest_;
   AppCache::EntryMap expect_extra_entries_;
diff --git a/webkit/appcache/mock_appcache_storage.cc b/webkit/appcache/mock_appcache_storage.cc
index a279ea0..abdc250 100644
--- a/webkit/appcache/mock_appcache_storage.cc
+++ b/webkit/appcache/mock_appcache_storage.cc
@@ -186,9 +186,10 @@ void MockAppCacheStorage::ProcessStoreGroupAndNewestCache(
     scoped_refptr<AppCacheGroup> group,
     scoped_refptr<AppCache> newest_cache,
     scoped_refptr<DelegateReference> delegate_ref) {
+  Delegate* delegate = delegate_ref->delegate;
   if (simulate_store_group_and_newest_cache_failure_) {
-    if (delegate_ref->delegate)
-      delegate_ref->delegate->OnGroupAndNewestCacheStored(group, false);
+    if (delegate)
+      delegate->OnGroupAndNewestCacheStored(group, newest_cache, false);
     return;
   }
 
@@ -204,8 +205,8 @@ void MockAppCacheStorage::ProcessStoreGroupAndNewestCache(
     RemoveStoredCaches(copy);
   }
 
-  if (delegate_ref->delegate)
-    delegate_ref->delegate->OnGroupAndNewestCacheStored(group, true);
+  if (delegate)
+    delegate->OnGroupAndNewestCacheStored(group, newest_cache, true);
 }
 
 namespace {
diff --git a/webkit/appcache/mock_appcache_storage_unittest.cc b/webkit/appcache/mock_appcache_storage_unittest.cc
index 77e6558..a2ed818 100644
--- a/webkit/appcache/mock_appcache_storage_unittest.cc
+++ b/webkit/appcache/mock_appcache_storage_unittest.cc
@@ -31,7 +31,8 @@ class MockAppCacheStorageTest : public testing::Test {
       loaded_manifest_url_ = manifest_url;
     }
 
-    void OnGroupAndNewestCacheStored(AppCacheGroup* group, bool success) {
+    void OnGroupAndNewestCacheStored(
+        AppCacheGroup* group, AppCache* newest_cache, bool success) {
       stored_group_ = group;
       stored_group_success_ = success;
     }