service worker: Add instrumentation to debug FollowRedirect crash.

ServiceWorkerSubresourceLoader::FollowRedirect() seems to be be getting
called when there is no redirect_info_. Add crash keys to see whether
a redirect was already followed, or was never received, or if something
has gone wrong in the loader's state transitions.

Bug: 1162035
Change-Id: I23ff73ca932d8ada60fe8156c9c33a132db42575
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617527Reviewed-by: Asami Doi <asamidoi@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#842407}

content/renderer/service_worker/service_worker_subresource_loader.cc

@@ -8,6 +8,7 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
+#include "base/debug/crash_logging.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/optional.h"
@@ -479,6 +480,7 @@ void ServiceWorkerSubresourceLoader::StartResponse(
       return;
     }
     response_head_->encoded_data_length = 0;
+    received_redirect_for_bug1162035_ = true;
     url_loader_client_->OnReceiveRedirect(*redirect_info_,
                                           response_head_.Clone());
     TransitionToStatus(Status::kSentRedirect);
@@ -684,7 +686,18 @@ void ServiceWorkerSubresourceLoader::FollowRedirect(
          "https://crbug.com/845683";
   DCHECK(!new_url.has_value()) << "Redirect with modified url was not "
                                   "supported yet. crbug.com/845683";
-  DCHECK(redirect_info_);
+
+  // TODO(crbug.com/1162035): Replace with a DCHECK or early return when
+  // the bug is understood.
+  if (!redirect_info_) {
+    SCOPED_CRASH_KEY_NUMBER("bug1162035", "follow_status",
+                            static_cast<int>(status_));
+    SCOPED_CRASH_KEY_BOOL("bug1162035", "received_redirect",
+                          received_redirect_for_bug1162035_);
+    SCOPED_CRASH_KEY_BOOL("bug1162035", "followed_redirect",
+                          followed_redirect_for_bug1162035_);
+    CHECK(false);
+  }
 
   bool should_clear_upload = false;
   net::RedirectUtil::UpdateHttpRequest(
@@ -707,6 +720,7 @@ void ServiceWorkerSubresourceLoader::FollowRedirect(
   // Restart the request.
   TransitionToStatus(Status::kNotStarted);
   redirect_info_.reset();
+  followed_redirect_for_bug1162035_ = true;
   response_callback_receiver_.reset();
   StartRequest(resource_request_);
 }
@@ -858,25 +872,31 @@ void ServiceWorkerSubresourceLoaderFactory::OnMojoDisconnect() {
 }
 
 void ServiceWorkerSubresourceLoader::TransitionToStatus(Status new_status) {
-#if DCHECK_IS_ON()
+  // TODO(crbug.com/1162035): Remove once the bug is understood and replace
+  // the CHECKs below to DCHECKs.
+  SCOPED_CRASH_KEY_NUMBER("bug1162035", "transition_old",
+                          static_cast<int>(status_));
+  SCOPED_CRASH_KEY_NUMBER("bug1162035", "transition_new",
+                          static_cast<int>(new_status));
+
   switch (new_status) {
     case Status::kNotStarted:
-      DCHECK_EQ(status_, Status::kSentRedirect);
+      CHECK_EQ(status_, Status::kSentRedirect);
       break;
     case Status::kStarted:
-      DCHECK_EQ(status_, Status::kNotStarted);
+      CHECK_EQ(status_, Status::kNotStarted);
       break;
     case Status::kSentRedirect:
-      DCHECK_EQ(status_, Status::kStarted);
+      CHECK_EQ(status_, Status::kStarted);
       break;
     case Status::kSentHeader:
-      DCHECK_EQ(status_, Status::kStarted);
+      CHECK_EQ(status_, Status::kStarted);
       break;
     case Status::kSentBody:
-      DCHECK_EQ(status_, Status::kSentHeader);
+      CHECK_EQ(status_, Status::kSentHeader);
       break;
     case Status::kCompleted:
-      DCHECK(
+      CHECK(
           // Network fallback before interception.
           status_ == Status::kNotStarted ||
           // Network fallback after interception.
@@ -887,7 +907,6 @@ void ServiceWorkerSubresourceLoader::TransitionToStatus(Status new_status) {
           status_ == Status::kSentBody);
       break;
   }
-#endif  // DCHECK_IS_ON()
 
   status_ = new_status;
 }

content/renderer/service_worker/service_worker_subresource_loader.h

@@ -206,6 +206,11 @@ class CONTENT_EXPORT ServiceWorkerSubresourceLoader
   blink::mojom::ServiceWorkerFetchEventTimingPtr fetch_event_timing_;
   network::mojom::FetchResponseSource response_source_;
 
+  // For debugging crbug.com/1162035. Set to true after a redirect is
+  // received/followed.
+  bool received_redirect_for_bug1162035_ = false;
+  bool followed_redirect_for_bug1162035_ = false;
+
   base::WeakPtrFactory<ServiceWorkerSubresourceLoader> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerSubresourceLoader);