Appcache update support for pending master entries:

 - Update process issues a URL request to fetch pending master entries.
 - Pending master entry fetch logic kept separate from regular url fetching as this will be the case in the long-term solution.
 - No optimizations to avoid issuing URL request if pending master entry is also listed in the manifest. (simpler)
 - Only optimized to prevent refetching something that has already been successfully fetched.

Long-term optimized solution should be to siphon the responses as the master resource is downloaded instead of having the update job issue URL requests.


TEST=new tests for update jobs with pending master entries
BUG=none

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

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

1 files changed, 327 insertions, 50 deletions

diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index 83190d0..8cd752f 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -10,7 +10,6 @@
 #include "net/base/io_buffer.h"
 #include "net/base/load_flags.h"
 #include "webkit/appcache/appcache_group.h"
-#include "webkit/appcache/appcache_host.h"
 
 namespace appcache {
 
@@ -25,6 +24,7 @@ class UpdateJobInfo :  public URLRequest::UserData {
   enum RequestType {
     MANIFEST_FETCH,
     URL_FETCH,
+    MASTER_ENTRY_FETCH,
     MANIFEST_REFETCH,
   };
 
@@ -123,6 +123,8 @@ AppCacheUpdateJob::~AppCacheUpdateJob() {
   DCHECK(!manifest_url_request_);
   DCHECK(pending_url_fetches_.empty());
   DCHECK(!inprogress_cache_);
+  DCHECK(pending_master_entries_.empty());
+  DCHECK(master_entry_fetches_.empty());
 
   if (group_)
     group_->SetUpdateStatus(AppCacheGroup::IDLE);
@@ -132,23 +134,40 @@ void AppCacheUpdateJob::StartUpdate(AppCacheHost* host,
                                     const GURL& new_master_resource) {
   DCHECK(group_->update_job() == this);
 
+  bool is_new_pending_master_entry = false;
   if (!new_master_resource.is_empty()) {
-    /* TODO(jennb): uncomment when processing master entries is implemented
+    DCHECK(new_master_resource == host->pending_master_entry_url());
+    DCHECK(!new_master_resource.has_ref());
+    DCHECK(new_master_resource.GetOrigin() == manifest_url_.GetOrigin());
+
+    // Cannot add more to this update if already terminating.
+    if (IsTerminating()) {
+      // TODO(jennb): requeue in group
+      return;
+    }
+
     std::pair<PendingMasters::iterator, bool> ret =
         pending_master_entries_.insert(
             PendingMasters::value_type(new_master_resource, PendingHosts()));
+    is_new_pending_master_entry = ret.second;
     ret.first->second.push_back(host);
-    */
+    host->AddObserver(this);
   }
 
   // Notify host (if any) if already checking or downloading.
-  appcache::AppCacheGroup::UpdateStatus update_status = group_->update_status();
+  AppCacheGroup::UpdateStatus update_status = group_->update_status();
   if (update_status == AppCacheGroup::CHECKING ||
       update_status == AppCacheGroup::DOWNLOADING) {
     if (host) {
       NotifySingleHost(host, CHECKING_EVENT);
       if (update_status == AppCacheGroup::DOWNLOADING)
         NotifySingleHost(host, DOWNLOADING_EVENT);
+
+      // Add to fetch list or an existing entry if already fetched.
+      if (!new_master_resource.is_empty()) {
+        AddMasterEntryToFetchList(host, new_master_resource,
+                                  is_new_pending_master_entry);
+      }
     }
     return;
   }
@@ -164,6 +183,11 @@ void AppCacheUpdateJob::StartUpdate(AppCacheHost* host,
     NotifySingleHost(host, CHECKING_EVENT);
   }
 
+  if (!new_master_resource.is_empty()) {
+    AddMasterEntryToFetchList(host, new_master_resource,
+                              is_new_pending_master_entry);
+  }
+
   FetchManifest(true);
 }
 
@@ -187,7 +211,8 @@ void AppCacheUpdateJob::OnResponseStarted(URLRequest *request) {
     // completion before reading any response data.
     UpdateJobInfo* info =
         static_cast<UpdateJobInfo*>(request->GetUserData(this));
-    if (info->type_ == UpdateJobInfo::URL_FETCH) {
+    if (info->type_ == UpdateJobInfo::URL_FETCH ||
+        info->type_ == UpdateJobInfo::MASTER_ENTRY_FETCH) {
       info->SetUpResponseWriter(
           service_->storage()->CreateResponseWriter(manifest_url_),
           this, request);
@@ -250,6 +275,7 @@ bool AppCacheUpdateJob::ConsumeResponseData(URLRequest* request,
       manifest_data_.append(info->buffer_->data(), bytes_read);
       break;
     case UpdateJobInfo::URL_FETCH:
+    case UpdateJobInfo::MASTER_ENTRY_FETCH:
       DCHECK(info->response_writer_.get());
       info->response_writer_->WriteData(info->buffer_, bytes_read,
                                         &info->write_callback_);
@@ -266,8 +292,6 @@ bool AppCacheUpdateJob::ConsumeResponseData(URLRequest* request,
 void AppCacheUpdateJob::OnWriteResponseComplete(int result,
                                                 URLRequest* request,
                                                 UpdateJobInfo* info) {
-  DCHECK(internal_state_ == DOWNLOADING);
-
   if (result < 0) {
     request->Cancel();
     OnResponseCompleted(request);
@@ -302,6 +326,9 @@ void AppCacheUpdateJob::OnResponseCompleted(URLRequest* request) {
     case UpdateJobInfo::URL_FETCH:
       HandleUrlFetchCompleted(request);
       break;
+    case UpdateJobInfo::MASTER_ENTRY_FETCH:
+      HandleMasterEntryFetchCompleted(request);
+      break;
     case UpdateJobInfo::MANIFEST_REFETCH:
       HandleManifestRefetchCompleted(request);
       break;
@@ -340,6 +367,10 @@ bool AppCacheUpdateJob::RetryRequest(URLRequest* request) {
       pending_url_fetches_.erase(url);
       pending_url_fetches_.insert(PendingUrlFetches::value_type(url, retry));
       break;
+    case UpdateJobInfo::MASTER_ENTRY_FETCH:
+      master_entry_fetches_.erase(url);
+      master_entry_fetches_.insert(PendingUrlFetches::value_type(url, retry));
+      break;
     default:
       NOTREACHED();
   }
@@ -354,21 +385,16 @@ void AppCacheUpdateJob::HandleManifestFetchCompleted(URLRequest* request) {
   DCHECK(internal_state_ == FETCH_MANIFEST);
   manifest_url_request_ = NULL;
 
-  if (!request->status().is_success()) {
-    LOG(INFO) << "Request non-success, status: " << request->status().status()
-        << " os_error: " << request->status().os_error();
-    internal_state_ = CACHE_FAILURE;
-    MaybeCompleteUpdate();  // if not done, run async cache failure steps
-    return;
-  }
-
-  int response_code = request->GetResponseCode();
+  int response_code = -1;
   std::string mime_type;
-  request->GetMimeType(&mime_type);
-  manifest_response_info_.reset(
-      new net::HttpResponseInfo(request->response_info()));
+  if (request->status().is_success()) {
+    response_code = request->GetResponseCode();
+    request->GetMimeType(&mime_type);
+  }
 
   if ((response_code / 100 == 2) && mime_type == kManifestMimeType) {
+    manifest_response_info_.reset(
+        new net::HttpResponseInfo(request->response_info()));
     if (update_type_ == UPGRADE_ATTEMPT)
       CheckIfManifestChanged();  // continues asynchronously
     else
@@ -378,15 +404,16 @@ void AppCacheUpdateJob::HandleManifestFetchCompleted(URLRequest* request) {
   } else if (response_code == 404 || response_code == 410) {
     service_->storage()->MakeGroupObsolete(group_, this);  // async
   } else {
-    LOG(INFO) << "Cache failure, response code: " << response_code;
     internal_state_ = CACHE_FAILURE;
+    CancelAllMasterEntryFetches();
     MaybeCompleteUpdate();  // if not done, run async cache failure steps
   }
 }
 
 void AppCacheUpdateJob::OnGroupMadeObsolete(AppCacheGroup* group,
                                             bool success) {
-  NotifyAllPendingMasterHosts(ERROR_EVENT);
+  DCHECK(master_entry_fetches_.empty());
+  CancelAllMasterEntryFetches();
   if (success) {
     DCHECK(group->is_obsolete());
     NotifyAllAssociatedHosts(OBSOLETE_EVENT);
@@ -404,6 +431,9 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
   if (!changed) {
     DCHECK(update_type_ == UPGRADE_ATTEMPT);
     internal_state_ = NO_UPDATE;
+
+    // Wait for pending master entries to download.
+    FetchMasterEntries();
     MaybeCompleteUpdate();  // if not done, run async 6.9.4 step 7 substeps
     return;
   }
@@ -413,6 +443,7 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
                      manifest_data_.length(), manifest)) {
     LOG(INFO) << "Failed to parse manifest: " << manifest_url_;
     internal_state_ = CACHE_FAILURE;
+    CancelAllMasterEntryFetches();
     MaybeCompleteUpdate();  // if not done, run async cache failure steps
     return;
   }
@@ -437,6 +468,7 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
   group_->SetUpdateStatus(AppCacheGroup::DOWNLOADING);
   NotifyAllAssociatedHosts(DOWNLOADING_EVENT);
   FetchUrls();
+  FetchMasterEntries();
   MaybeCompleteUpdate();  // if not done, continues when async fetches complete
 }
 
@@ -451,15 +483,14 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLRequest* request) {
       ? request->GetResponseCode() : -1;
   AppCacheEntry& entry = url_file_list_.find(url)->second;
 
-  UpdateJobInfo* info =
-      static_cast<UpdateJobInfo*>(request->GetUserData(this));
-
   if (request->status().is_success() && (response_code / 100 == 2)) {
     // Associate storage with the new entry.
+    UpdateJobInfo* info =
+        static_cast<UpdateJobInfo*>(request->GetUserData(this));
     DCHECK(info->response_writer_.get());
     entry.set_response_id(info->response_writer_->response_id());
 
-    inprogress_cache_->AddEntry(url, entry);
+    inprogress_cache_->AddOrModifyEntry(url, entry);
 
     // Foreign entries will be detected during cache selection.
     // Note: 6.9.4, step 17.9 possible optimization: if resource is HTML or XML
@@ -476,17 +507,8 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLRequest* request) {
 
     if (entry.IsExplicit() || entry.IsFallback()) {
       internal_state_ = CACHE_FAILURE;
-
-      // Cancel any pending URL requests.
-      for (PendingUrlFetches::iterator it = pending_url_fetches_.begin();
-           it != pending_url_fetches_.end(); ++it) {
-        delete it->second;
-      }
-
-      url_fetches_completed_ +=
-          pending_url_fetches_.size() + urls_to_fetch_.size();
-      pending_url_fetches_.clear();
-      urls_to_fetch_.clear();
+      CancelAllUrlFetches();
+      CancelAllMasterEntryFetches();
     } else if (response_code == 404 || response_code == 410) {
       // Entry is skipped.  They are dropped from the cache.
     } else if (update_type_ == UPGRADE_ATTEMPT) {
@@ -505,6 +527,83 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLRequest* request) {
   MaybeCompleteUpdate();
 }
 
+void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(URLRequest* request) {
+  DCHECK(internal_state_ == NO_UPDATE || internal_state_ == DOWNLOADING);
+
+  // TODO(jennb): Handle downloads completing during cache failure when update
+  // no longer fetches master entries directly. For now, we cancel all pending
+  // master entry fetches when entering cache failure state so this will never
+  // be called in CACHE_FAILURE state.
+
+  const GURL& url = request->original_url();
+  master_entry_fetches_.erase(url);
+  ++master_entries_completed_;
+
+  int response_code = request->status().is_success()
+      ? request->GetResponseCode() : -1;
+
+  PendingMasters::iterator found = pending_master_entries_.find(url);
+  DCHECK(found != pending_master_entries_.end());
+  PendingHosts& hosts = found->second;
+
+  // Section 6.9.4. No update case: step 7.3, else step 22.
+  if (response_code / 100 == 2) {
+    // Add fetched master entry to the appropriate cache.
+    UpdateJobInfo* info =
+        static_cast<UpdateJobInfo*>(request->GetUserData(this));
+    AppCache* cache = inprogress_cache_ ? inprogress_cache_.get() :
+        group_->newest_complete_cache();
+    DCHECK(info->response_writer_.get());
+    AppCacheEntry master_entry(AppCacheEntry::MASTER,
+                               info->response_writer_->response_id());
+    cache->AddOrModifyEntry(url, master_entry);
+
+    // In no-update case, associate host with the newest cache.
+    if (!inprogress_cache_) {
+      DCHECK(cache == group_->newest_complete_cache());
+      for (PendingHosts::iterator host_it = hosts.begin();
+           host_it != hosts.end(); ++host_it) {
+        (*host_it)->AssociateCache(cache);
+      }
+    }
+  } else {
+    HostNotifier host_notifier;
+    for (PendingHosts::iterator host_it = hosts.begin();
+         host_it != hosts.end(); ++host_it) {
+      AppCacheHost* host = *host_it;
+      host_notifier.AddHost(host);
+
+      // In downloading case, disassociate host from inprogress cache.
+      if (inprogress_cache_) {
+        host->AssociateCache(NULL);
+      }
+
+      host->RemoveObserver(this);
+    }
+    hosts.clear();
+    host_notifier.SendNotifications(ERROR_EVENT);
+
+    // In downloading case, update result is different if all master entries
+    // failed vs. only some failing.
+    if (inprogress_cache_) {
+      // Only count successful downloads to know if all master entries failed.
+      pending_master_entries_.erase(found);
+      --master_entries_completed_;
+
+      // Section 6.9.4, step 22.3.
+      if (update_type_ == CACHE_ATTEMPT && pending_master_entries_.empty()) {
+        CancelAllUrlFetches();
+        internal_state_ = CACHE_FAILURE;
+      }
+    }
+  }
+
+  if (internal_state_ != CACHE_FAILURE)
+    FetchMasterEntries();
+
+  MaybeCompleteUpdate();
+}
+
 void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
   DCHECK(internal_state_ == REFETCH_MANIFEST);
   manifest_url_request_ = NULL;
@@ -512,7 +611,7 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(URLRequest* request) {
   int response_code = request->status().is_success()
       ? request->GetResponseCode() : -1;
   if (response_code == 304 || manifest_data_ == manifest_refetch_data_) {
-    // Only need to store response in storage if manifest is not already an
+    // Only need to store response in storage if manifest is not already
     // an entry in the cache.
     AppCacheEntry* entry = inprogress_cache_->GetEntry(manifest_url_);
     if (entry) {
@@ -570,11 +669,6 @@ 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
-      NotifyAllAssociatedHosts(UPDATE_READY_EVENT);
-    internal_state_ = COMPLETED;
     MaybeCompleteUpdate();  // will definitely complete
   } else {
     protect_new_cache_ = NULL;
@@ -585,9 +679,9 @@ void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
 }
 
 void AppCacheUpdateJob::HandleManifestRefetchFailure() {
-    ScheduleUpdateRetry(kRerunDelayMs);
-    internal_state_ = CACHE_FAILURE;
-    MaybeCompleteUpdate();  // will definitely complete
+  ScheduleUpdateRetry(kRerunDelayMs);
+  internal_state_ = CACHE_FAILURE;
+  MaybeCompleteUpdate();  // will definitely complete
 }
 
 void AppCacheUpdateJob::NotifySingleHost(AppCacheHost* host,
@@ -638,6 +732,17 @@ void AppCacheUpdateJob::NotifyAllAssociatedHosts(EventID event_id) {
   host_notifier.SendNotifications(event_id);
 }
 
+void AppCacheUpdateJob::OnDestructionImminent(AppCacheHost* host) {
+  // The host is about to be deleted; remove from our collection.
+  PendingMasters::iterator found =
+      pending_master_entries_.find(host->pending_master_entry_url());
+  DCHECK(found != pending_master_entries_.end());
+  PendingHosts& hosts = found->second;
+  PendingHosts::iterator it = std::find(hosts.begin(), hosts.end(), host);
+  DCHECK(it != hosts.end());
+  hosts.erase(it);
+}
+
 void AppCacheUpdateJob::CheckIfManifestChanged() {
   DCHECK(update_type_ == UPGRADE_ATTEMPT);
   AppCacheEntry* entry =
@@ -724,6 +829,8 @@ void AppCacheUpdateJob::FetchUrls() {
     AppCacheEntry& entry = it->second;
     if (ShouldSkipUrlFetch(entry)) {
       ++url_fetches_completed_;
+    } else if (AlreadyFetchedEntry(url, entry.types())) {
+      ++url_fetches_completed_;  // saved a URL request
     } else if (!storage_checked && MaybeLoadFromNewestCache(url, entry)) {
       // Continues asynchronously after data is loaded from newest cache.
     } else {
@@ -739,6 +846,19 @@ void AppCacheUpdateJob::FetchUrls() {
   }
 }
 
+void AppCacheUpdateJob::CancelAllUrlFetches() {
+  // Cancel any pending URL requests.
+  for (PendingUrlFetches::iterator it = pending_url_fetches_.begin();
+       it != pending_url_fetches_.end(); ++it) {
+    delete it->second;
+  }
+
+  url_fetches_completed_ +=
+      pending_url_fetches_.size() + urls_to_fetch_.size();
+  pending_url_fetches_.clear();
+  urls_to_fetch_.clear();
+}
+
 bool AppCacheUpdateJob::ShouldSkipUrlFetch(const AppCacheEntry& entry) {
   if (entry.IsExplicit() || entry.IsFallback()) {
     return false;
@@ -748,6 +868,133 @@ bool AppCacheUpdateJob::ShouldSkipUrlFetch(const AppCacheEntry& entry) {
   return false;
 }
 
+bool AppCacheUpdateJob::AlreadyFetchedEntry(const GURL& url,
+                                            int entry_type) {
+  DCHECK(internal_state_ == DOWNLOADING || internal_state_ == NO_UPDATE);
+  AppCacheEntry* existing = inprogress_cache_ ?
+      inprogress_cache_->GetEntry(url) :
+      group_->newest_complete_cache()->GetEntry(url);
+  if (existing) {
+    existing->add_types(entry_type);
+    return true;
+  }
+  return false;
+}
+
+void AppCacheUpdateJob::AddMasterEntryToFetchList(AppCacheHost* host,
+                                                  const GURL& url,
+                                                  bool is_new) {
+  DCHECK(!IsTerminating());
+
+  if (internal_state_ == DOWNLOADING || internal_state_ == NO_UPDATE) {
+    AppCache* cache;
+    if (inprogress_cache_) {
+      host->AssociateCache(inprogress_cache_);  // always associate
+      cache = inprogress_cache_.get();
+    } else {
+      cache = group_->newest_complete_cache();
+    }
+
+    // Update existing entry if it has already been fetched.
+    AppCacheEntry* entry = cache->GetEntry(url);
+    if (entry) {
+      entry->add_types(AppCacheEntry::MASTER);
+      if (internal_state_ == NO_UPDATE)
+        host->AssociateCache(cache);  // only associate if have entry
+      if (is_new)
+        ++master_entries_completed_;  // pretend fetching completed
+      return;
+    }
+  }
+
+  // Add to fetch list if not already fetching.
+  if (master_entry_fetches_.find(url) == master_entry_fetches_.end()) {
+    master_entries_to_fetch_.insert(url);
+    if (internal_state_ == DOWNLOADING || internal_state_ == NO_UPDATE)
+      FetchMasterEntries();
+  }
+}
+
+void AppCacheUpdateJob::FetchMasterEntries() {
+  DCHECK(internal_state_ == NO_UPDATE || internal_state_ == DOWNLOADING);
+
+  // Fetch each master entry in the list, up to the concurrent limit.
+  // Additional fetches will be triggered as each fetch completes.
+  while (master_entry_fetches_.size() < kMaxConcurrentUrlFetches &&
+         !master_entries_to_fetch_.empty()) {
+    const GURL& url = *master_entries_to_fetch_.begin();
+
+    if (AlreadyFetchedEntry(url, AppCacheEntry::MASTER)) {
+      ++master_entries_completed_;  // saved a URL request
+
+      // In no update case, associate hosts to newest cache in group
+      // now that master entry has been "successfully downloaded".
+      if (internal_state_ == NO_UPDATE) {
+        DCHECK(!inprogress_cache_.get());
+        AppCache* cache = group_->newest_complete_cache();
+        PendingMasters::iterator found = pending_master_entries_.find(url);
+        DCHECK(found != pending_master_entries_.end());
+        PendingHosts& hosts = found->second;
+        for (PendingHosts::iterator host_it = hosts.begin();
+             host_it != hosts.end(); ++host_it) {
+          (*host_it)->AssociateCache(cache);
+        }
+      }
+    } else {
+      // Send URL request for the master entry.
+      URLRequest* request = new URLRequest(url, this);
+      request->SetUserData(this,
+          new UpdateJobInfo(UpdateJobInfo::MASTER_ENTRY_FETCH));
+      request->set_context(service_->request_context());
+      request->set_load_flags(
+          request->load_flags() | net::LOAD_DISABLE_INTERCEPT);
+      request->Start();
+      master_entry_fetches_.insert(PendingUrlFetches::value_type(url, request));
+    }
+
+    master_entries_to_fetch_.erase(master_entries_to_fetch_.begin());
+  }
+}
+
+void AppCacheUpdateJob::CancelAllMasterEntryFetches() {
+  // For now, cancel all in-progress fetches for master entries and pretend
+  // all master entries fetches have completed.
+  // TODO(jennb): Delete this when update no longer fetches master entries
+  // directly.
+
+  // Cancel all in-progress fetches.
+  for (PendingUrlFetches::iterator it = master_entry_fetches_.begin();
+       it != master_entry_fetches_.end(); ++it) {
+    delete it->second;
+    master_entries_to_fetch_.insert(it->first);  // back in unfetched list
+  }
+  master_entry_fetches_.clear();
+
+  master_entries_completed_ += master_entries_to_fetch_.size();
+
+  // Cache failure steps, step 2.
+  // Pretend all master entries that have not yet been fetched have completed
+  // downloading. Unassociate hosts from any appcache and send ERROR event.
+  HostNotifier host_notifier;
+  while (!master_entries_to_fetch_.empty()) {
+    const GURL& url = *master_entries_to_fetch_.begin();
+    PendingMasters::iterator found = pending_master_entries_.find(url);
+    DCHECK(found != pending_master_entries_.end());
+    PendingHosts& hosts = found->second;
+    for (PendingHosts::iterator host_it = hosts.begin();
+         host_it != hosts.end(); ++host_it) {
+      AppCacheHost* host = *host_it;
+      host->AssociateCache(NULL);
+      host_notifier.AddHost(host);
+      host->RemoveObserver(this);
+    }
+    hosts.clear();
+
+    master_entries_to_fetch_.erase(master_entries_to_fetch_.begin());
+  }
+  host_notifier.SendNotifications(ERROR_EVENT);
+}
+
 bool AppCacheUpdateJob::MaybeLoadFromNewestCache(const GURL& url,
                                                  AppCacheEntry& entry) {
   if (update_type_ != UPGRADE_ATTEMPT)
@@ -787,13 +1034,13 @@ void AppCacheUpdateJob::CopyEntryToCache(const GURL& url,
                                          AppCacheEntry* dest) {
   DCHECK(dest);
   dest->set_response_id(src.response_id());
-  inprogress_cache_->AddEntry(url, *dest);
+  inprogress_cache_->AddOrModifyEntry(url, *dest);
 }
 
 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() ) {
+      url_fetches_completed_ != url_file_list_.size()) {
     DCHECK(internal_state_ != COMPLETED);
     return;
   }
@@ -802,17 +1049,22 @@ void AppCacheUpdateJob::MaybeCompleteUpdate() {
     case NO_UPDATE:
       // 6.9.4 steps 7.3-7.7.
       NotifyAllAssociatedHosts(NO_UPDATE_EVENT);
-      pending_master_entries_.clear();
       internal_state_ = COMPLETED;
       break;
     case DOWNLOADING:
       internal_state_ = REFETCH_MANIFEST;
       FetchManifest(false);
       break;
+    case REFETCH_MANIFEST:
+      if (update_type_ == CACHE_ATTEMPT)
+        NotifyAllAssociatedHosts(CACHED_EVENT);
+      else
+        NotifyAllAssociatedHosts(UPDATE_READY_EVENT);
+      internal_state_ = COMPLETED;
+      break;
     case CACHE_FAILURE:
       // 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
@@ -847,8 +1099,15 @@ void AppCacheUpdateJob::Cancel() {
        it != pending_url_fetches_.end(); ++it) {
     delete it->second;
   }
+  pending_url_fetches_.clear();
 
-  pending_master_entries_.clear();
+  for (PendingUrlFetches::iterator it = master_entry_fetches_.begin();
+       it != master_entry_fetches_.end(); ++it) {
+    delete it->second;
+  }
+  master_entry_fetches_.clear();
+
+  ClearPendingMasterEntries();
   DiscardInprogressCache();
 
   // Delete response writer to avoid any callbacks.
@@ -858,10 +1117,27 @@ void AppCacheUpdateJob::Cancel() {
   service_->storage()->CancelDelegateCallbacks(this);
 }
 
+void AppCacheUpdateJob::ClearPendingMasterEntries() {
+  for (PendingMasters::iterator it = pending_master_entries_.begin();
+       it != pending_master_entries_.end(); ++it) {
+    PendingHosts& hosts = it->second;
+    for (PendingHosts::iterator host_it = hosts.begin();
+         host_it != hosts.end(); ++host_it) {
+      (*host_it)->RemoveObserver(this);
+    }
+  }
+
+  pending_master_entries_.clear();
+}
+
 void AppCacheUpdateJob::DiscardInprogressCache() {
   if (!inprogress_cache_)
     return;
 
+  AppCache::AppCacheHosts& hosts = inprogress_cache_->associated_hosts();
+  while (!hosts.empty())
+    (*hosts.begin())->AssociateCache(NULL);
+
   // TODO(jennb): Cleanup stored responses for entries in the cache?
   // May not be necessary if handled automatically by storage layer.
 
@@ -869,6 +1145,7 @@ void AppCacheUpdateJob::DiscardInprogressCache() {
 }
 
 void AppCacheUpdateJob::DeleteSoon() {
+  ClearPendingMasterEntries();
   manifest_response_writer_.reset();
   service_->storage()->CancelDelegateCallbacks(this);