Implement cancelling an appcache update. An update is cancelled when its application cache group is no longer in use. Refcounting of caches and groups changed to make cancelling an update work.

TEST=updated existing tests, verify deleting group cancels update, verify new refcounting behavior
BUG=none


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

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

1 files changed, 66 insertions, 46 deletions

diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index d9cf729..46048b5 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -88,12 +88,15 @@ AppCacheUpdateJob::AppCacheUpdateJob(AppCacheService* service,
 }
 
 AppCacheUpdateJob::~AppCacheUpdateJob() {
-  Cancel();
+  if (internal_state_ != COMPLETED)
+    Cancel();
 
   DCHECK(!manifest_url_request_);
   DCHECK(pending_url_fetches_.empty());
+  DCHECK(!inprogress_cache_);
 
-  group_->SetUpdateStatus(AppCacheGroup::IDLE);
+  if (group_)
+    group_->SetUpdateStatus(AppCacheGroup::IDLE);
 }
 
 void AppCacheUpdateJob::StartUpdate(AppCacheHost* host,
@@ -156,7 +159,7 @@ void AppCacheUpdateJob::OnResponseStarted(URLRequest *request) {
 }
 
 void AppCacheUpdateJob::ReadResponseData(URLRequest* request) {
-  if (internal_state_ == CACHE_FAILURE)
+  if (internal_state_ == CACHE_FAILURE || internal_state_ == CANCELLED)
     return;
 
   int bytes_read = 0;
@@ -311,14 +314,16 @@ void AppCacheUpdateJob::HandleManifestFetchCompleted(URLRequest* request) {
       ContinueHandleManifestFetchCompleted(true);
   } else if (response_code == 304 && update_type_ == UPGRADE_ATTEMPT) {
     ContinueHandleManifestFetchCompleted(false);
-  } else if (response_code == 404 || response_code == 410) {
-    group_->set_obsolete(true);
-    NotifyAllAssociatedHosts(OBSOLETE_EVENT);
-    NotifyAllPendingMasterHosts(ERROR_EVENT);
-    DeleteSoon();
   } else {
-    LOG(INFO) << "Cache failure, response code: " << response_code;
-    internal_state_ = CACHE_FAILURE;
+    if (response_code == 404 || response_code == 410) {
+      group_->set_obsolete(true);
+      NotifyAllAssociatedHosts(OBSOLETE_EVENT);
+      NotifyAllPendingMasterHosts(ERROR_EVENT);
+      internal_state_ = COMPLETED;
+    } else {
+      LOG(INFO) << "Cache failure, response code: " << response_code;
+      internal_state_ = CACHE_FAILURE;
+    }
     MaybeCompleteUpdate();  // if not done, run async cache failure steps
   }
 }
@@ -346,7 +351,6 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
   internal_state_ = DOWNLOADING;
   inprogress_cache_ = new AppCache(service_,
                                    service_->storage()->NewCacheId());
-  inprogress_cache_->set_owning_group(group_);
   BuildUrlFileList(manifest);
   inprogress_cache_->InitializeWithManifest(&manifest);
 
@@ -398,7 +402,6 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLRequest* request) {
       // Cancel any pending URL requests.
       for (PendingUrlFetches::iterator it = pending_url_fetches_.begin();
            it != pending_url_fetches_.end(); ++it) {
-        it->second->Cancel();
         delete it->second;
       }
 
@@ -436,21 +439,19 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
     inprogress_cache_->AddOrModifyEntry(manifest_url_, entry);
     inprogress_cache_->set_update_time(base::TimeTicks::Now());
 
-    // TODO(jennb): start of part to make async (cache storage may fail;
-    // group storage may fail)
+    // TODO(jennb): start of part to make async (cache/group storage may fail)
     inprogress_cache_->set_complete(true);
-
-    // TODO(jennb): write new cache to storage here
     group_->AddCache(inprogress_cache_);
-    // TODO(jennb): write new group to storage here
-    inprogress_cache_ = NULL;
+    protect_new_cache_.swap(inprogress_cache_);
+
+    // TODO(jennb): write new group and cache to storage here
 
     if (update_type_ == CACHE_ATTEMPT) {
       NotifyAllAssociatedHosts(CACHED_EVENT);
     } else {
       NotifyAllAssociatedHosts(UPDATE_READY_EVENT);
     }
-    DeleteSoon();
+    internal_state_ = COMPLETED;
     // TODO(jennb): end of part that needs to be made async.
   } else {
     LOG(INFO) << "Request status: " << request->status().status()
@@ -458,8 +459,9 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
         << " response code: " << response_code;
     ScheduleUpdateRetry(kRerunDelayMs);
     internal_state_ = CACHE_FAILURE;
-    HandleCacheFailure();
   }
+
+  MaybeCompleteUpdate();  // will definitely complete
 }
 
 void AppCacheUpdateJob::NotifySingleHost(AppCacheHost* host,
@@ -655,10 +657,12 @@ void AppCacheUpdateJob::CopyEntryToCache(const GURL& url,
   inprogress_cache_->AddEntry(url, *dest);
 }
 
-bool AppCacheUpdateJob::MaybeCompleteUpdate() {
+void AppCacheUpdateJob::MaybeCompleteUpdate() {
+  // Must wait for any pending master entries or url fetches to complete.
   if (master_entries_completed_ != pending_master_entries_.size() ||
       url_fetches_completed_ != url_file_list_.size() ) {
-    return false;
+    DCHECK(internal_state_ != COMPLETED);
+    return;
   }
 
   switch (internal_state_) {
@@ -666,37 +670,30 @@ bool AppCacheUpdateJob::MaybeCompleteUpdate() {
       // 6.9.4 steps 7.3-7.7.
       NotifyAllAssociatedHosts(NO_UPDATE_EVENT);
       pending_master_entries_.clear();
-      DeleteSoon();
+      internal_state_ = COMPLETED;
       break;
     case DOWNLOADING:
       internal_state_ = REFETCH_MANIFEST;
       FetchManifest(false);
-      return false;
+      break;
     case CACHE_FAILURE:
-      HandleCacheFailure();
+      // 6.9.4 cache failure steps 2-8.
+      NotifyAllAssociatedHosts(ERROR_EVENT);
+      pending_master_entries_.clear();
+      DiscardInprogressCache();
+      // For a CACHE_ATTEMPT, group will be discarded when the host(s) that
+      // started this update removes its reference to the group. Nothing more
+      // to do here.
+      internal_state_ = COMPLETED;
       break;
     default:
-      NOTREACHED();
-  }
-  return true;
-}
-
-void AppCacheUpdateJob::HandleCacheFailure() {
-  // 6.9.4 cache failure steps 2-8.
-  NotifyAllAssociatedHosts(ERROR_EVENT);
-  pending_master_entries_.clear();
-
-  // Discard the inprogress cache.
-  // TODO(jennb): cleanup possible storage for entries in the cache
-  if (inprogress_cache_) {
-    inprogress_cache_->set_owning_group(NULL);
-    inprogress_cache_ = NULL;
+      break;
   }
 
-  // For a CACHE_ATTEMPT, group will be discarded when this update
-  // job removes its reference to the group. Nothing more to do here.
-
-  DeleteSoon();
+  // Let the stack unwind before deletion to make it less risky as this
+  // method is called from multiple places in this file.
+  if (internal_state_ == COMPLETED)
+    DeleteSoon();
 }
 
 void AppCacheUpdateJob::ScheduleUpdateRetry(int delay_ms) {
@@ -706,16 +703,39 @@ void AppCacheUpdateJob::ScheduleUpdateRetry(int delay_ms) {
 }
 
 void AppCacheUpdateJob::Cancel() {
+  internal_state_ = CANCELLED;
+
   if (manifest_url_request_) {
     delete manifest_url_request_;
     manifest_url_request_ = NULL;
   }
 
-  // TODO(jennb): code other cancel cleanup (pending url requests, storage)
+  for (PendingUrlFetches::iterator it = pending_url_fetches_.begin();
+       it != pending_url_fetches_.end(); ++it) {
+    delete it->second;
+  }
+
+  pending_master_entries_.clear();
+  DiscardInprogressCache();
+  // TODO(jennb): cancel any storage callbacks
+}
+
+void AppCacheUpdateJob::DiscardInprogressCache() {
+  if (!inprogress_cache_)
+    return;
+
+  // TODO(jennb): cleanup stored responses for entries in the cache
+  inprogress_cache_ = NULL;
 }
 
 void AppCacheUpdateJob::DeleteSoon() {
-  // TODO(jennb): revisit if update should be deleting itself
+  // 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);
 }
+
 }  // namespace appcache