AppCache: fix a browser crashing bug that can happen during updates.

content/browser/appcache/appcache_update_job.cc

 // 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.
 
 #include "content/browser/appcache/appcache_update_job.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/strings/string_util.h"
@@ skipping 409 matching lines @@
                                     const GURL& new_master_resource) {
   DCHECK(group_->update_job() == this);
   DCHECK(!group_->is_obsolete());
 
   bool is_new_pending_master_entry = false;
   if (!new_master_resource.is_empty()) {
     DCHECK(new_master_resource == host->pending_master_entry_url());
     DCHECK(!new_master_resource.has_ref());
     DCHECK(new_master_resource.GetOrigin() == manifest_url_.GetOrigin());
 
+    if (ContainsKey(failed_master_entries_, new_master_resource))
+      return;

[gzobqq, 2015/11/20 06:10:54]

Do we care about lack of host notification?

[michaeln, 2015/11/20 20:38:55]

we care more about the crash

for this to happen, the master resource load must succeed a moment earlier and
then fail to load a moment later. this seems like a small corner case. i think
trading minor correctness errors (visible events) in that case for robustness is
a reasonable trade.

[michaeln, 2015/11/20 21:19:15]

If we were to care... looks like the sequence of expected events should be:
CHECKING, DOWNLOADING, ERROR and AssociateNoCache(GURL())?

+
     // Cannot add more to this update if already terminating.
     if (IsTerminating()) {
       group_->QueueUpdate(host, new_master_resource);
       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;
@@ skipping 417 matching lines @@
       host_notifier.AddHost(host);
 
       // In downloading case, disassociate host from inprogress cache.
       if (inprogress_cache_.get())
         host->AssociateNoCache(GURL());
 
       host->RemoveObserver(this);
     }
     hosts.clear();
 
+    failed_master_entries_.insert(url);
+
     const char* kFormatString = "Manifest fetch failed (%d) %s";
     std::string message = FormatUrlErrorMessage(
         kFormatString, request->url(), fetcher->result(), response_code);
     host_notifier.SendErrorNotifications(
         AppCacheErrorDetails(message,
                      APPCACHE_MANIFEST_ERROR,
                      request->url(),
                      response_code,
                      false /*is_cross_origin*/));
 
     // In downloading case, update result is different if all master entries
     // failed vs. only some failing.
     if (inprogress_cache_.get()) {
       // Only count successful downloads to know if all master entries failed.
       pending_master_entries_.erase(found);
       --master_entries_completed_;

[gzobqq, 2015/11/20 06:10:54]

Another possible fix would be to do this erase() and -- outside the if. This way
any failed fetches won't be counted in master_entries_completed_, so failed urls
can't be counted multiple times. On the positive side, any re-fetches will also
get notified. But I'm not quite sure this change doesn't have side effects.

[michaeln, 2015/11/20 20:38:55]

This is very tricky code, the concern for breaking something else is what led me
to add the new collection instead of trying to alter the existing book keeping.
The simple addition is a lot easier to reason about.

 
       // Section 6.9.4, step 22.3.
       if (update_type_ == CACHE_ATTEMPT && pending_master_entries_.empty()) {
         HandleCacheFailure(AppCacheErrorDetails(message,
                                         APPCACHE_MANIFEST_ERROR,
                                         request->url(),
                                         response_code,
                                         false /*is_cross_origin*/),
                            fetcher->result(),
                            GURL());
@@ skipping 788 matching lines @@
       !failed_resource_url.is_empty() &&
           (failed_resource_url.GetOrigin() != manifest_url_.GetOrigin());
 
   AppCacheHistograms::LogUpdateFailureStats(
       manifest_url_.GetOrigin(),
       percent_complete,
       was_making_progress,
       off_origin_resource_failure);
 }
 
 void AppCacheUpdateJob::DeleteSoon() {

[gzobqq, 2015/11/20 06:10:54]

In any case, perhaps double check here or in the destructor that
master_entry_fetches_ and pending_url_fetches_ are empty. It's difficult to
verify the correctness of the check at MaybeCompleteUpdate().

[michaeln, 2015/11/20 20:38:55]

I've converted a few DCHECKs in the dtor to CHECKs given the severity of getting
it wrong.

   ClearPendingMasterEntries();
   manifest_response_writer_.reset();
   storage_->CancelDelegateCallbacks(this);
   service_->RemoveObserver(this);
   service_ = NULL;
 
   // Break the connection with the group so the group cannot call delete
   // on this object after we've posted a task to delete ourselves.
   if (group_) {
     group_->SetUpdateAppCacheStatus(AppCacheGroup::IDLE);
     group_ = NULL;
   }
 
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
 }
 
 }  // namespace content