S13nSW: Improve error messages in ServiceWorkerNewScriptLoader

This fixes failures in
http/tests/serviceworker/chromium/register-error-messages.html

Bug: 715640
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: Id089672074157d5e861d7539ee08115cf7b55488
Reviewed-on: https://chromium-review.googlesource.com/1004555
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#550102}

content/browser/service_worker/service_worker_new_script_loader.cc

@@ -96,6 +96,10 @@ ServiceWorkerNewScriptLoader::ServiceWorkerNewScriptLoader(
                                                      cache_resource_id);
   AdvanceState(State::kStarted);
 
+  // Disable MIME sniffing sniffing. The spec requires the header list to have
+  // a JavaScript MIME type. Therefore, no sniffing is needed.
+  options &= ~network::mojom::kURLLoadOptionSniffMimeType;
+
   network::mojom::URLLoaderClientPtr network_client;
   network_client_binding_.Bind(mojo::MakeRequest(&network_client));
   loader_factory_getter->GetNetworkFactory()->CreateLoaderAndStart(
@@ -134,7 +138,8 @@ void ServiceWorkerNewScriptLoader::OnReceiveResponse(
     const network::ResourceResponseHead& response_head,
     network::mojom::DownloadedTempFilePtr downloaded_file) {
   if (!version_->context() || version_->is_redundant()) {
-    CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED));
+    CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED),
+                    kServiceWorkerFetchScriptError);
     return;
   }
 
@@ -156,10 +161,12 @@ void ServiceWorkerNewScriptLoader::OnReceiveResponse(
 
   if (response_head.headers->response_code() / 100 != 2) {
     // Non-2XX HTTP status code is handled as an error.
-    // TODO(nhiroki): Show an error message equivalent to kBadHTTPResponseError
-    // in service_worker_write_to_cache_job.cc.
+    std::string error_message =
+        base::StringPrintf(kServiceWorkerBadHTTPResponseError,
+                           response_head.headers->response_code());
     CommitCompleted(
-        network::URLLoaderCompletionStatus(net::ERR_INVALID_RESPONSE));
+        network::URLLoaderCompletionStatus(net::ERR_INVALID_RESPONSE),
+        error_message);
     return;
   }
 
@@ -167,25 +174,28 @@ void ServiceWorkerNewScriptLoader::OnReceiveResponse(
   if (net::IsCertStatusError(response_head.cert_status) &&
       !base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kIgnoreCertificateErrors)) {
-    // TODO(nhiroki): Show an error message equivalent to kSSLError in
-    // service_worker_write_to_cache_job.cc.
-    CommitCompleted(network::URLLoaderCompletionStatus(
-        net::MapCertStatusToNetError(response_head.cert_status)));
+    CommitCompleted(
+        network::URLLoaderCompletionStatus(
+            net::MapCertStatusToNetError(response_head.cert_status)),
+        kServiceWorkerSSLError);
     return;
   }
 
   if (resource_type_ == RESOURCE_TYPE_SERVICE_WORKER) {
     if (!blink::IsSupportedJavascriptMimeType(response_head.mime_type)) {
-      // TODO(nhiroki): Show an error message equivalent to kNoMIMEError or
-      // kBadMIMEError in service_worker_write_to_cache_job.cc.
+      std::string error_message =
+          response_head.mime_type.empty()
+              ? kServiceWorkerNoMIMEError
+              : base::StringPrintf(kServiceWorkerBadMIMEError,
+                                   response_head.mime_type.c_str());
       CommitCompleted(
-          network::URLLoaderCompletionStatus(net::ERR_INSECURE_RESPONSE));
+          network::URLLoaderCompletionStatus(net::ERR_INSECURE_RESPONSE),
+          error_message);
       return;
     }
 
     // Check the path restriction defined in the spec:
     // https://w3c.github.io/ServiceWorker/#service-worker-script-response
-    const char kServiceWorkerAllowed[] = "Service-Worker-Allowed";
     std::string service_worker_allowed;
     bool has_header = response_head.headers->EnumerateHeader(
         nullptr, kServiceWorkerAllowed, &service_worker_allowed);
@@ -193,9 +203,9 @@ void ServiceWorkerNewScriptLoader::OnReceiveResponse(
     if (!ServiceWorkerUtils::IsPathRestrictionSatisfied(
             version_->scope(), request_url_,
             has_header ? &service_worker_allowed : nullptr, &error_message)) {
-      // TODO(nhiroki): Report |error_message|.
       CommitCompleted(
-          network::URLLoaderCompletionStatus(net::ERR_INSECURE_RESPONSE));
+          network::URLLoaderCompletionStatus(net::ERR_INSECURE_RESPONSE),
+          error_message);
       return;
     }
 
@@ -215,10 +225,8 @@ void ServiceWorkerNewScriptLoader::OnReceiveRedirect(
   //
   // Step 7.5: "Set request's redirect mode to "error"."
   // https://w3c.github.io/ServiceWorker/#update-algorithm
-  //
-  // TODO(nhiroki): Show an error message equivalent to kRedirectError in
-  // service_worker_write_to_cache_job.cc.
-  CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT));
+  CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT),
+                  kServiceWorkerRedirectError);
 }
 
 void ServiceWorkerNewScriptLoader::OnDataDownloaded(int64_t data_len,
@@ -250,7 +258,8 @@ void ServiceWorkerNewScriptLoader::OnStartLoadingResponseBody(
   mojo::ScopedDataPipeConsumerHandle client_consumer;
   if (mojo::CreateDataPipe(nullptr, &client_producer_, &client_consumer) !=
       MOJO_RESULT_OK) {
-    CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED));
+    CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED),
+                    kServiceWorkerFetchScriptError);
     return;
   }
 
@@ -264,7 +273,7 @@ void ServiceWorkerNewScriptLoader::OnStartLoadingResponseBody(
 void ServiceWorkerNewScriptLoader::OnComplete(
     const network::URLLoaderCompletionStatus& status) {
   if (status.error_code != net::OK) {
-    CommitCompleted(status);
+    CommitCompleted(status, kServiceWorkerFetchScriptError);
     return;
   }
 
@@ -279,7 +288,8 @@ void ServiceWorkerNewScriptLoader::OnComplete(
       // storage.
       return;
     case State::kWroteData:
-      CommitCompleted(network::URLLoaderCompletionStatus(net::OK));
+      CommitCompleted(network::URLLoaderCompletionStatus(net::OK),
+                      std::string() /* status_message */);
       return;
   }
   NOTREACHED() << static_cast<int>(state_);
@@ -328,7 +338,8 @@ void ServiceWorkerNewScriptLoader::WriteHeaders(
 void ServiceWorkerNewScriptLoader::OnWriteHeadersComplete(net::Error error) {
   DCHECK_NE(net::ERR_IO_PENDING, error);
   if (error != net::OK) {
-    CommitCompleted(network::URLLoaderCompletionStatus(error));
+    CommitCompleted(network::URLLoaderCompletionStatus(error),
+                    kServiceWorkerFetchScriptError);
     return;
   }
   AdvanceState(State::kWroteHeaders);
@@ -371,7 +382,8 @@ void ServiceWorkerNewScriptLoader::OnNetworkDataAvailable(MojoResult) {
       // notified via OnComplete().
       AdvanceState(State::kWroteData);
       if (network_load_completed_)
-        CommitCompleted(network::URLLoaderCompletionStatus(net::OK));
+        CommitCompleted(network::URLLoaderCompletionStatus(net::OK),
+                        std::string() /* status_message */);
       return;
     case MOJO_RESULT_SHOULD_WAIT:
       network_watcher_.ArmOrNotify();
@@ -395,7 +407,8 @@ void ServiceWorkerNewScriptLoader::WriteData(
     case MOJO_RESULT_OK:
       break;
     case MOJO_RESULT_FAILED_PRECONDITION:
-      CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED));
+      CommitCompleted(network::URLLoaderCompletionStatus(net::ERR_FAILED),
+                      kServiceWorkerFetchScriptError);
       return;
     case MOJO_RESULT_SHOULD_WAIT:
       // No data was written to |client_producer_| because the pipe was full.
@@ -432,7 +445,8 @@ void ServiceWorkerNewScriptLoader::OnWriteDataComplete(
     net::Error error) {
   DCHECK_NE(net::ERR_IO_PENDING, error);
   if (error != net::OK) {
-    CommitCompleted(network::URLLoaderCompletionStatus(error));
+    CommitCompleted(network::URLLoaderCompletionStatus(error),
+                    kServiceWorkerFetchScriptError);
     return;
   }
   DCHECK(pending_buffer);
@@ -443,7 +457,8 @@ void ServiceWorkerNewScriptLoader::OnWriteDataComplete(
 }
 
 void ServiceWorkerNewScriptLoader::CommitCompleted(
-    const network::URLLoaderCompletionStatus& status) {
+    const network::URLLoaderCompletionStatus& status,
+    const std::string& status_message) {
   AdvanceState(State::kCompleted);
   net::Error error_code = static_cast<net::Error>(status.error_code);
   int bytes_written = -1;
@@ -459,16 +474,13 @@ void ServiceWorkerNewScriptLoader::CommitCompleted(
   } else {
     // AddMessageConsole must be called before notifying that an error occurred
     // because the worker stops soon after receiving the error response.
-    // TODO(nhiroki): Provide more accurate error message instead of
-    // |kFetchScriptError|.
     // TODO(nhiroki): Consider replacing this hacky way with the new error code
     // handling mechanism in URLLoader.
     version_->embedded_worker()->AddMessageToConsole(
-        blink::WebConsoleMessage::kLevelError, kFetchScriptError);
+        blink::WebConsoleMessage::kLevelError, status_message);
   }
   version_->script_cache_map()->NotifyFinishedCaching(
-      request_url_, bytes_written, error_code,
-      std::string() /* status_message */);
+      request_url_, bytes_written, error_code, status_message);
 
   // TODO(nhiroki): Record ServiceWorkerMetrics::CountWriteResponseResult().
   // (https://crbug.com/762357)

content/browser/service_worker/service_worker_new_script_loader.h

@@ -119,7 +119,8 @@ class CONTENT_EXPORT ServiceWorkerNewScriptLoader
 
   // This is the last method that is called on this class. Notifies the final
   // result to |client_| and clears all mojo connections etc.
-  void CommitCompleted(const network::URLLoaderCompletionStatus& status);
+  void CommitCompleted(const network::URLLoaderCompletionStatus& status,
+                       const std::string& status_message);
 
   const GURL request_url_;
 

content/browser/service_worker/service_worker_register_job.cc

@@ -388,7 +388,7 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
   if (main_script_status.status() != net::URLRequestStatus::SUCCESS) {
     message = new_version()->script_cache_map()->main_script_status_message();
     if (message.empty())
-      message = kFetchScriptError;
+      message = kServiceWorkerFetchScriptError;
   }
   Complete(status, message);
 }

content/browser/service_worker/service_worker_write_to_cache_job.cc

@@ -34,17 +34,8 @@ namespace content {
 namespace {
 
 const char kKilledError[] = "The request to fetch the script was interrupted.";
-const char kBadHTTPResponseError[] =
-    "A bad HTTP response code (%d) was received when fetching the script.";
-const char kSSLError[] =
-    "An SSL certificate error occurred when fetching the script.";
-const char kBadMIMEError[] = "The script has an unsupported MIME type ('%s').";
-const char kNoMIMEError[] = "The script does not have a MIME type.";
 const char kClientAuthenticationError[] =
     "Client authentication was required to fetch the script.";
-const char kRedirectError[] =
-    "The script resource is behind a redirect, which is disallowed.";
-const char kServiceWorkerAllowed[] = "Service-Worker-Allowed";
 
 bool ShouldIgnoreSSLError(net::URLRequest* request) {
   const net::HttpNetworkSession::Params* session_params =
@@ -185,7 +176,7 @@ int ServiceWorkerWriteToCacheJob::ReadRawData(net::IOBuffer* buf,
 
   if (rv < 0) {
     net::Error error = static_cast<net::Error>(rv);
-    error = NotifyFinishedCaching(error, kFetchScriptError);
+    error = NotifyFinishedCaching(error, kServiceWorkerFetchScriptError);
     DCHECK_EQ(rv, error);
     return error;
   }
@@ -271,7 +262,7 @@ void ServiceWorkerWriteToCacheJob::OnReceivedRedirect(
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerWriteToCacheJob::OnReceivedRedirect");
   // Script resources can't redirect.
-  NotifyStartErrorHelper(net::ERR_UNSAFE_REDIRECT, kRedirectError);
+  NotifyStartErrorHelper(net::ERR_UNSAFE_REDIRECT, kServiceWorkerRedirectError);
 }
 
 void ServiceWorkerWriteToCacheJob::OnAuthRequired(
@@ -307,7 +298,7 @@ void ServiceWorkerWriteToCacheJob::OnSSLCertificateError(
   } else {
     NotifyStartErrorHelper(
         net::Error(net::MapCertStatusToNetError(ssl_info.cert_status)),
-        kSSLError);
+        kServiceWorkerSSLError);
   }
 }
 
@@ -318,12 +309,12 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest* request,
 
   if (net_error != net::OK) {
     net::Error error = static_cast<net::Error>(net_error);
-    NotifyStartErrorHelper(error, kFetchScriptError);
+    NotifyStartErrorHelper(error, kServiceWorkerFetchScriptError);
     return;
   }
   if (request->GetResponseCode() / 100 != 2) {
-    std::string error_message =
-        base::StringPrintf(kBadHTTPResponseError, request->GetResponseCode());
+    std::string error_message = base::StringPrintf(
+        kServiceWorkerBadHTTPResponseError, request->GetResponseCode());
     NotifyStartErrorHelper(net::ERR_INVALID_RESPONSE, error_message);
     // TODO(michaeln): Instead of error'ing immediately, send the net
     // response to our consumer, just don't cache it?
@@ -335,7 +326,7 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest* request,
       !ShouldIgnoreSSLError(request)) {
     NotifyStartErrorHelper(net::Error(net::MapCertStatusToNetError(
                                request->ssl_info().cert_status)),
-                           kSSLError);
+                           kServiceWorkerSSLError);
     return;
   }
 
@@ -344,9 +335,9 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest* request,
     request->GetMimeType(&mime_type);
     if (!blink::IsSupportedJavascriptMimeType(mime_type)) {
       std::string error_message =
-          mime_type.empty()
-              ? kNoMIMEError
-              : base::StringPrintf(kBadMIMEError, mime_type.c_str());
+          mime_type.empty() ? kServiceWorkerNoMIMEError
+                            : base::StringPrintf(kServiceWorkerBadMIMEError,
+                                                 mime_type.c_str());
       NotifyStartErrorHelper(net::ERR_INSECURE_RESPONSE, error_message);
       return;
     }
@@ -409,7 +400,7 @@ void ServiceWorkerWriteToCacheJob::OnReadCompleted(net::URLRequest* request,
   int result;
   if (bytes_read < 0) {
     net::Error error = static_cast<net::Error>(bytes_read);
-    result = NotifyFinishedCaching(error, kFetchScriptError);
+    result = NotifyFinishedCaching(error, kServiceWorkerFetchScriptError);
   } else {
     result = HandleNetData(bytes_read);
   }
@@ -474,7 +465,8 @@ net::Error ServiceWorkerWriteToCacheJob::NotifyFinishedCaching(
     // response.
     version_->embedded_worker()->AddMessageToConsole(
         blink::WebConsoleMessage::kLevelError,
-        status_message.empty() ? kFetchScriptError : status_message);
+        status_message.empty() ? kServiceWorkerFetchScriptError
+                               : status_message);
   } else {
     size = cache_writer_->bytes_written();
   }

content/common/service_worker/service_worker_types.cc

@@ -19,8 +19,19 @@ const char kServiceWorkerGetRegistrationErrorPrefix[] =
     "Failed to get a ServiceWorkerRegistration: ";
 const char kServiceWorkerGetRegistrationsErrorPrefix[] =
     "Failed to get ServiceWorkerRegistration objects: ";
-const char kFetchScriptError[] =
+const char kServiceWorkerFetchScriptError[] =
     "An unknown error occurred when fetching the script.";
+const char kServiceWorkerBadHTTPResponseError[] =
+    "A bad HTTP response code (%d) was received when fetching the script.";
+const char kServiceWorkerSSLError[] =
+    "An SSL certificate error occurred when fetching the script.";
+const char kServiceWorkerBadMIMEError[] =
+    "The script has an unsupported MIME type ('%s').";
+const char kServiceWorkerNoMIMEError[] =
+    "The script does not have a MIME type.";
+const char kServiceWorkerRedirectError[] =
+    "The script resource is behind a redirect, which is disallowed.";
+const char kServiceWorkerAllowed[] = "Service-Worker-Allowed";
 
 ServiceWorkerFetchRequest::ServiceWorkerFetchRequest() = default;
 

content/common/service_worker/service_worker_types.h

@@ -46,7 +46,13 @@ extern const char kServiceWorkerUpdateErrorPrefix[];
 extern const char kServiceWorkerUnregisterErrorPrefix[];
 extern const char kServiceWorkerGetRegistrationErrorPrefix[];
 extern const char kServiceWorkerGetRegistrationsErrorPrefix[];
-extern const char kFetchScriptError[];
+extern const char kServiceWorkerFetchScriptError[];
+extern const char kServiceWorkerBadHTTPResponseError[];
+extern const char kServiceWorkerSSLError[];
+extern const char kServiceWorkerBadMIMEError[];
+extern const char kServiceWorkerNoMIMEError[];
+extern const char kServiceWorkerRedirectError[];
+extern const char kServiceWorkerAllowed[];
 
 // Constants for invalid identifiers.
 static const int kInvalidEmbeddedWorkerThreadId = -1;

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

@@ -71,7 +71,6 @@ Bug(none) http/tests/security/mime-type-execute-as-html-04.html [ Failure ]
 Bug(none) http/tests/security/mixedContent/filesystem-url-in-iframe.html [ Failure Timeout ]
 Bug(none) http/tests/security/offscreen-canvas-worker-read-blocked-by-setting.html [ Crash Pass Timeout ]
 Bug(none) http/tests/serviceworker/chromium.update-served-from-cache.html [ Failure ]
-Bug(none) http/tests/serviceworker/chromium/register-error-messages.html [ Failure ]
 Bug(none) plugins/iframe-plugin-bgcolor.html [ Timeout ]
 
 # Plugin throttle is not implemented.