Revert "OOR-CORS: store more information to CORSErrorStatus on preflight errors"

This reverts commit 1f675b46.

Reason for revert: Breaks builder WebKit Linux Trusty ASAN

Original change's description:
> OOR-CORS: store more information to CORSErrorStatus on preflight errors
> 
> Current implementation does not store rejected method or header
> information to CORSErrorStatus on CORS-preflight checks, and
> results in missing hint parameter on CreateErrorString() in Blink.
> This patch provides required information to generate right error
> messages, and fixes some crashed layout tests.
> 
> Bug: 803766, 836741
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
> Change-Id: If4de6b916a77d204812a7b010e39080bf059bc9e
> Tbr: kinuko@chromium.org
> Reviewed-on: https://chromium-review.googlesource.com/1030670
> Commit-Queue: Takashi Toyoshima <toyoshim@chromium.org>
> Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#554642}

TBR=kinuko@chromium.org,toyoshim@chromium.org,yhirano@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 803766, 836741, 838057
Change-Id: Ia8eb9bf63b1b0c6edda93eb7d45c1bb20d26a7b7
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1034532Reviewed-by: Christos Froussios <cfroussios@chromium.org>
Commit-Queue: Christos Froussios <cfroussios@chromium.org>
Cr-Commit-Position: refs/heads/master@{#554712}

services/network/cors/preflight_controller.cc

@@ -150,19 +150,34 @@ std::unique_ptr<PreflightResult> CreatePreflightResult(
       detected_error);
 }
 
-base::Optional<CORSErrorStatus> CheckPreflightResult(
+base::Optional<mojom::CORSError> CheckPreflightResult(
     PreflightResult* result,
-    const ResourceRequest& original_request) {
+    const ResourceRequest& original_request,
+    scoped_refptr<net::HttpResponseHeaders>* error_header) {
+  DCHECK(error_header);
+
   base::Optional<mojom::CORSError> error =
       result->EnsureAllowedCrossOriginMethod(original_request.method);
   if (error)
-    return CORSErrorStatus(*error, original_request.method);
+    return error;
 
   std::string detected_error_header;
   error = result->EnsureAllowedCrossOriginHeaders(original_request.headers,
                                                   &detected_error_header);
-  if (error)
-    return CORSErrorStatus(*error, detected_error_header);
+  if (error) {
+    // Gather information to report the error's details.
+    DCHECK(!detected_error_header.empty());
+    std::string header_value;
+    bool found = original_request.headers.GetHeader(detected_error_header,
+                                                    &header_value);
+    DCHECK(found);
+    // Status line below is dummy to construct a response header instance.
+    *error_header =
+        base::MakeRefCounted<net::HttpResponseHeaders>(base::StringPrintf(
+            "HTTP/1.0 200 OK\n%s: %s", detected_error_header.c_str(),
+            header_value.c_str()));
+    return error;
+  }
 
   return base::nullopt;
 }
@@ -276,25 +291,27 @@ class PreflightController::PreflightLoader final {
     std::unique_ptr<PreflightResult> result = CreatePreflightResult(
         final_url, head, original_request_, &detected_error);
 
-    base::Optional<CORSErrorStatus> detected_error_status;
+    scoped_refptr<net::HttpResponseHeaders> error_header;
     if (result) {
       // Preflight succeeded. Check |original_request_| with |result|.
       DCHECK(!detected_error);
-      detected_error_status =
-          CheckPreflightResult(result.get(), original_request_);
-    } else {
-      DCHECK(detected_error);
-      detected_error_status = CORSErrorStatus(*detected_error);
+      detected_error =
+          CheckPreflightResult(result.get(), original_request_, &error_header);
     }
 
     // TODO(toyoshim): Check the spec if we cache |result| regardless of
     // following checks.
-    if (!detected_error_status) {
+    if (!detected_error) {
       controller_->AppendToCache(*original_request_.request_initiator,
                                  original_request_.url, std::move(result));
     }
 
-    std::move(completion_callback_).Run(detected_error_status);
+    if (detected_error) {
+      std::move(completion_callback_)
+          .Run(CORSErrorStatus(*detected_error, error_header));
+    } else {
+      std::move(completion_callback_).Run(base::nullopt);
+    }
 
     RemoveFromController();
     // |this| is deleted here.

services/network/public/cpp/cors/cors_error_status.cc

@@ -19,15 +19,18 @@ CORSErrorStatus::CORSErrorStatus(const CORSErrorStatus& status) = default;
 CORSErrorStatus::CORSErrorStatus(network::mojom::CORSError error)
     : cors_error(error) {}
 
-CORSErrorStatus::CORSErrorStatus(network::mojom::CORSError error,
-                                 const std::string& failed_parameter)
-    : cors_error(error), failed_parameter(failed_parameter) {}
+CORSErrorStatus::CORSErrorStatus(
+    network::mojom::CORSError error,
+    scoped_refptr<net::HttpResponseHeaders> headers)
+    : CORSErrorStatus(error) {
+  related_response_headers = headers;
+}
 
 CORSErrorStatus::~CORSErrorStatus() = default;
 
 bool CORSErrorStatus::operator==(const CORSErrorStatus& rhs) const {
   return cors_error == rhs.cors_error &&
-         failed_parameter == rhs.failed_parameter;
+         related_response_headers == rhs.related_response_headers;
 }
 
 }  // namespace network

services/network/public/cpp/cors/cors_error_status.h

@@ -5,8 +5,6 @@
 #ifndef SERVICES_NETWORK_PUBLIC_CPP_CORS_CORS_ERROR_STATUS_H_
 #define SERVICES_NETWORK_PUBLIC_CPP_CORS_CORS_ERROR_STATUS_H_
 
-#include <string>
-
 #include "base/component_export.h"
 #include "base/memory/scoped_refptr.h"
 #include "net/http/http_response_headers.h"
@@ -24,8 +22,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) CORSErrorStatus {
   CORSErrorStatus(const CORSErrorStatus& status);
 
   explicit CORSErrorStatus(network::mojom::CORSError error);
-  CORSErrorStatus(network::mojom::CORSError error,
-                  const std::string& failed_parameter);
+  CORSErrorStatus(
+      network::mojom::CORSError error,
+      scoped_refptr<net::HttpResponseHeaders> related_response_headers);
 
   ~CORSErrorStatus();
 
@@ -34,8 +33,9 @@ struct COMPONENT_EXPORT(NETWORK_CPP_BASE) CORSErrorStatus {
 
   network::mojom::CORSError cors_error;
 
-  // Contains request method name, or header name that didn't pass a CORS check.
-  std::string failed_parameter;
+  // Partial HTTP response headers including status line that will be useful to
+  // generate a human readable error message.
+  scoped_refptr<net::HttpResponseHeaders> related_response_headers;
 };
 
 }  // namespace network

services/network/public/cpp/network_param_ipc_traits.h

@@ -277,7 +277,7 @@ IPC_ENUM_TRAITS_MAX_VALUE(network::mojom::RequestContextFrameType,
 
 IPC_STRUCT_TRAITS_BEGIN(network::CORSErrorStatus)
   IPC_STRUCT_TRAITS_MEMBER(cors_error)
-  IPC_STRUCT_TRAITS_MEMBER(failed_parameter)
+  IPC_STRUCT_TRAITS_MEMBER(related_response_headers)
 IPC_STRUCT_TRAITS_END()
 
 IPC_STRUCT_TRAITS_BEGIN(network::URLLoaderCompletionStatus)

third_party/WebKit/LayoutTests/TestExpectations

@@ -1228,6 +1228,18 @@ crbug.com/826878 [ Mac ] virtual/color_space/fast/canvas/color-space/toDataURL-c
 # Total  2098
 # Counts skip: 2 crash: 14 text: 67 image: 1 pass: 2005 timeout: 9
 
+# Release build only wpt test crashes on CORS-preflight. Should be fixed soon.
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-cookies-redirect.any.worker.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight-star.any.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight-star.any.worker.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight.any.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-preflight.any.worker.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Crash ]
+crbug.com/836741 [ Release ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Crash ]
+
 # Skipped tests in dictionary order.
 crbug.com/757165 [ Win ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/navigation-redirect.https.html [ Skip ]
 crbug.com/691944 virtual/outofblink-cors/external/wpt/service-workers/service-worker/update-after-oneday.https.html [ Skip ]
@@ -1273,17 +1285,20 @@ crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/simple-cross-
 crbug.com/736308 virtual/outofblink-cors/http/tests/xmlhttprequest/workers/cross-origin-unsupported-url.html [ Timeout ]
 
 # Failing tests in dictionary order.
-crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Failure ]
-crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Failure ]
+crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.html [ Failure ]
+crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect-preflight.any.worker.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect.any.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/fetch/api/cors/cors-redirect.any.worker.html [ Failure ]
 crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/img-mime-types-coverage.tentative.sub.html [ Failure ]
+crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/img-png-mislabeled-as-html-nosniff.tentative.sub.html [ Failure ]
+crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/preload-image-png-mislabeled-as-html-nosniff.tentative.sub.html [ Failure ]
+crbug.com/802835 virtual/outofblink-cors/external/wpt/fetch/corb/script-html-correctly-labeled.tentative.sub.html [ Failure ]
 crbug.com/745327 virtual/outofblink-cors/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/registration-attribute.https.html [ Failure Pass ]
 crbug.com/834183 virtual/outofblink-cors/external/wpt/service-workers/service-worker/ServiceWorkerGlobalScope/update.https.html [ Failure Pass ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-image-cache.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-image.https.html [ Failure ]
-crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Failure ]
-crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Failure ]
+crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video-cache.https.html [ Failure ]
+crbug.com/736308 [ Debug ] virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-canvas-tainting-video.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-event-referrer-policy.https.html [ Failure ]
 crbug.com/595993 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-header-visibility.https.html [ Failure ]
 crbug.com/736308 virtual/outofblink-cors/external/wpt/service-workers/service-worker/fetch-request-html-imports.https.html [ Failure ]

third_party/blink/renderer/core/loader/document_threadable_loader.cc

@@ -1116,15 +1116,20 @@ void DocumentThreadableLoader::DispatchDidFail(const ResourceError& error) {
   if (error.CORSErrorStatus()) {
     DCHECK(out_of_blink_cors_);
     // TODO(toyoshim): Should consider to pass correct arguments instead of
-    // KURL(), 0, and HTTPHeaderMap() to GetErrorString().
-    // We still need plumbing some more information.
+    // WebURL() and WebHTTPHeaderMap() to GetErrorString().
+    // We still need plumbing required information.
+    const int response_code =
+        error.CORSErrorStatus()->related_response_headers
+            ? error.CORSErrorStatus()->related_response_headers->response_code()
+            : 0;
     GetExecutionContext()->AddConsoleMessage(ConsoleMessage::Create(
         kJSMessageSource, kErrorMessageLevel,
         "Failed to load " + error.FailingURL() + ": " +
-            CORS::GetErrorString(
-                CORS::ErrorParameter::Create(
-                    *error.CORSErrorStatus(), KURL(error.FailingURL()), KURL(),
-                    0, HTTPHeaderMap(), *GetSecurityOrigin(), request_context_))
+            CORS::GetErrorString(CORS::ErrorParameter::Create(
+                                     error.CORSErrorStatus()->cors_error,
+                                     KURL(error.FailingURL()), KURL(),
+                                     response_code, HTTPHeaderMap(),
+                                     *GetSecurityOrigin(), request_context_))
                 .Utf8()
                 .data()));
   }

third_party/blink/renderer/platform/loader/cors/cors_error_string.cc

@@ -37,25 +37,15 @@ ErrorParameter CreateWrongParameter(network::mojom::CORSError error) {
 
 // static
 ErrorParameter ErrorParameter::Create(
-    const network::CORSErrorStatus& error_status,
+    const network::mojom::CORSError error,
     const KURL& first_url,
     const KURL& second_url,
     const int status_code,
     const HTTPHeaderMap& header_map,
     const SecurityOrigin& origin,
     const WebURLRequest::RequestContext context) {
-  String hint;
-  switch (error_status.cors_error) {
-    case network::mojom::CORSError::kMethodDisallowedByPreflightResponse:
-    case network::mojom::CORSError::kHeaderDisallowedByPreflightResponse:
-      DCHECK(!error_status.failed_parameter.empty());
-      hint = String(error_status.failed_parameter.c_str());
-      break;
-    default:
-      break;
-  }
-  return ErrorParameter(error_status.cors_error, first_url, second_url,
-                        status_code, header_map, origin, context, hint, false);
+  return ErrorParameter(error, first_url, second_url, status_code, header_map,
+                        origin, context, String(), false);
 }
 
 // static

third_party/blink/renderer/platform/loader/cors/cors_error_string.h

@@ -6,7 +6,6 @@
 #define THIRD_PARTY_BLINK_RENDERER_PLATFORM_LOADER_CORS_CORS_ERROR_STRING_H_
 
 #include "base/macros.h"
-#include "services/network/public/cpp/cors/cors_error_status.h"
 #include "services/network/public/mojom/cors.mojom-shared.h"
 #include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
@@ -24,7 +23,7 @@ namespace CORS {
 struct PLATFORM_EXPORT ErrorParameter {
   // Creates an ErrorParameter for generic cases. Use this function if |error|
   // can contain any.
-  static ErrorParameter Create(const network::CORSErrorStatus&,
+  static ErrorParameter Create(const network::mojom::CORSError,
                                const KURL& first_url,
                                const KURL& second_url,
                                const int status_code,

third_party/blink/renderer/platform/loader/fetch/resource_loader.cc

@@ -308,8 +308,7 @@ bool ResourceLoader::WillFollowRedirect(
         if (!unused_preload) {
           Context().AddErrorConsoleMessage(
               CORS::GetErrorString(CORS::ErrorParameter::Create(
-                  network::CORSErrorStatus(*cors_error),
-                  redirect_response.Url(), new_url,
+                  *cors_error, redirect_response.Url(), new_url,
                   redirect_response.HttpStatusCode(),
                   redirect_response.HttpHeaderFields(), *source_origin.get(),
                   resource_->LastResourceRequest().GetRequestContext())),
@@ -488,7 +487,7 @@ CORSStatus ResourceLoader::DetermineCORSStatus(const ResourceResponse& response,
   error_msg.Append(source_origin->ToString());
   error_msg.Append("' has been blocked by CORS policy: ");
   error_msg.Append(CORS::GetErrorString(CORS::ErrorParameter::Create(
-      network::CORSErrorStatus(*cors_error), initial_request.Url(), KURL(),
+      *cors_error, initial_request.Url(), KURL(),
       response_for_access_control.HttpStatusCode(),
       response_for_access_control.HttpHeaderFields(), *source_origin,
       initial_request.GetRequestContext())));