Expose details for appcache error events [Chromium]

webkit/browser/appcache/appcache_update_job.cc

Index: webkit/browser/appcache/appcache_update_job.cc
diff --git a/webkit/browser/appcache/appcache_update_job.cc b/webkit/browser/appcache/appcache_update_job.cc
index 3d4a4b20f469c3e47dd8515f35fb80c49d3fda6a..29abf8ef6f85c995eeabf774679b24d12e3ac83b 100644
--- a/webkit/browser/appcache/appcache_update_job.cc
+++ b/webkit/browser/appcache/appcache_update_job.cc
@@ -76,12 +76,16 @@ class HostNotifier {
     }
   }
 
-  void SendErrorNotifications(const std::string& error_message) {
+  void SendErrorNotifications(const std::string& error_message,
+                              const std::string& reason,
+                              const GURL& url,
+                              int status) {
     DCHECK(!error_message.empty());
     for (NotifyHostMap::iterator it = hosts_to_notify.begin();
          it != hosts_to_notify.end(); ++it) {
       AppCacheFrontend* frontend = it->first;
-      frontend->OnErrorEventRaised(it->second, error_message);
+      frontend->OnErrorEventRaised(
+          it->second, error_message, reason, url, status);
     }
   }
 
@@ -420,17 +424,21 @@ AppCacheResponseWriter* AppCacheUpdateJob::CreateResponseWriter() {
   return writer;
 }
 
-void AppCacheUpdateJob::HandleCacheFailure(
-    const std::string& error_message,
-    ResultType result) {
+void AppCacheUpdateJob::HandleCacheFailure(const std::string& error_message,
+                                           const std::string& reason,
+                                           const GURL& resource_url,
+                                           int resource_status,
+                                           ResultType result) {
   // 6.9.4 cache failure steps 2-8.
   DCHECK(internal_state_ != CACHE_FAILURE);
   DCHECK(!error_message.empty());
   DCHECK(result != UPDATE_OK);
   internal_state_ = CACHE_FAILURE;
   CancelAllUrlFetches();
-  CancelAllMasterEntryFetches(error_message);
-  NotifyAllError(error_message);
+  // TODO: Should both of these errors be generated?

[jsbell, 2014/02/21 23:48:23]

The code here will result in two errors - CancelAllMasterEntryFetches() and
NotifyAllError() both call SendErrorNotifications().

As you say, I may just be clueless about where the errors get routed to.

[michaeln, 2014/02/22 00:19:39]

Like everything... not as simple as it should be. There are multiple recipients
of the set of progress/error/yada events generated by an update job. Not all
recipients are equal, some see a different event stream than others (you have to
read the spec real close, but there it is).

What CancelAllMasterEntryFetches does is raise the error event into all pages
that caused a new master entry to be added, and then unplugs those recipients
from receiving any other events. The way in which "recipients are not equal" has
to do with whether a new master entry is being established vs a preexisting
cached page was loaded.

NotifyAllError then spams the remaining recipients with the same error.

In this particular HandleCacheFailure case, see same error is being given to all
recipeints, but there are other cases where the page with the new master will
see an error, but other pages may continue and ultimately see success. (see
HandleMasterEntryFetchCompleted)

Each recipient should see only one terminating event.

Also, a potential source of "update hangs" is bugs in this notifier plumbing
that just drops the ball and leaves some should be recipients out.

+  CancelAllMasterEntryFetches(
+      error_message, reason, resource_url, resource_status);
+  NotifyAllError(error_message, reason, resource_url, resource_status);
   DiscardInprogressCache();
   internal_state_ = COMPLETED;
   AppCacheHistograms::CountUpdateJobResult(
@@ -498,15 +506,24 @@ void AppCacheUpdateJob::HandleManifestFetchCompleted(
     const char* kFormatString = "Manifest fetch failed (%d) %s";
     std::string message = FormatUrlErrorMessage(
         kFormatString, manifest_url_, fetcher->result(), response_code);
-    HandleCacheFailure(message, fetcher->result());
+    HandleCacheFailure(message,
+                       "ManifestFetchFailed",
+                       manifest_url_,
+                       response_code,
+                       fetcher->result());
   }
 }
 
 void AppCacheUpdateJob::OnGroupMadeObsolete(AppCacheGroup* group,
                                             bool success) {
   DCHECK(master_entry_fetches_.empty());
-  CancelAllMasterEntryFetches("The cache has been made obsolete, "
-                              "the manifest file returned 404 or 410");
+  CancelAllMasterEntryFetches(
+      "The cache has been made obsolete, "
+      "the manifest file returned 404 or 410",
+      "CacheObsolete",
+      // TODO: Can we populate these correctly?
+      GURL(),
+      0);
   if (success) {
     DCHECK(group->is_obsolete());
     NotifyAllAssociatedHosts(OBSOLETE_EVENT);
@@ -514,7 +531,11 @@ void AppCacheUpdateJob::OnGroupMadeObsolete(AppCacheGroup* group,
     MaybeCompleteUpdate();
   } else {
     // Treat failure to mark group obsolete as a cache failure.
-    HandleCacheFailure("Failed to mark the cache as obsolete", DB_ERROR);
+    HandleCacheFailure("Failed to mark the cache as obsolete",
+                       "FailedToMarkObsolete",
+                       GURL(),
+                       0,
+                       DB_ERROR);
   }
 }
 
@@ -537,7 +558,8 @@ void AppCacheUpdateJob::ContinueHandleManifestFetchCompleted(bool changed) {
     const char* kFormatString = "Failed to parse manifest %s";
     const std::string message = base::StringPrintf(kFormatString,
         manifest_url_.spec().c_str());
-    HandleCacheFailure(message, MANIFEST_ERROR);
+    HandleCacheFailure(
+        message, "InvalidManifest", manifest_url_, 0, MANIFEST_ERROR);
     VLOG(1) << message;
     return;
   }
@@ -611,7 +633,11 @@ void AppCacheUpdateJob::HandleUrlFetchCompleted(URLFetcher* fetcher) {
         const char* kFormatString = "Resource fetch failed (%d) %s";
         std::string message = FormatUrlErrorMessage(
             kFormatString, url, fetcher->result(), response_code);
-        HandleCacheFailure(message, fetcher->result());
+        HandleCacheFailure(message,
+                           "ResourceFetchFailed",
+                           url,
+                           response_code,
+                           fetcher->result());
         return;
       }
     } else if (response_code == 404 || response_code == 410) {
@@ -696,7 +722,8 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
     const char* kFormatString = "Manifest fetch failed (%d) %s";
     std::string message = FormatUrlErrorMessage(
         kFormatString, request->url(), fetcher->result(), response_code);
-    host_notifier.SendErrorNotifications(message);
+    host_notifier.SendErrorNotifications(
+        message, "ManifestFetchFailed", request->url(), response_code);
 
     // In downloading case, update result is different if all master entries
     // failed vs. only some failing.
@@ -707,7 +734,12 @@ void AppCacheUpdateJob::HandleMasterEntryFetchCompleted(
 
       // Section 6.9.4, step 22.3.
       if (update_type_ == CACHE_ATTEMPT && pending_master_entries_.empty()) {
-        HandleCacheFailure(message, fetcher->result());
+        // TODO: Is this a duplicate error?

[michaeln, 2014/02/22 01:18:50]

No duplicates, the recipients of the error sent by
host_notifier.SendErrorNotifications on line 725 have been unplugged so to speak
(see line 716), so the error sent via HandleCacheFailure will not be delivered
to them.

+        HandleCacheFailure(message,
+                           "ManifestFetchFailed",
+                           request->url(),
+                           response_code,
+                           fetcher->result());
         return;
       }
     }
@@ -749,12 +781,20 @@ void AppCacheUpdateJob::HandleManifestRefetchCompleted(
             << " response code: " << response_code;
     ScheduleUpdateRetry(kRerunDelayMs);
     if (response_code == 200) {
-      HandleCacheFailure("Manifest changed during update", MANIFEST_ERROR);
+      HandleCacheFailure("Manifest changed during update",
+                         "ManifestChanged",
+                         manifest_url_,
+                         response_code,
+                         MANIFEST_ERROR);
     } else {
       const char* kFormatString = "Manifest re-fetch failed (%d) %s";
       std::string message = FormatUrlErrorMessage(
           kFormatString, manifest_url_, fetcher->result(), response_code);
-      HandleCacheFailure(message, fetcher->result());
+      HandleCacheFailure(message,
+                         "ManifestFetchFailed",
+                         manifest_url_,
+                         response_code,
+                         fetcher->result());
     }
   }
 }
@@ -770,6 +810,9 @@ void AppCacheUpdateJob::OnManifestInfoWriteComplete(int result) {
                    base::Unretained(this)));
   } else {
     HandleCacheFailure("Failed to write the manifest headers to storage",
+                       "InternalStorageError",
+                       GURL(),
+                       0,
                        DISKCACHE_ERROR);
   }
 }
@@ -784,6 +827,9 @@ void AppCacheUpdateJob::OnManifestDataWriteComplete(int result) {
     StoreGroupAndCache();
   } else {
     HandleCacheFailure("Failed to write the manifest data to storage",
+                       "InternalStorageError",
+                       GURL(),
+                       0,
                        DISKCACHE_ERROR);
   }
 }
@@ -823,7 +869,12 @@ void AppCacheUpdateJob::OnGroupAndNewestCacheStored(AppCacheGroup* group,
       message.append(", would exceed quota");
       result = QUOTA_ERROR;
     }
-    HandleCacheFailure(message, result);
+    HandleCacheFailure(
+        message,
+        would_exceed_quota ? "QuotaError" : "InternalStorageError",
+        GURL(),
+        0,
+        result);
   }
 }
 
@@ -851,10 +902,14 @@ void AppCacheUpdateJob::NotifyAllFinalProgress() {
   NotifyAllProgress(GURL());
 }
 
-void AppCacheUpdateJob::NotifyAllError(const std::string& error_message) {
+void AppCacheUpdateJob::NotifyAllError(const std::string& error_message,
+                                       const std::string& reason,
+                                       const GURL& resource_url,
+                                       int resource_status) {
   HostNotifier host_notifier;
   AddAllAssociatedHostsToNotifier(&host_notifier);
-  host_notifier.SendErrorNotifications(error_message);
+  host_notifier.SendErrorNotifications(
+      error_message, reason, resource_url, resource_status);
 }
 
 void AppCacheUpdateJob::AddAllAssociatedHostsToNotifier(
@@ -909,6 +964,9 @@ void AppCacheUpdateJob::CheckIfManifestChanged() {
       // Use a local variable because service_ is reset in HandleCacheFailure.
       AppCacheService* service = service_;
       HandleCacheFailure("Manifest entry not found in existing cache",
+                         "InternalStorageError",
+                         GURL(),
+                         0,
                          DB_ERROR);
       AppCacheHistograms::AddMissingManifestEntrySample();
       service->DeleteAppCacheGroup(manifest_url_, net::CompletionCallback());
@@ -1150,7 +1208,10 @@ void AppCacheUpdateJob::FetchMasterEntries() {
 }
 
 void AppCacheUpdateJob::CancelAllMasterEntryFetches(
-    const std::string& error_message) {
+    const std::string& error_message,
+    const std::string& reason,
+    const GURL& resource_url,
+    int resource_status) {
   // 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
@@ -1186,7 +1247,8 @@ void AppCacheUpdateJob::CancelAllMasterEntryFetches(
 
     master_entries_to_fetch_.erase(master_entries_to_fetch_.begin());
   }
-  host_notifier.SendErrorNotifications(error_message);
+  host_notifier.SendErrorNotifications(
+      error_message, reason, resource_url, resource_status);
 }
 
 bool AppCacheUpdateJob::MaybeLoadFromNewestCache(const GURL& url,