Workaround for a crashing appcache bug seen on the crash servers. Add an UMA stat to see how often this happens. Also cleanup a renmant of base::Bind transition.

BUG=95101
Review URL: https://chromiumcodereview.appspot.com/10207020

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

8 files changed, 123 insertions, 75 deletions

diff --git a/webkit/appcache/appcache_histograms.cc b/webkit/appcache/appcache_histograms.cc
index d933c77..ae2576f 100644
--- a/webkit/appcache/appcache_histograms.cc
+++ b/webkit/appcache/appcache_histograms.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -8,14 +8,12 @@
 
 namespace appcache {
 
-// static
 void AppCacheHistograms::CountInitResult(InitResultType init_result) {
   UMA_HISTOGRAM_ENUMERATION(
        "appcache.InitResult",
        init_result, NUM_INIT_RESULT_TYPES);
 }
 
-// static
 void AppCacheHistograms::CountCheckResponseResult(
     CheckResponseResultType result) {
   UMA_HISTOGRAM_ENUMERATION(
@@ -23,28 +21,35 @@ void AppCacheHistograms::CountCheckResponseResult(
        result, NUM_CHECK_RESPONSE_RESULT_TYPES);
 }
 
-// static
 void AppCacheHistograms::AddTaskQueueTimeSample(
     const base::TimeDelta& duration) {
   UMA_HISTOGRAM_TIMES("appcache.TaskQueueTime", duration);
 }
 
-// static
 void AppCacheHistograms::AddTaskRunTimeSample(
     const base::TimeDelta& duration) {
   UMA_HISTOGRAM_TIMES("appcache.TaskRunTime", duration);
 }
 
-// static
 void AppCacheHistograms::AddCompletionQueueTimeSample(
     const base::TimeDelta& duration) {
   UMA_HISTOGRAM_TIMES("appcache.CompletionQueueTime", duration);
 }
 
-// static
 void AppCacheHistograms::AddCompletionRunTimeSample(
     const base::TimeDelta& duration) {
   UMA_HISTOGRAM_TIMES("appcache.CompletionRunTime", duration);
 }
 
+void AppCacheHistograms::AddMissingManifestEntrySample() {
+  UMA_HISTOGRAM_BOOLEAN("appcache.MissingManifestEntry", true);
+}
+
+void AppCacheHistograms::AddMissingManifestDetectedAtCallsite(
+    MissingManifestCallsiteType callsite) {
+  UMA_HISTOGRAM_ENUMERATION(
+       "appcache.MissingManifestDetectedAtCallsite",
+       callsite, NUM_MISSING_MANIFEST_CALLSITE_TYPES);
+}
+
 }  // namespace appcache
diff --git a/webkit/appcache/appcache_histograms.h b/webkit/appcache/appcache_histograms.h
index d74f9e7..e9371c3 100644
--- a/webkit/appcache/appcache_histograms.h
+++ b/webkit/appcache/appcache_histograms.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -34,6 +34,15 @@ class AppCacheHistograms {
   static void AddCompletionQueueTimeSample(const base::TimeDelta& duration);
   static void AddCompletionRunTimeSample(const base::TimeDelta& duration);
 
+  static void AddMissingManifestEntrySample();
+
+  enum MissingManifestCallsiteType {
+    CALLSITE_0, CALLSITE_1, CALLSITE_2, CALLSITE_3,
+    NUM_MISSING_MANIFEST_CALLSITE_TYPES
+  };
+  static void AddMissingManifestDetectedAtCallsite(
+      MissingManifestCallsiteType type);
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(AppCacheHistograms);
 };
diff --git a/webkit/appcache/appcache_service.cc b/webkit/appcache/appcache_service.cc
index b9a3079..2bb99cf 100644
--- a/webkit/appcache/appcache_service.cc
+++ b/webkit/appcache/appcache_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -38,54 +38,12 @@ AppCacheInfoCollection::~AppCacheInfoCollection() {}
 
 // AsyncHelper -------
 
-class AppCacheService::NewAsyncHelper
-    : public AppCacheStorage::Delegate {
- public:
-  NewAsyncHelper(AppCacheService* service,
-                 const net::CompletionCallback& callback)
-      : service_(service), callback_(callback) {
-    service_->pending_new_helpers_.insert(this);
-  }
-
-  virtual ~NewAsyncHelper() {
-    if (service_)
-      service_->pending_new_helpers_.erase(this);
-  }
-
-  virtual void Start() = 0;
-  virtual void Cancel();
-
- protected:
-  void CallCallback(int rv) {
-    if (!callback_.is_null()) {
-      // Defer to guarantee async completion.
-      MessageLoop::current()->PostTask(
-          FROM_HERE, base::Bind(&DeferredCallback, callback_, rv));
-    }
-    callback_.Reset();
-  }
-
-  AppCacheService* service_;
-  net::CompletionCallback callback_;
-};
-
-void AppCacheService::NewAsyncHelper::Cancel() {
-  if (!callback_.is_null()) {
-    callback_.Run(net::ERR_ABORTED);
-    callback_.Reset();
-  }
-
-  service_->storage()->CancelDelegateCallbacks(this);
-  service_ = NULL;
-}
-
 class AppCacheService::AsyncHelper
     : public AppCacheStorage::Delegate {
  public:
-  AsyncHelper(
-      AppCacheService* service, const net::CompletionCallback& callback)
-      : service_(service),
-        callback_(callback) {
+  AsyncHelper(AppCacheService* service,
+              const net::CompletionCallback& callback)
+      : service_(service), callback_(callback) {
     service_->pending_helpers_.insert(this);
   }
 
@@ -122,12 +80,12 @@ void AppCacheService::AsyncHelper::Cancel() {
 
 // CanHandleOfflineHelper -------
 
-class AppCacheService::CanHandleOfflineHelper : NewAsyncHelper {
+class AppCacheService::CanHandleOfflineHelper : AsyncHelper {
  public:
   CanHandleOfflineHelper(
       AppCacheService* service, const GURL& url,
       const GURL& first_party, const net::CompletionCallback& callback)
-      : NewAsyncHelper(service, callback),
+      : AsyncHelper(service, callback),
         url_(url),
         first_party_(first_party) {
   }
@@ -167,12 +125,12 @@ void AppCacheService::CanHandleOfflineHelper::OnMainResponseFound(
 
 // DeleteHelper -------
 
-class AppCacheService::DeleteHelper : public NewAsyncHelper {
+class AppCacheService::DeleteHelper : public AsyncHelper {
  public:
   DeleteHelper(
       AppCacheService* service, const GURL& manifest_url,
       const net::CompletionCallback& callback)
-      : NewAsyncHelper(service, callback), manifest_url_(manifest_url) {
+      : AsyncHelper(service, callback), manifest_url_(manifest_url) {
   }
 
   virtual void Start() {
@@ -210,12 +168,12 @@ void AppCacheService::DeleteHelper::OnGroupMadeObsolete(
 
 // DeleteOriginHelper -------
 
-class AppCacheService::DeleteOriginHelper : public NewAsyncHelper {
+class AppCacheService::DeleteOriginHelper : public AsyncHelper {
  public:
   DeleteOriginHelper(
       AppCacheService* service, const GURL& origin,
       const net::CompletionCallback& callback)
-      : NewAsyncHelper(service, callback), origin_(origin),
+      : AsyncHelper(service, callback), origin_(origin),
         num_caches_to_delete_(0), successes_(0), failures_(0) {
   }
 
@@ -302,12 +260,12 @@ void AppCacheService::DeleteOriginHelper::CacheCompleted(bool success) {
 
 // GetInfoHelper -------
 
-class AppCacheService::GetInfoHelper : NewAsyncHelper {
+class AppCacheService::GetInfoHelper : AsyncHelper {
  public:
   GetInfoHelper(
       AppCacheService* service, AppCacheInfoCollection* collection,
       const net::CompletionCallback& callback)
-      : NewAsyncHelper(service, callback), collection_(collection) {
+      : AsyncHelper(service, callback), collection_(collection) {
   }
 
   virtual void Start() {
@@ -481,10 +439,6 @@ AppCacheService::~AppCacheService() {
                 pending_helpers_.end(),
                 std::mem_fun(&AsyncHelper::Cancel));
   STLDeleteElements(&pending_helpers_);
-  std::for_each(pending_new_helpers_.begin(),
-                pending_new_helpers_.end(),
-                std::mem_fun(&NewAsyncHelper::Cancel));
-  STLDeleteElements(&pending_new_helpers_);
   if (quota_client_)
     quota_client_->NotifyAppCacheDestroyed();
 
diff --git a/webkit/appcache/appcache_service.h b/webkit/appcache/appcache_service.h
index a94e12b..c05bea4 100644
--- a/webkit/appcache/appcache_service.h
+++ b/webkit/appcache/appcache_service.h
@@ -99,7 +99,7 @@ class APPCACHE_EXPORT AppCacheService {
 
   // Checks the integrity of 'response_id' by reading the headers and data.
   // If it cannot be read, the cache group for 'manifest_url' is deleted.
-  void CheckAppCacheResponse(const GURL& manifest_url_, int64 cache_id,
+  void CheckAppCacheResponse(const GURL& manifest_url, int64 cache_id,
                              int64 response_id);
 
   // Context for use during cache updates, should only be accessed
@@ -159,7 +159,6 @@ class APPCACHE_EXPORT AppCacheService {
   friend class AppCacheServiceTest;
 
   class AsyncHelper;
-  class NewAsyncHelper;
   class CanHandleOfflineHelper;
   class DeleteHelper;
   class DeleteOriginHelper;
@@ -167,7 +166,6 @@ class APPCACHE_EXPORT AppCacheService {
   class CheckResponseHelper;
 
   typedef std::set<AsyncHelper*> PendingAsyncHelpers;
-  typedef std::set<NewAsyncHelper*> PendingNewAsyncHelpers;
   typedef std::map<int, AppCacheBackendImpl*> BackendMap;
 
   AppCachePolicy* appcache_policy_;
@@ -176,7 +174,6 @@ class APPCACHE_EXPORT AppCacheService {
   scoped_refptr<quota::SpecialStoragePolicy> special_storage_policy_;
   scoped_refptr<quota::QuotaManagerProxy> quota_manager_proxy_;
   PendingAsyncHelpers pending_helpers_;
-  PendingNewAsyncHelpers pending_new_helpers_;
   BackendMap backends_;  // One 'backend' per child process.
   // Context for use during cache updates.
   net::URLRequestContext* request_context_;
diff --git a/webkit/appcache/appcache_storage_impl.cc b/webkit/appcache/appcache_storage_impl.cc
index 3a01390..1ddfaa4 100644
--- a/webkit/appcache/appcache_storage_impl.cc
+++ b/webkit/appcache/appcache_storage_impl.cc
@@ -424,6 +424,13 @@ void AppCacheStorageImpl::StoreOrLoadTask::CreateCacheAndGroupFromRecords(
     (*group) = cache->get()->owning_group();
     DCHECK(group->get());
     DCHECK_EQ(group_record_.group_id, group->get()->group_id());
+
+    // TODO(michaeln): histogram is fishing for clues to crbug/95101
+    if (!cache->get()->GetEntry(group_record_.manifest_url)) {
+      AppCacheHistograms::AddMissingManifestDetectedAtCallsite(
+          AppCacheHistograms::CALLSITE_0);
+    }
+
     storage_->NotifyStorageAccessed(group_record_.origin);
     return;
   }
@@ -440,12 +447,24 @@ void AppCacheStorageImpl::StoreOrLoadTask::CreateCacheAndGroupFromRecords(
   if (group->get()) {
     DCHECK(group_record_.group_id == group->get()->group_id());
     group->get()->AddCache(cache->get());
+
+    // TODO(michaeln): histogram is fishing for clues to crbug/95101
+    if (!cache->get()->GetEntry(group_record_.manifest_url)) {
+      AppCacheHistograms::AddMissingManifestDetectedAtCallsite(
+          AppCacheHistograms::CALLSITE_1);
+    }
   } else {
     (*group) = new AppCacheGroup(
         storage_->service_, group_record_.manifest_url,
         group_record_.group_id);
     group->get()->set_creation_time(group_record_.creation_time);
     group->get()->AddCache(cache->get());
+
+    // TODO(michaeln): histogram is fishing for clues to crbug/95101
+    if (!cache->get()->GetEntry(group_record_.manifest_url)) {
+      AppCacheHistograms::AddMissingManifestDetectedAtCallsite(
+          AppCacheHistograms::CALLSITE_2);
+    }
   }
   DCHECK(group->get()->newest_complete_cache() == cache->get());
 
@@ -1423,6 +1442,12 @@ void AppCacheStorageImpl::StoreGroupAndNewestCache(
       new StoreGroupAndCacheTask(this, group, newest_cache));
   task->AddDelegate(GetOrCreateDelegateReference(delegate));
   task->GetQuotaThenSchedule();
+
+  // TODO(michaeln): histogram is fishing for clues to crbug/95101
+  if (!newest_cache->GetEntry(group->manifest_url())) {
+    AppCacheHistograms::AddMissingManifestDetectedAtCallsite(
+        AppCacheHistograms::CALLSITE_3);
+  }
 }
 
 void AppCacheStorageImpl::FindResponseForMainRequest(
diff --git a/webkit/appcache/appcache_update_job.cc b/webkit/appcache/appcache_update_job.cc
index d7475bc..02fc15b 100644
--- a/webkit/appcache/appcache_update_job.cc
+++ b/webkit/appcache/appcache_update_job.cc
@@ -16,6 +16,7 @@
 #include "net/http/http_request_headers.h"
 #include "net/http/http_response_headers.h"
 #include "webkit/appcache/appcache_group.h"
+#include "webkit/appcache/appcache_histograms.h"
 
 namespace appcache {
 
@@ -294,6 +295,7 @@ bool AppCacheUpdateJob::URLFetcher::MaybeRetryRequest() {
 AppCacheUpdateJob::AppCacheUpdateJob(AppCacheService* service,
                                      AppCacheGroup* group)
     : service_(service),
+      manifest_url_(group->manifest_url()),
       group_(group),
       update_type_(UNKNOWN_TYPE),
       internal_state_(FETCH_MANIFEST),
@@ -301,8 +303,6 @@ AppCacheUpdateJob::AppCacheUpdateJob(AppCacheService* service,
       url_fetches_completed_(0),
       manifest_fetcher_(NULL),
       stored_state_(UNSTORED) {
-  DCHECK(group_);
-  manifest_url_ = group_->manifest_url();
 }
 
 AppCacheUpdateJob::~AppCacheUpdateJob() {
@@ -751,6 +751,9 @@ void AppCacheUpdateJob::StoreGroupAndCache() {
   else
     newest_cache = group_->newest_complete_cache();
   newest_cache->set_update_time(base::Time::Now());
+
+  // TODO(michaeln): dcheck is fishing for clues to crbug/95101
+  DCHECK_EQ(manifest_url_, group_->manifest_url());
   service_->storage()->StoreGroupAndNewestCache(group_, newest_cache,
                                                 this);  // async
 }
@@ -842,7 +845,14 @@ void AppCacheUpdateJob::CheckIfManifestChanged() {
   DCHECK(update_type_ == UPGRADE_ATTEMPT);
   AppCacheEntry* entry =
       group_->newest_complete_cache()->GetEntry(manifest_url_);
-  DCHECK(entry);
+  if (!entry) {
+    // TODO(michaeln): This is just a bandaid to avoid a crash.
+    // http://code.google.com/p/chromium/issues/detail?id=95101
+    HandleCacheFailure("Manifest entry not found in existing cache");
+    AppCacheHistograms::AddMissingManifestEntrySample();
+    service_->DeleteAppCacheGroup(manifest_url_, net::CompletionCallback());
+    return;
+  }
 
   // Load manifest data from storage to compare against fetched manifest.
   manifest_response_reader_.reset(
diff --git a/webkit/appcache/appcache_update_job.h b/webkit/appcache/appcache_update_job.h
index 5961214..7d34448 100644
--- a/webkit/appcache/appcache_update_job.h
+++ b/webkit/appcache/appcache_update_job.h
@@ -235,8 +235,8 @@ class APPCACHE_EXPORT AppCacheUpdateJob : public AppCacheStorage::Delegate,
   bool IsTerminating() { return internal_state_ >= REFETCH_MANIFEST ||
                                 stored_state_ != UNSTORED; }
 
-  GURL manifest_url_;  // here for easier access
   AppCacheService* service_;
+  const GURL manifest_url_;  // here for easier access
 
   scoped_refptr<AppCache> inprogress_cache_;
 
diff --git a/webkit/appcache/appcache_update_job_unittest.cc b/webkit/appcache/appcache_update_job_unittest.cc
index bb26d65..604eaf1 100644
--- a/webkit/appcache/appcache_update_job_unittest.cc
+++ b/webkit/appcache/appcache_update_job_unittest.cc
@@ -571,6 +571,7 @@ class AppCacheUpdateJobTest : public testing::Test,
       : do_checks_after_update_finished_(false),
         expect_group_obsolete_(false),
         expect_group_has_cache_(false),
+        expect_group_is_being_deleted_(false),
         expect_old_cache_(NULL),
         expect_newest_cache_(NULL),
         expect_non_null_update_time_(false),
@@ -989,6 +990,41 @@ class AppCacheUpdateJobTest : public testing::Test,
     // Start update after data write completes asynchronously.
   }
 
+  // See http://code.google.com/p/chromium/issues/detail?id=95101
+  void Bug95101Test() {
+    ASSERT_EQ(MessageLoop::TYPE_IO, MessageLoop::current()->type());
+
+    MakeService();
+    group_ = new AppCacheGroup(
+        service_.get(), MockHttpServer::GetMockUrl("files/empty-manifest"),
+        service_->storage()->NewGroupId());
+    AppCacheUpdateJob* update = new AppCacheUpdateJob(service_.get(), group_);
+    group_->update_job_ = update;
+
+    // Create a malformed cache with a missing manifest entry.
+    GURL wrong_manifest_url =
+        MockHttpServer::GetMockUrl("files/missing-mime-manifest");
+    AppCache* cache = MakeCacheForGroup(1, wrong_manifest_url, 111);
+    MockFrontend* frontend = MakeMockFrontend();
+    AppCacheHost* host = MakeHost(1, frontend);
+    host->AssociateCompleteCache(cache);
+
+    update->StartUpdate(NULL, GURL());
+    EXPECT_TRUE(update->manifest_fetcher_ != NULL);
+
+    // Set up checks for when update job finishes.
+    do_checks_after_update_finished_ = true;
+    expect_group_is_being_deleted_ = true;
+    expect_group_has_cache_ = true;
+    expect_newest_cache_ = cache;  // newest cache unaffected by update
+    MockFrontend::HostIds id(1, host->host_id());
+    frontend->AddExpectedEvent(id, CHECKING_EVENT);
+    frontend->AddExpectedEvent(id, ERROR_EVENT);
+    frontend->expected_error_message_ =
+        "Manifest entry not found in existing cache";
+    WaitForUpdateToFinish();
+  }
+
   void StartUpdateAfterSeedingStorageData(int result) {
     ASSERT_GT(result, 0);
     response_writer_.reset();
@@ -2902,13 +2938,19 @@ class AppCacheUpdateJobTest : public testing::Test,
   }
 
   AppCache* MakeCacheForGroup(int64 cache_id, int64 manifest_response_id) {
+    return MakeCacheForGroup(cache_id, group_->manifest_url(),
+                             manifest_response_id);
+  }
+
+  AppCache* MakeCacheForGroup(int64 cache_id, const GURL& manifest_entry_url,
+                              int64 manifest_response_id) {
     AppCache* cache = new AppCache(service_.get(), cache_id);
     cache->set_complete(true);
     cache->set_update_time(base::Time::Now());
     group_->AddCache(cache);
 
     // Add manifest entry to cache.
-    cache->AddEntry(group_->manifest_url(),
+    cache->AddEntry(manifest_entry_url,
         AppCacheEntry(AppCacheEntry::MANIFEST, manifest_response_id));
 
     return cache;
@@ -2945,6 +2987,7 @@ class AppCacheUpdateJobTest : public testing::Test,
     HttpHeadersRequestTestJob::Verify();
 
     EXPECT_EQ(expect_group_obsolete_, group_->is_obsolete());
+    EXPECT_EQ(expect_group_is_being_deleted_, group_->is_being_deleted());
 
     if (expect_group_has_cache_) {
       EXPECT_TRUE(group_->newest_complete_cache() != NULL);
@@ -3243,6 +3286,7 @@ class AppCacheUpdateJobTest : public testing::Test,
   bool do_checks_after_update_finished_;
   bool expect_group_obsolete_;
   bool expect_group_has_cache_;
+  bool expect_group_is_being_deleted_;
   AppCache* expect_old_cache_;
   AppCache* expect_newest_cache_;
   bool expect_non_null_update_time_;
@@ -3359,6 +3403,10 @@ TEST_F(AppCacheUpdateJobTest, UpgradeManifestDataUnchanged) {
   RunTestOnIOThread(&AppCacheUpdateJobTest::UpgradeManifestDataUnchangedTest);
 }
 
+TEST_F(AppCacheUpdateJobTest, Bug95101Test) {
+  RunTestOnIOThread(&AppCacheUpdateJobTest::Bug95101Test);
+}
+
 TEST_F(AppCacheUpdateJobTest, BasicCacheAttemptSuccess) {
   RunTestOnIOThread(&AppCacheUpdateJobTest::BasicCacheAttemptSuccessTest);
 }