Race OnStartLoadingResponseBody and OnReceiveResponse of NavigationPreloadRequest.

content/renderer/service_worker/service_worker_context_client.cc

Index: content/renderer/service_worker/service_worker_context_client.cc
diff --git a/content/renderer/service_worker/service_worker_context_client.cc b/content/renderer/service_worker/service_worker_context_client.cc
index 72e40dc891c8409841cd6d4420acaf968aa8198a..f76e81dbe660899a2d5904ad16242c7ea2a8af31 100644
--- a/content/renderer/service_worker/service_worker_context_client.cc
+++ b/content/renderer/service_worker/service_worker_context_client.cc
@@ -242,11 +242,9 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
                            const GURL& url,
                            mojom::FetchEventPreloadHandlePtr preload_handle)
       : fetch_event_id_(fetch_event_id),
+        url_(url),
         url_loader_(std::move(preload_handle->url_loader)),
-        binding_(this, std::move(preload_handle->url_loader_client_request)),
-        response_(base::MakeUnique<blink::WebServiceWorkerResponse>()) {
-    response_->setURL(url);
-  }
+        binding_(this, std::move(preload_handle->url_loader_client_request)) {}
 
   ~NavigationPreloadRequest() override {
     if (result_reported_)
@@ -265,7 +263,9 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
   }
 
   void OnReceiveResponse(const ResourceResponseHead& response_head) override {
-    DCHECK(response_);
+    DCHECK(!response_);
+    response_ = base::MakeUnique<blink::WebServiceWorkerResponse>();
+    response_->setURL(url_);
     DCHECK(response_head.headers);
     response_->setStatus(response_head.headers->response_code());
     response_->setStatusText(
@@ -280,19 +280,14 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
                               blink::WebString::fromUTF8(header_value));
     }
     response_->setResponseTime(response_head.response_time.ToInternalValue());
+    MaybeReportToClient();
   }
 
   void OnStartLoadingResponseBody(
       mojo::ScopedDataPipeConsumerHandle body) override {
-    DCHECK(!result_reported_);
-    ServiceWorkerContextClient* client =
-        ServiceWorkerContextClient::ThreadSpecificInstance();
-    if (!client)
-      return;
-    client->OnNavigationPreloadResponse(
-        fetch_event_id_, std::move(response_),
-        base::MakeUnique<WebDataConsumerHandleImpl>(std::move(body)));
-    result_reported_ = true;
+    DCHECK(!body_.is_valid());
+    body_ = std::move(body);
+    MaybeReportToClient();
   }
 
   void OnComplete(const ResourceRequestCompletionStatus& status) override {
@@ -315,10 +310,28 @@ class ServiceWorkerContextClient::NavigationPreloadRequest final
   }
 
  private:
+  void MaybeReportToClient() {
+    DCHECK(!result_reported_);

[falken, 2016/11/04 09:36:50]

I'm missing something... how does this DCHECK pass since both
OnReceiveRersponse() and OnStartLoadingResposneBody() call
MaybeReportToClient()? Does it need to be moved under the if below?

[horo, 2016/11/04 09:41:48]

Both OnReceiveRersponse() and OnStartLoadingResposneBody() are called only once.
OnReceiveRersponse() sets |response_|.
OnStartLoadingResposneBody() sets |body_|.
So this check "if (!response_ || !body_.is_valid())" pass only once.

[falken, 2016/11/04 09:46:55]

Ah yes... I was totally missing something.

+    if (!response_ || !body_.is_valid())
+      return;
+    ServiceWorkerContextClient* client =
+        ServiceWorkerContextClient::ThreadSpecificInstance();
+    if (!client)
+      return;
+
+    client->OnNavigationPreloadResponse(
+        fetch_event_id_, std::move(response_),
+        base::MakeUnique<WebDataConsumerHandleImpl>(std::move(body_)));
+    result_reported_ = true;
+  }
+
   const int fetch_event_id_;
+  const GURL url_;
   mojom::URLLoaderPtr url_loader_;
   mojo::Binding<mojom::URLLoaderClient> binding_;
+
   std::unique_ptr<blink::WebServiceWorkerResponse> response_;
+  mojo::ScopedDataPipeConsumerHandle body_;
   bool result_reported_ = false;
 };