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

11 files changed, 302 insertions, 139 deletions

diff --git a/webkit/appcache/appcache.cc b/webkit/appcache/appcache.cc
index 01bf37d..41c3f54 100644
--- a/webkit/appcache/appcache.cc
+++ b/webkit/appcache/appcache.cc
@@ -24,16 +24,16 @@ AppCache::AppCache(AppCacheService *service, int64 cache_id)
 
 AppCache::~AppCache() {
   DCHECK(associated_hosts_.empty());
+  if (owning_group_) {
+    DCHECK(is_complete_);
+    owning_group_->RemoveCache(this);
+  }
   DCHECK(!owning_group_);
   service_->storage()->working_set()->RemoveCache(this);
 }
 
 void AppCache::UnassociateHost(AppCacheHost* host) {
   associated_hosts_.erase(host);
-
-  // Inform group if this cache is no longer in use.
-  if (associated_hosts_.empty() && owning_group_)
-    owning_group_->RemoveCache(this);
 }
 
 void AppCache::AddEntry(const GURL& url, const AppCacheEntry& entry) {
diff --git a/webkit/appcache/appcache.h b/webkit/appcache/appcache.h
index c80cafd..203b582 100644
--- a/webkit/appcache/appcache.h
+++ b/webkit/appcache/appcache.h
@@ -29,6 +29,7 @@ class AppCacheService;
 class AppCache : public base::RefCounted<AppCache> {
  public:
   typedef std::map<GURL, AppCacheEntry> EntryMap;
+  typedef std::set<AppCacheHost*> AppCacheHosts;
 
   AppCache(AppCacheService *service, int64 cache_id);
   ~AppCache();
@@ -36,7 +37,6 @@ class AppCache : public base::RefCounted<AppCache> {
   int64 cache_id() const { return cache_id_; }
 
   AppCacheGroup* owning_group() const { return owning_group_; }
-  void set_owning_group(AppCacheGroup* group) { owning_group_ = group; }
 
   bool is_complete() const { return is_complete_; }
   void set_complete(bool value) { is_complete_ = value; }
@@ -55,9 +55,7 @@ class AppCache : public base::RefCounted<AppCache> {
 
   const EntryMap& entries() const { return entries_; }
 
-  const std::set<AppCacheHost*>& associated_hosts() const {
-    return associated_hosts_;
-  }
+  AppCacheHosts& associated_hosts() { return associated_hosts_; }
 
   bool IsNewerThan(AppCache* cache) const {
     if (update_time_ > cache->update_time_)
@@ -79,9 +77,13 @@ class AppCache : public base::RefCounted<AppCache> {
   void InitializeWithManifest(Manifest* manifest);
 
  private:
+  friend class AppCacheGroup;
   friend class AppCacheHost;
   friend class AppCacheUpdateJobTest;
 
+  // Use AppCacheGroup::Add/RemoveCache() to manipulate owning group.
+  void set_owning_group(AppCacheGroup* group) { owning_group_ = group; }
+
   // Use AppCacheHost::AssociateCache() to manipulate host association.
   void AssociateHost(AppCacheHost* host) {
     associated_hosts_.insert(host);
@@ -89,8 +91,8 @@ class AppCache : public base::RefCounted<AppCache> {
   void UnassociateHost(AppCacheHost* host);
 
   const int64 cache_id_;
-  AppCacheGroup* owning_group_;
-  std::set<AppCacheHost*> associated_hosts_;
+  scoped_refptr<AppCacheGroup> owning_group_;
+  AppCacheHosts associated_hosts_;
 
   EntryMap entries_;    // contains entries of all types
 
diff --git a/webkit/appcache/appcache_group.cc b/webkit/appcache/appcache_group.cc
index b45e90b..8d7571c 100644
--- a/webkit/appcache/appcache_group.cc
+++ b/webkit/appcache/appcache_group.cc
@@ -28,26 +28,27 @@ AppCacheGroup::AppCacheGroup(AppCacheService* service,
 
 AppCacheGroup::~AppCacheGroup() {
   DCHECK(old_caches_.empty());
-  DCHECK(!update_job_);
+  DCHECK(!newest_complete_cache_);
 
-  // Newest complete cache might never have been associated with a host
-  // and thus would not be cleaned up by the backend impl during shutdown.
-  if (newest_complete_cache_)
-    RemoveCache(newest_complete_cache_);
+  if (update_job_)
+    delete update_job_;
+  DCHECK_EQ(IDLE, update_status_);
 
   service_->storage()->working_set()->RemoveGroup(this);
 }
 
-void AppCacheGroup::AddObserver(Observer* observer) {
+void AppCacheGroup::AddUpdateObserver(UpdateObserver* observer) {
   observers_.AddObserver(observer);
 }
 
-void AppCacheGroup::RemoveObserver(Observer* observer) {
+void AppCacheGroup::RemoveUpdateObserver(UpdateObserver* observer) {
   observers_.RemoveObserver(observer);
 }
 
 void AppCacheGroup::AddCache(AppCache* complete_cache) {
   DCHECK(complete_cache->is_complete());
+  complete_cache->set_owning_group(this);
+
   if (!newest_complete_cache_) {
     newest_complete_cache_ = complete_cache;
     return;
@@ -56,41 +57,44 @@ void AppCacheGroup::AddCache(AppCache* complete_cache) {
   if (complete_cache->IsNewerThan(newest_complete_cache_)) {
     old_caches_.push_back(newest_complete_cache_);
     newest_complete_cache_ = complete_cache;
+
+    // Update hosts of older caches to add a reference to the newest cache.
+    for (Caches::iterator it = old_caches_.begin();
+         it != old_caches_.end(); ++it) {
+      AppCache::AppCacheHosts& hosts = (*it)->associated_hosts();
+      for (AppCache::AppCacheHosts::iterator host_it = hosts.begin();
+           host_it != hosts.end(); ++host_it) {
+        (*host_it)->SetSwappableCache(this);
+      }
+    }
   } else {
     old_caches_.push_back(complete_cache);
   }
 }
 
-bool AppCacheGroup::RemoveCache(AppCache* cache) {
+void AppCacheGroup::RemoveCache(AppCache* cache) {
+  DCHECK(cache->associated_hosts().empty());
   if (cache == newest_complete_cache_) {
-    // Cannot remove newest cache if there are older caches as those may
-    // eventually be swapped to the newest cache.
-    if (!old_caches_.empty())
-      return false;
-
-    newest_complete_cache_->set_owning_group(NULL);
+    AppCache* cache = newest_complete_cache_;
     newest_complete_cache_ = NULL;
+    cache->set_owning_group(NULL);  // may cause this group to be deleted
   } else {
-    // Unused old cache can always be removed.
     Caches::iterator it =
         std::find(old_caches_.begin(), old_caches_.end(), cache);
     if (it != old_caches_.end()) {
-      (*it)->set_owning_group(NULL);
+      AppCache* cache = *it;
       old_caches_.erase(it);
+      cache->set_owning_group(NULL);  // may cause group to be deleted
     }
   }
-
-  return true;
 }
 
 void AppCacheGroup::StartUpdateWithNewMasterEntry(
     AppCacheHost* host, const GURL& new_master_resource) {
-  /* TODO(jennb): enable after have logic for cancelling an update
   if (!update_job_)
     update_job_ = new AppCacheUpdateJob(service_, this);
 
   update_job_->StartUpdate(host, new_master_resource);
-  */
 }
 
 void AppCacheGroup::SetUpdateStatus(UpdateStatus status) {
@@ -103,7 +107,7 @@ void AppCacheGroup::SetUpdateStatus(UpdateStatus status) {
     DCHECK(update_job_);
   } else {
     update_job_ = NULL;
-    FOR_EACH_OBSERVER(Observer, observers_, OnUpdateComplete(this));
+    FOR_EACH_OBSERVER(UpdateObserver, observers_, OnUpdateComplete(this));
   }
 }
 
diff --git a/webkit/appcache/appcache_group.h b/webkit/appcache/appcache_group.h
index 4230e8d..a5ff5fb 100644
--- a/webkit/appcache/appcache_group.h
+++ b/webkit/appcache/appcache_group.h
@@ -24,11 +24,11 @@ class AppCacheUpdateJob;
 class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
  public:
 
-  class Observer {
+  class UpdateObserver {
     public:
       // Called just after an appcache update has completed.
       virtual void OnUpdateComplete(AppCacheGroup* group) = 0;
-      virtual ~Observer() { }
+      virtual ~UpdateObserver() { }
   };
 
   enum UpdateStatus {
@@ -40,10 +40,10 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   AppCacheGroup(AppCacheService* service, const GURL& manifest_url);
   ~AppCacheGroup();
 
-  // Adds/removes an observer, the AppCacheGroup does not take
+  // Adds/removes an update observer, the AppCacheGroup does not take
   // ownership of the observer.
-  void AddObserver(Observer* observer);
-  void RemoveObserver(Observer* observer);
+  void AddUpdateObserver(UpdateObserver* observer);
+  void RemoveUpdateObserver(UpdateObserver* observer);
 
   const GURL& manifest_url() { return manifest_url_; }
 
@@ -54,11 +54,9 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
 
   void AddCache(AppCache* complete_cache);
 
-  // Returns false if cache cannot be removed. The newest complete cache
-  // cannot be removed as long as the group is still in use.
-  bool RemoveCache(AppCache* cache);
+  void RemoveCache(AppCache* cache);
 
-  bool HasCache() { return newest_complete_cache_ || !old_caches_.empty(); }
+  bool HasCache() { return newest_complete_cache_ != NULL; }
 
   UpdateStatus update_status() { return update_status_; }
 
@@ -82,7 +80,7 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   friend class AppCacheUpdateJob;
   friend class AppCacheUpdateJobTest;
 
-  typedef std::vector<scoped_refptr<AppCache> > Caches;
+  typedef std::vector<AppCache*> Caches;
 
   AppCacheUpdateJob* update_job() { return update_job_; }
   void SetUpdateStatus(UpdateStatus status);
@@ -97,7 +95,7 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   Caches old_caches_;
 
   // Newest cache in this group to be complete, aka relevant cache.
-  scoped_refptr<AppCache> newest_complete_cache_;
+  AppCache* newest_complete_cache_;
 
   // Current update job for this group, if any.
   AppCacheUpdateJob* update_job_;
@@ -106,9 +104,10 @@ class AppCacheGroup : public base::RefCounted<AppCacheGroup> {
   AppCacheService* service_;
 
   // List of objects observing this group.
-  ObserverList<Observer> observers_;
+  ObserverList<UpdateObserver> observers_;
 
   FRIEND_TEST(AppCacheGroupTest, StartUpdate);
+  FRIEND_TEST(AppCacheGroupTest, CancelUpdate);
   FRIEND_TEST(AppCacheUpdateJobTest, AlreadyChecking);
   FRIEND_TEST(AppCacheUpdateJobTest, AlreadyDownloading);
   DISALLOW_COPY_AND_ASSIGN(AppCacheGroup);
diff --git a/webkit/appcache/appcache_group_unittest.cc b/webkit/appcache/appcache_group_unittest.cc
index e9b7acd..aca8552 100644
--- a/webkit/appcache/appcache_group_unittest.cc
+++ b/webkit/appcache/appcache_group_unittest.cc
@@ -42,6 +42,20 @@ class TestAppCacheFrontend : public appcache::AppCacheFrontend {
 
 namespace appcache {
 
+class TestUpdateObserver : public AppCacheGroup::UpdateObserver {
+ public:
+  TestUpdateObserver() : update_completed_(false), group_has_cache_(false) {
+  }
+
+  virtual void OnUpdateComplete(AppCacheGroup* group) {
+    update_completed_ = true;
+    group_has_cache_ = group->HasCache();
+  }
+
+  bool update_completed_;
+  bool group_has_cache_;
+};
+
 class AppCacheGroupTest : public testing::Test {
 };
 
@@ -52,61 +66,66 @@ TEST(AppCacheGroupTest, AddRemoveCache) {
 
   base::TimeTicks ticks = base::TimeTicks::Now();
 
-  AppCache* cache1 = new AppCache(&service, 111);
+  scoped_refptr<AppCache> cache1 = new AppCache(&service, 111);
   cache1->set_complete(true);
   cache1->set_update_time(ticks);
-  cache1->set_owning_group(group);
   group->AddCache(cache1);
   EXPECT_EQ(cache1, group->newest_complete_cache());
 
   // Adding older cache does not change newest complete cache.
-  AppCache* cache2 = new AppCache(&service, 222);
+  scoped_refptr<AppCache> cache2 = new AppCache(&service, 222);
   cache2->set_complete(true);
   cache2->set_update_time(ticks - base::TimeDelta::FromDays(1));
-  cache2->set_owning_group(group);
   group->AddCache(cache2);
   EXPECT_EQ(cache1, group->newest_complete_cache());
 
   // Adding newer cache does change newest complete cache.
-  AppCache* cache3 = new AppCache(&service, 333);
+  scoped_refptr<AppCache> cache3 = new AppCache(&service, 333);
   cache3->set_complete(true);
   cache3->set_update_time(ticks + base::TimeDelta::FromDays(1));
-  cache3->set_owning_group(group);
   group->AddCache(cache3);
   EXPECT_EQ(cache3, group->newest_complete_cache());
 
   // Adding cache with same update time uses one with larger ID.
-  AppCache* cache4 = new AppCache(&service, 444);
+  scoped_refptr<AppCache> cache4 = new AppCache(&service, 444);
   cache4->set_complete(true);
-  cache4->set_update_time(ticks + base::TimeDelta::FromDays(1)); // same as 3
-  cache4->set_owning_group(group);
+  cache4->set_update_time(ticks + base::TimeDelta::FromDays(1));  // same as 3
   group->AddCache(cache4);
   EXPECT_EQ(cache4, group->newest_complete_cache());
 
-  AppCache* cache5 = new AppCache(&service, 55);  // smaller id
+  scoped_refptr<AppCache> cache5 = new AppCache(&service, 55);  // smaller id
   cache5->set_complete(true);
-  cache5->set_update_time(ticks + base::TimeDelta::FromDays(1)); // same as 4
-  cache5->set_owning_group(group);
+  cache5->set_update_time(ticks + base::TimeDelta::FromDays(1));  // same as 4
   group->AddCache(cache5);
   EXPECT_EQ(cache4, group->newest_complete_cache());  // no change
 
   // Old caches can always be removed.
-  EXPECT_TRUE(group->RemoveCache(cache1));
-  EXPECT_EQ(cache4, group->newest_complete_cache());  // newest unchanged
-
-  // Cannot remove newest cache if there are older caches.
-  EXPECT_FALSE(group->RemoveCache(cache4));
+  group->RemoveCache(cache1);
+  EXPECT_FALSE(cache1->owning_group());
   EXPECT_EQ(cache4, group->newest_complete_cache());  // newest unchanged
 
-  // Can remove newest cache after all older caches are removed.
-  EXPECT_TRUE(group->RemoveCache(cache2));
+  // Remove rest of caches.
+  group->RemoveCache(cache2);
+  EXPECT_FALSE(cache2->owning_group());
   EXPECT_EQ(cache4, group->newest_complete_cache());  // newest unchanged
-  EXPECT_TRUE(group->RemoveCache(cache3));
+  group->RemoveCache(cache3);
+  EXPECT_FALSE(cache3->owning_group());
   EXPECT_EQ(cache4, group->newest_complete_cache());  // newest unchanged
-  EXPECT_TRUE(group->RemoveCache(cache5));
+  group->RemoveCache(cache5);
+  EXPECT_FALSE(cache5->owning_group());
   EXPECT_EQ(cache4, group->newest_complete_cache());  // newest unchanged
-  EXPECT_TRUE(group->RemoveCache(cache4));            // newest removed
+  group->RemoveCache(cache4);  // newest removed
+  EXPECT_FALSE(cache4->owning_group());
   EXPECT_FALSE(group->newest_complete_cache());       // no more newest cache
+
+  // Can remove newest cache if there are older caches.
+  group->AddCache(cache1);
+  EXPECT_EQ(cache1, group->newest_complete_cache());
+  group->AddCache(cache4);
+  EXPECT_EQ(cache4, group->newest_complete_cache());
+  group->RemoveCache(cache4);  // remove newest
+  EXPECT_FALSE(cache4->owning_group());
+  EXPECT_FALSE(group->newest_complete_cache());  // newest removed
 }
 
 TEST(AppCacheGroupTest, CleanupUnusedGroup) {
@@ -122,7 +141,6 @@ TEST(AppCacheGroupTest, CleanupUnusedGroup) {
   AppCache* cache1 = new AppCache(&service, 111);
   cache1->set_complete(true);
   cache1->set_update_time(ticks);
-  cache1->set_owning_group(group);
   group->AddCache(cache1);
   EXPECT_EQ(cache1, group->newest_complete_cache());
 
@@ -139,7 +157,6 @@ TEST(AppCacheGroupTest, CleanupUnusedGroup) {
   AppCache* cache2 = new AppCache(&service, 222);
   cache2->set_complete(true);
   cache2->set_update_time(ticks + base::TimeDelta::FromDays(1));
-  cache2->set_owning_group(group);
   group->AddCache(cache2);
   EXPECT_EQ(cache2, group->newest_complete_cache());
 
@@ -152,7 +169,6 @@ TEST(AppCacheGroupTest, CleanupUnusedGroup) {
 }
 
 TEST(AppCacheGroupTest, StartUpdate) {
-  /* TODO(jennb) - uncomment after AppCacheGroup::StartUpdate does something.
   MockAppCacheService service;
   scoped_refptr<AppCacheGroup> group =
       new AppCacheGroup(&service, GURL("http://foo.com"));
@@ -167,11 +183,29 @@ TEST(AppCacheGroupTest, StartUpdate) {
   group->StartUpdateWithHost(NULL);
   EXPECT_EQ(update, group->update_job_);
 
-  // Remove update job's reference to this group.
+  // Deleting the update should restore the group to IDLE.
   delete update;
   EXPECT_TRUE(group->update_job_ == NULL);
   EXPECT_EQ(AppCacheGroup::IDLE, group->update_status());
-  */
+}
+
+TEST(AppCacheGroupTest, CancelUpdate) {
+  MockAppCacheService service;
+  scoped_refptr<AppCacheGroup> group =
+      new AppCacheGroup(&service, GURL("http://foo.com"));
+
+  // Set state to checking to prevent update job from executing fetches.
+  group->update_status_ = AppCacheGroup::CHECKING;
+  group->StartUpdate();
+  AppCacheUpdateJob* update = group->update_job_;
+  EXPECT_TRUE(update != NULL);
+
+  // Deleting the group should cancel the update.
+  TestUpdateObserver observer;
+  group->AddUpdateObserver(&observer);
+  group = NULL;  // causes group to be deleted
+  EXPECT_TRUE(observer.update_completed_);
+  EXPECT_FALSE(observer.group_has_cache_);
 }
 
 }  // namespace appcache
diff --git a/webkit/appcache/appcache_host.cc b/webkit/appcache/appcache_host.cc
index 50b24a5..9266fd5 100644
--- a/webkit/appcache/appcache_host.cc
+++ b/webkit/appcache/appcache_host.cc
@@ -5,11 +5,7 @@
 #include "webkit/appcache/appcache_host.h"
 
 #include "base/logging.h"
-#include "webkit/appcache/appcache.h"
-#include "webkit/appcache/appcache_group.h"
-#include "webkit/appcache/appcache_interfaces.h"
 #include "webkit/appcache/appcache_request_handler.h"
-#include "webkit/appcache/appcache_service.h"
 
 namespace appcache {
 
@@ -25,6 +21,8 @@ AppCacheHost::~AppCacheHost() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDestructionImminent(this));
   if (associated_cache_.get())
     associated_cache_->UnassociateHost(this);
+  if (group_being_updated_.get())
+    group_being_updated_->RemoveUpdateObserver(this);
   service_->storage()->CancelDelegateCallbacks(this);
 }
 
@@ -232,7 +230,7 @@ void AppCacheHost::FinishCacheSelection(
     DCHECK(new_master_entry_url_.is_empty());
     AssociateCache(cache);
     cache->owning_group()->StartUpdateWithHost(this);
-
+    ObserveGroupBeingUpdated(cache->owning_group());
   } else if (group) {
     // If document was loaded using HTTP GET or equivalent, and, there is a
     // manifest URL, and manifest URL has the same origin as document.
@@ -242,7 +240,7 @@ void AppCacheHost::FinishCacheSelection(
     DCHECK(new_master_entry_url_.is_valid());
     AssociateCache(NULL);  // The UpdateJob may produce one for us later.
     group->StartUpdateWithNewMasterEntry(this, new_master_entry_url_);
-
+    ObserveGroupBeingUpdated(group);
   } else {
     // Otherwise, the Document is not associated with any application cache.
     AssociateCache(NULL);
@@ -259,21 +257,45 @@ void AppCacheHost::FinishCacheSelection(
   FOR_EACH_OBSERVER(Observer, observers_, OnCacheSelectionComplete(this));
 }
 
+void AppCacheHost::ObserveGroupBeingUpdated(AppCacheGroup* group) {
+  DCHECK(!group_being_updated_);
+  group_being_updated_ = group;
+  group->AddUpdateObserver(this);
+}
+
+void AppCacheHost::OnUpdateComplete(AppCacheGroup* group) {
+  DCHECK_EQ(group, group_being_updated_);
+  group->RemoveUpdateObserver(this);
+
+  // Add a reference to the newest complete cache.
+  SetSwappableCache(group);
+
+  group_being_updated_ = NULL;
+}
+
+void AppCacheHost::SetSwappableCache(AppCacheGroup* group) {
+  AppCache* new_cache = group ? group->newest_complete_cache() : NULL;
+  if (new_cache != associated_cache_)
+    swappable_cache_ = new_cache;
+  else
+    swappable_cache_ = NULL;
+}
+
 void AppCacheHost::AssociateCache(AppCache* cache) {
   if (associated_cache_.get()) {
     associated_cache_->UnassociateHost(this);
-    group_ = NULL;
   }
 
   associated_cache_ = cache;
 
   if (cache) {
     cache->AssociateHost(this);
-    group_ = cache->owning_group();
     frontend_->OnCacheSelected(host_id_, cache->cache_id(), GetStatus());
   } else {
     frontend_->OnCacheSelected(host_id_, kNoCacheId, UNCACHED);
   }
+
+  SetSwappableCache(cache ? cache->owning_group() : NULL);
 }
 
 }  // namespace appcache
diff --git a/webkit/appcache/appcache_host.h b/webkit/appcache/appcache_host.h
index 5be3c5c..1ebb68b 100644
--- a/webkit/appcache/appcache_host.h
+++ b/webkit/appcache/appcache_host.h
@@ -11,6 +11,7 @@
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest_prod.h"
 #include "webkit/appcache/appcache.h"
+#include "webkit/appcache/appcache_group.h"
 #include "webkit/appcache/appcache_interfaces.h"
 #include "webkit/appcache/appcache_service.h"
 #include "webkit/appcache/appcache_storage.h"
@@ -21,7 +22,6 @@ namespace appcache {
 
 class AppCache;
 class AppCacheFrontend;
-class AppCacheGroup;
 class AppCacheRequestHandler;
 
 typedef Callback2<Status, void*>::Type GetStatusCallback;
@@ -29,7 +29,8 @@ typedef Callback2<bool, void*>::Type StartUpdateCallback;
 typedef Callback2<bool, void*>::Type SwapCacheCallback;
 
 // Server-side representation of an application cache host.
-class AppCacheHost : public AppCacheStorage::Delegate {
+class AppCacheHost : public AppCacheStorage::Delegate,
+                     public AppCacheGroup::UpdateObserver {
  public:
 
   class Observer {
@@ -76,6 +77,10 @@ class AppCacheHost : public AppCacheStorage::Delegate {
   // or by the update algorithm (see AppCacheUpdateJob).
   void AssociateCache(AppCache* cache);
 
+  // Adds a reference to the newest complete cache in a group, unless it's the
+  // same as the cache that is currently associated with the host.
+  void SetSwappableCache(AppCacheGroup* group);
+
   int host_id() const { return host_id_; }
   AppCacheService* service() const { return service_; }
   AppCacheFrontend* frontend() const { return frontend_; }
@@ -100,15 +105,25 @@ class AppCacheHost : public AppCacheStorage::Delegate {
   void DoPendingStartUpdate();
   void DoPendingSwapCache();
 
+  void ObserveGroupBeingUpdated(AppCacheGroup* group);
+
+  // AppCacheGroup::UpdateObserver method
+  virtual void OnUpdateComplete(AppCacheGroup* group);
+
   // Identifies the corresponding appcache host in the child process.
   int host_id_;
 
   // The cache associated with this host, if any.
   scoped_refptr<AppCache> associated_cache_;
 
-  // The reference to the group ensures the group exists
-  // while we have an association with a cache in the group.
-  scoped_refptr<AppCacheGroup> group_;
+  // Hold a reference to the newest complete cache (if associated cache is
+  // not the newest) to keep the newest cache in existence while the app cache
+  // group is in use. The newest complete cache may have no associated hosts
+  // holding any references to it and would otherwise be deleted prematurely.
+  scoped_refptr<AppCache> swappable_cache_;
+
+  // Keep a reference to the group being updated until the update completes.
+  scoped_refptr<AppCacheGroup> group_being_updated_;
 
   // Cache loading is async, if we're loading a specific cache or group
   // for the purposes of cache selection, one or the other of these will
@@ -145,6 +160,7 @@ class AppCacheHost : public AppCacheStorage::Delegate {
   FRIEND_TEST(AppCacheHostTest, ForeignEntry);
   FRIEND_TEST(AppCacheHostTest, FailedCacheLoad);
   FRIEND_TEST(AppCacheHostTest, FailedGroupLoad);
+  FRIEND_TEST(AppCacheHostTest, SetSwappableCache);
   DISALLOW_COPY_AND_ASSIGN(AppCacheHost);
 };
 
diff --git a/webkit/appcache/appcache_host_unittest.cc b/webkit/appcache/appcache_host_unittest.cc
index cdfaee2..5e36eef 100644
--- a/webkit/appcache/appcache_host_unittest.cc
+++ b/webkit/appcache/appcache_host_unittest.cc
@@ -224,6 +224,65 @@ TEST_F(AppCacheHostTest, FailedGroupLoad) {
   EXPECT_EQ(reinterpret_cast<void*>(1), last_callback_param_);
 }
 
+TEST_F(AppCacheHostTest, SetSwappableCache) {
+  AppCacheHost host(1, &mock_frontend_, &service_);
+  host.SetSwappableCache(NULL);
+  EXPECT_FALSE(host.swappable_cache_.get());
+
+  scoped_refptr<AppCacheGroup> group1 =
+      new AppCacheGroup(&service_, GURL::EmptyGURL());
+  host.SetSwappableCache(group1);
+  EXPECT_FALSE(host.swappable_cache_.get());
+
+  AppCache* cache1 = new AppCache(&service_, 111);
+  cache1->set_complete(true);
+  group1->AddCache(cache1);
+  host.SetSwappableCache(group1);
+  EXPECT_EQ(cache1, host.swappable_cache_.get());
+
+  host.AssociateCache(cache1);
+  EXPECT_FALSE(host.swappable_cache_.get());  // was same as associated cache
+
+  AppCache* cache2 = new AppCache(&service_, 222);
+  cache2->set_complete(true);
+  group1->AddCache(cache2);
+  EXPECT_EQ(cache2, host.swappable_cache_.get());  // updated to newest
+
+  scoped_refptr<AppCacheGroup> group2 =
+      new AppCacheGroup(&service_, GURL("http://foo.com"));
+  AppCache* cache3 = new AppCache(&service_, 333);
+  cache3->set_complete(true);
+  group2->AddCache(cache3);
+
+  AppCache* cache4 = new AppCache(&service_, 444);
+  cache4->set_complete(true);
+  group2->AddCache(cache4);
+  EXPECT_EQ(cache2, host.swappable_cache_.get());  // unchanged
+
+  host.AssociateCache(cache3);
+  EXPECT_EQ(cache4, host.swappable_cache_.get());  // newest cache in group2
+  EXPECT_FALSE(group1->HasCache());  // both caches in group1 have refcount 0
+
+  host.AssociateCache(NULL);
+  EXPECT_FALSE(host.swappable_cache_.get());
+  EXPECT_FALSE(group2->HasCache());  // both caches in group2 have refcount 0
+
+  // Host adds reference to newest cache when an update is complete.
+  AppCache* cache5 = new AppCache(&service_, 555);
+  cache5->set_complete(true);
+  group2->AddCache(cache5);
+  host.group_being_updated_ = group2;
+  host.OnUpdateComplete(group2);
+  EXPECT_FALSE(host.group_being_updated_);
+  EXPECT_EQ(cache5, host.swappable_cache_.get());
+
+  group2->RemoveCache(cache5);
+  EXPECT_FALSE(group2->HasCache());
+  host.group_being_updated_ = group2;
+  host.OnUpdateComplete(group2);
+  EXPECT_FALSE(host.group_being_updated_);
+  EXPECT_FALSE(host.swappable_cache_.get());  // group2 had no newest cache
+}
 // TODO(michaeln): Flesh these tests out more.
 
 }  // namespace appcache
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
diff --git a/webkit/appcache/appcache_update_job.h b/webkit/appcache/appcache_update_job.h
index 0c19196..518404e 100644
--- a/webkit/appcache/appcache_update_job.h
+++ b/webkit/appcache/appcache_update_job.h
@@ -60,6 +60,7 @@ class AppCacheUpdateJob : public URLRequest::Delegate {
     REFETCH_MANIFEST,
     CACHE_FAILURE,
     CANCELLED,
+    COMPLETED,
   };
 
   // Methods for URLRequest::Delegate.
@@ -130,18 +131,14 @@ class AppCacheUpdateJob : public URLRequest::Delegate {
   // Does nothing if update process is still waiting for pending master
   // entries or URL fetches to complete downloading. Otherwise, completes
   // the update process.
-  // Returns true if update process is completed.
-  bool MaybeCompleteUpdate();
-
-  // Runs the cache failure steps after all pending master entry downloads
-  // have completed.
-  void HandleCacheFailure();
+  void MaybeCompleteUpdate();
 
   // Schedules a rerun of the entire update with the same parameters as
   // this update job after a short delay.
   void ScheduleUpdateRetry(int delay_ms);
 
   void Cancel();
+  void DiscardInprogressCache();
 
   // Deletes this object after letting the stack unwind.
   void DeleteSoon();
@@ -151,8 +148,16 @@ class AppCacheUpdateJob : public URLRequest::Delegate {
 
   GURL manifest_url_;  // here for easier access
   AppCacheService* service_;
+
   scoped_refptr<AppCache> inprogress_cache_;
-  scoped_refptr<AppCacheGroup> group_;
+
+  // 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_;
   InternalUpdateState internal_state_;
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index 5dd1536..be1d716 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -177,7 +177,7 @@ RetryRequestTestJob::RetryHeader RetryRequestTestJob::retry_after_;
 int RetryRequestTestJob::expected_requests_ = 0;
 
 class AppCacheUpdateJobTest : public testing::Test,
-                              public AppCacheGroup::Observer {
+                              public AppCacheGroup::UpdateObserver {
  public:
   AppCacheUpdateJobTest()
       : ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)),
@@ -1043,11 +1043,12 @@ class AppCacheUpdateJobTest : public testing::Test,
     if (group_->update_status() == AppCacheGroup::IDLE)
       UpdateFinished();
     else
-      group_->AddObserver(this);
+      group_->AddUpdateObserver(this);
   }
 
   void OnUpdateComplete(AppCacheGroup* group) {
     ASSERT_EQ(group_, group);
+    protect_newest_cache_ = group->newest_complete_cache();
     UpdateFinished();
   }
 
@@ -1066,6 +1067,7 @@ class AppCacheUpdateJobTest : public testing::Test,
       VerifyExpectations();
 
     // Clean up everything that was created on the IO thread.
+    protect_newest_cache_ = NULL;
     group_ = NULL;
     STLDeleteContainerPointers(hosts_.begin(), hosts_.end());
     STLDeleteContainerPointers(frontends_.begin(), frontends_.end());
@@ -1086,7 +1088,6 @@ class AppCacheUpdateJobTest : public testing::Test,
     AppCache* cache = new AppCache(service_.get(), cache_id);
     cache->set_complete(true);
     cache->set_update_time(base::TimeTicks::Now());
-    cache->set_owning_group(group_);
     group_->AddCache(cache);
     return cache;
   }
@@ -1280,6 +1281,7 @@ class AppCacheUpdateJobTest : public testing::Test,
   scoped_ptr<MockAppCacheService> service_;
   scoped_refptr<TestURLRequestContext> request_context_;
   scoped_refptr<AppCacheGroup> group_;
+  scoped_refptr<AppCache> protect_newest_cache_;
   scoped_ptr<base::WaitableEvent> event_;
 
   // Hosts used by an async test that need to live until update job finishes.