S13nServiceWorker: do not destroy SW's main resource loader too early

After the main resource loader is bound to a loader request
the loader shouldn't be deleted when the request handler is
deleted. (The loader should be able to outlive as far as the
loader's Mojo pipe is held by its client)

Bug: 790933
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I81dab1599d1b0af3aa03f2acfed5f8edc4dd53dd
Reviewed-on: https://chromium-review.googlesource.com/828225Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Commit-Queue: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524373}

content/browser/service_worker/service_worker_url_job_wrapper.cc

@@ -19,7 +19,15 @@ ServiceWorkerURLJobWrapper::ServiceWorkerURLJobWrapper(
     std::unique_ptr<ServiceWorkerURLLoaderJob> url_loader_job)
     : url_loader_job_(std::move(url_loader_job)) {}
 
-ServiceWorkerURLJobWrapper::~ServiceWorkerURLJobWrapper() {}
+ServiceWorkerURLJobWrapper::~ServiceWorkerURLJobWrapper() {
+  if (url_loader_job_) {
+    // Detach the URLLoader from this wrapper (and therefore
+    // from the request handler). This may delete the
+    // |url_loader_job_| or may make it self-owned if it is
+    // bound to a Mojo endpoint.
+    url_loader_job_.release()->DetachedFromRequest();
+  }
+}
 
 void ServiceWorkerURLJobWrapper::FallbackToNetwork() {
   if (url_loader_job_) {

content/browser/service_worker/service_worker_url_loader_job.cc

@@ -87,7 +87,7 @@ ServiceWorkerURLLoaderJob::ServiceWorkerURLLoaderJob(
   response_head_.load_timing.request_start_time = base::Time::Now();
 }
 
-ServiceWorkerURLLoaderJob::~ServiceWorkerURLLoaderJob() {}
+ServiceWorkerURLLoaderJob::~ServiceWorkerURLLoaderJob() = default;
 
 void ServiceWorkerURLLoaderJob::FallbackToNetwork() {
   response_type_ = ResponseType::FALLBACK_TO_NETWORK;
@@ -137,12 +137,18 @@ void ServiceWorkerURLLoaderJob::Cancel() {
   url_loader_client_->OnComplete(
       network::URLLoaderCompletionStatus(net::ERR_ABORTED));
   url_loader_client_.reset();
+  DeleteIfNeeded();
 }
 
 bool ServiceWorkerURLLoaderJob::WasCanceled() const {
   return status_ == Status::kCancelled;
 }
 
+void ServiceWorkerURLLoaderJob::DetachedFromRequest() {
+  detached_from_request_ = true;
+  DeleteIfNeeded();
+}
+
 void ServiceWorkerURLLoaderJob::StartRequest() {
   DCHECK_EQ(ResponseType::FORWARD_TO_SERVICE_WORKER, response_type_);
   DCHECK_EQ(Status::kNotStarted, status_);
@@ -418,4 +424,9 @@ void ServiceWorkerURLLoaderJob::OnComplete(
   url_loader_client_->OnComplete(status);
 }
 
+void ServiceWorkerURLLoaderJob::DeleteIfNeeded() {
+  if (!binding_.is_bound() && detached_from_request_)
+    delete this;
+}
+
 }  // namespace content

content/browser/service_worker/service_worker_url_loader_job.h

@@ -41,6 +41,9 @@ class ServiceWorkerVersion;
 // but with mojom::URLLoader instead of URLRequest.
 // This also works as a URLLoaderClient for BlobURLLoader while reading
 // the blob content returned by SW.
+// This class is owned by the job wrapper until it is bound to a URLLoader
+// request. After it is bound |this| is kept alive until the Mojo connection
+// to this URLLoader is dropped.
 class CONTENT_EXPORT ServiceWorkerURLLoaderJob : public mojom::URLLoader,
                                                  public mojom::URLLoaderClient {
  public:
@@ -93,6 +96,13 @@ class CONTENT_EXPORT ServiceWorkerURLLoaderJob : public mojom::URLLoader,
   void Cancel();
   bool WasCanceled() const;
 
+  // The navigation request that was holding this job is
+  // going away. Calling this internally calls |DeleteIfNeeded()|
+  // and may delete |this| if it is not bound to a endpoint.
+  // Otherwise |this| will be kept around as far as the loader
+  // endpoint is held by the client.
+  void DetachedFromRequest();
+
  private:
   class StreamWaiter;
 
@@ -157,6 +167,8 @@ class CONTENT_EXPORT ServiceWorkerURLLoaderJob : public mojom::URLLoader,
       mojo::ScopedDataPipeConsumerHandle body) override;
   void OnComplete(const network::URLLoaderCompletionStatus& status) override;
 
+  void DeleteIfNeeded();
+
   ResponseType response_type_ = ResponseType::NOT_DETERMINED;
   LoaderCallback loader_callback_;
 
@@ -187,6 +199,8 @@ class CONTENT_EXPORT ServiceWorkerURLLoaderJob : public mojom::URLLoader,
   };
   Status status_ = Status::kNotStarted;
 
+  bool detached_from_request_ = false;
+
   base::WeakPtrFactory<ServiceWorkerURLLoaderJob> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ServiceWorkerURLLoaderJob);

third_party/WebKit/LayoutTests/FlagExpectations/enable-features=NetworkService

@@ -25,14 +25,18 @@ Bug(none) external/wpt/navigation-timing/nav2_test_attributes_values.html [ Fail
 Bug(none) external/wpt/preload/fetch-destination.https.html [ Crash Failure Timeout ]
 Bug(none) external/wpt/resource-timing/test_resource_timing.html [ Failure Timeout ]
 Bug(none) external/wpt/service-workers/service-worker/claim-shared-worker-fetch.https.html [ Failure ]
+Bug(none) external/wpt/service-workers/service-worker/client-navigate.https.html [ Pass Crash Failure ]
 Bug(none) external/wpt/service-workers/service-worker/clients-get-client-types.https.html [ Failure ]
 Bug(none) external/wpt/service-workers/service-worker/clients-get.https.html [ Failure ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-event-respond-with-response-body-with-invalid-chunk.https.html [ Failure ]
+Bug(none) external/wpt/service-workers/service-worker/fetch-event.https.html [ Pass Failure Crash ]
+Bug(none) external/wpt/service-workers/service-worker/fetch-frame-resource.https.html [ Pass Failure ]
+Bug(none) external/wpt/service-workers/service-worker/fetch-header-visibility.https.html [ Pass Failure Crash ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-request-resources.https.html [ Pass Failure Crash ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-request-xhr-sync.https.html [ Timeout ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-request-xhr.https.html [ Timeout ]
 Bug(none) external/wpt/service-workers/service-worker/fetch-response-xhr.https.html [ Failure ]
-Bug(none) external/wpt/service-workers/service-worker/import-scripts-resource-map.https.html [ Crash ]
+Bug(none) external/wpt/service-workers/service-worker/import-scripts-resource-map.https.html [ Failure Crash ]
 crbug.com/771118 external/wpt/service-workers/service-worker/mime-sniffing.https.html [ Failure ]
 Bug(none) external/wpt/service-workers/service-worker/register-link-header.https.html [ Timeout ]
 Bug(none) external/wpt/service-workers/service-worker/registration-updateviacache.https.html [ Failure Timeout ]
@@ -44,6 +48,8 @@ Bug(none) external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/r
 Bug(none) external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/unregister.https.html [ Pass Failure ]
 Bug(none) external/wpt/service-workers/service-worker/appcache-ordering-main.https.html [ Pass Failure ]
 Bug(none) external/wpt/service-workers/service-worker/ready.https.html [ Pass Crash Failure ]
+Bug(none) http/tests/local/serviceworker/fetch-request-body-file.html [ Pass Crash Failure ]
+Bug(none) http/tests/serviceworker/chromium/stop-worker-during-respond-with.html [ Pass Failure ]
 Bug(none) http/tests/appcache/top-frame-3.html [ Pass Timeout ]
 Bug(none) http/tests/appcache/top-frame-4.html [ Pass Timeout ]
 Bug(none) http/tests/appcache/local-content.html [ Pass Timeout ]
@@ -186,6 +192,7 @@ crbug.com/783982 http/tests/devtools/elements/styles-2/property-ui-location.js [
 crbug.com/783982 http/tests/devtools/elements/styles-1/empty-background-url.js [ Failure ]
 
 # http/tests/fetch
+crbug.com/778721 http/tests/fetch/referrer/origin-when-cross-origin-serviceworker-from-document.html [ Pass Crash ]
 crbug.com/778721 http/tests/fetch/serviceworker-proxied/thorough/auth-base-https-other-https.html [ Pass Failure Timeout ]
 crbug.com/778721 http/tests/fetch/serviceworker-proxied/thorough/cookie-nocors-other-https.html [ Pass Failure Timeout ]
 crbug.com/778721 http/tests/fetch/serviceworker-proxied/thorough/cookie.html [ Pass Failure Timeout ]
@@ -229,48 +236,3 @@ crbug.com/777879 external/wpt/FileAPI/file/send-file-form-utf-8.html [ Crash ]
 crbug.com/777879 external/wpt/FileAPI/file/send-file-form-windows-1252.tentative.html [ Crash ]
 crbug.com/777879 external/wpt/FileAPI/file/send-file-form-x-user-defined.tentative.html [ Crash ]
 crbug.com/777879 external/wpt/FileAPI/file/send-file-form.html [ Crash ]
-
-# NetworkService: ServiceWorker URLLoader are destroyed before the end of a navigation.
-crbug.com/790933 external/wpt/fetch/api/abort/serviceworker-intercepted.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/claim-affect-other-registration.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/client-navigate.https.html [ Pass Failure Crash Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-canvas-tainting-cache.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-canvas-tainting.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-cors-exposed-header-names.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-cors-xhr.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-csp.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-event-redirect.https.html [ Pass Crash Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-event.https.html [ Pass Failure Crash Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-frame-resource.https.html [ Pass Failure Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-header-visibility.https.html [ Pass Failure Crash Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-mixed-content-to-inscope.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-mixed-content-to-outscope.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-request-css-cross-origin-mime-check.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-request-no-freshness-headers.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-request-redirect.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/fetch-response-taint.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/getregistrations.https.html [ Pass Failure Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/navigation-preload/chunked-encoding.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/navigation-preload/redirect.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/navigation-preload/request-headers.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/navigation-redirect.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/navigation-redirect-body.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/referer.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/referrer-policy-header.https.html [ Pass Timeout ]
-crbug.com/790933 external/wpt/service-workers/service-worker/sandboxed-iframe-fetch-event.https.html [ Pass Timeout ]
-crbug.com/790933 http/tests/devtools/service-workers/service-workers-navigation-preload.js [ Pass Timeout ]
-crbug.com/790933 http/tests/fetch/referrer/origin-when-cross-origin-serviceworker-from-document.html [ Pass Timeout Crash ]
-crbug.com/790933 http/tests/fetch/referrer/serviceworker-from-origin-only-document.html [ Pass Timeout ]
-crbug.com/790933 http/tests/local/serviceworker/fetch-request-body-file.html [ Pass Crash Failure Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.fetch-canvas-tainting.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.fetch-cors-xhr.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.fetch-csp.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.redirected-response.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.respond-to-same-origin-request-with-cross-origin-response.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium.respond-to-same-origin-request-with-redirected-cross-origin-response.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium/css-import-crash.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium/fetch-request-with-gc.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium/fetch-script-onerror.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/chromium/stop-worker-during-respond-with.html [ Pass Failure Timeout ]
-crbug.com/790933 http/tests/serviceworker/navigation-preload/chromium/navigation-preload-gc-before-response.html [ Pass Timeout ]
-crbug.com/790933 http/tests/serviceworker/navigation-preload/chromium/use-counter.html [ Pass Timeout ]