OOR-CORS: Remove redundant checks from ThreadableLoader

This should be the last OOR-CORS related minor clean up
in the ThreadableLoader.

 - Remove redundant kDisallowedByMode checks from the
   ThreadableLoader as this is checked at the CORS checks
   in the network service.

 - Remove unused methods, ReportResponseReceived,
   GetSecurityOrigin, and GetFrame.

 - Add Resource* and client_ dchecks to methods that
   haven't had such checks to be consistent.

 - Remove ThreadableLoaderTest.*DidFailInStart as Start
   never fails after this change.

Change-Id: Ie91586c56c6d3cf37ec28a9c3f0ea199b95056a2
Bug: 1053866
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2525384
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Auto-Submit: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826800}

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

@@ -54,7 +54,6 @@
 #include "third_party/blink/renderer/platform/loader/fetch/resource_loader.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_loader_options.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_request.h"
-#include "third_party/blink/renderer/platform/weborigin/security_origin.h"
 #include "third_party/blink/renderer/platform/wtf/assertions.h"
 
 namespace blink {
@@ -126,23 +125,8 @@ void ThreadableLoader::Start(ResourceRequest request) {
              network::mojom::CorsPreflightPolicy::kConsiderPreflight ||
          cors::IsCorsEnabledRequestMode(request.GetMode()));
 
-  // TODO(crbug.com/1053866): Remove the following check later as the same
-  // condition is checked outside the Blink.
-  if (cors::CalculateCorsFlag(request.Url(), GetSecurityOrigin(),
-                              request.IsolatedWorldOrigin().get(),
-                              request.GetMode()) &&
-      request.GetMode() == network::mojom::RequestMode::kSameOrigin) {
-    ThreadableLoaderClient* client = client_;
-    Clear();
-    client->DidFail(ResourceError(
-        request.Url(), network::CorsErrorStatus(
-                           network::mojom::CorsError::kDisallowedByMode)));
-    return;
-  }
-
-  security_origin_ = request.RequestorOrigin();
-  request_mode_ = request.GetMode();
   request_started_ = base::TimeTicks::Now();
+  request_mode_ = request.GetMode();
 
   // Set the service worker mode to none if "bypass for network" in DevTools is
   // enabled.
@@ -294,61 +278,44 @@ void ThreadableLoader::DidDownloadToBlob(Resource* resource,
   client_->DidDownloadToBlob(std::move(blob));
 }
 
-void ThreadableLoader::ReportResponseReceived(
-    uint64_t identifier,
-    const ResourceResponse& response) {
-  LocalFrame* frame = GetFrame();
-  if (!frame)
-    return;
-  DocumentLoader* loader = frame->Loader().GetDocumentLoader();
-  probe::DidReceiveResourceResponse(probe::ToCoreProbeSink(execution_context_),
-                                    identifier, loader, response,
-                                    GetResource());
-  frame->Console().ReportResourceResponseReceived(loader, identifier, response);
-}
-
 void ThreadableLoader::ResponseReceived(Resource* resource,
                                         const ResourceResponse& response) {
-  DCHECK_EQ(resource, GetResource());
   DCHECK(client_);
-
-  checker_.ResponseReceived();
-
+  DCHECK_EQ(resource, GetResource());
   DCHECK(!response.WasFallbackRequiredByServiceWorker());
 
-  if (!response.WasFetchedViaServiceWorker()) {
-    client_->DidReceiveResponse(resource->InspectorId(), response);
-    return;
-  }
+  checker_.ResponseReceived();
 
-  // It's possible that we issue a fetch with request with non "no-cors"
-  // mode but get an opaque filtered response if a service worker is involved.
-  // We dispatch a CORS failure for the case.
-  // TODO(yhirano): This is probably not spec conformant. Fix it after
-  // https://github.com/w3c/preload/issues/100 is addressed.
-  // TODO(crbug.com/1053866): Check if this check is still needed.
-  if (request_mode_ != network::mojom::RequestMode::kNoCors &&
+  // Now the following check is not needed as the service worker added their own
+  // checks and today memory cache and preload matching rules are more strict.
+  // TODO(crbug.com/1053866): Remove the check.
+  if (response.WasFetchedViaServiceWorker() &&
+      request_mode_ != network::mojom::RequestMode::kNoCors &&
       response.GetType() == network::mojom::FetchResponseType::kOpaque) {
     DispatchDidFail(ResourceError(
         response.CurrentRequestUrl(),
         network::CorsErrorStatus(network::mojom::CorsError::kInvalidResponse)));
     return;
   }
-
   client_->DidReceiveResponse(resource->InspectorId(), response);
 }
 
-void ThreadableLoader::ResponseBodyReceived(Resource*, BytesConsumer& body) {
-  checker_.ResponseBodyReceived();
+void ThreadableLoader::ResponseBodyReceived(Resource* resource,
+                                            BytesConsumer& body) {
+  DCHECK(client_);
+  DCHECK_EQ(resource, GetResource());
 
+  checker_.ResponseBodyReceived();
   client_->DidStartLoadingResponseBody(body);
 }
 
-void ThreadableLoader::SetSerializedCachedMetadata(Resource*,
+void ThreadableLoader::SetSerializedCachedMetadata(Resource* resource,
                                                    const uint8_t* data,
                                                    size_t size) {
-  checker_.SetSerializedCachedMetadata();
+  DCHECK(client_);
+  DCHECK_EQ(resource, GetResource());
 
+  checker_.SetSerializedCachedMetadata();
   client_->DidReceiveCachedMetadata(reinterpret_cast<const char*>(data),
                                     SafeCast<int>(size));
 }
@@ -356,8 +323,8 @@ void ThreadableLoader::SetSerializedCachedMetadata(Resource*,
 void ThreadableLoader::DataReceived(Resource* resource,
                                     const char* data,
                                     size_t data_length) {
-  DCHECK_EQ(resource, GetResource());
   DCHECK(client_);
+  DCHECK_EQ(resource, GetResource());
 
   checker_.DataReceived();
 
@@ -410,18 +377,6 @@ void ThreadableLoader::DispatchDidFail(const ResourceError& error) {
   client->DidFail(error);
 }
 
-const SecurityOrigin* ThreadableLoader::GetSecurityOrigin() const {
-  return security_origin_ ? security_origin_.get()
-                          : resource_fetcher_->GetProperties()
-                                .GetFetchClientSettingsObject()
-                                .GetSecurityOrigin();
-}
-
-LocalFrame* ThreadableLoader::GetFrame() const {
-  auto* window = DynamicTo<LocalDOMWindow>(execution_context_.Get());
-  return window ? window->GetFrame() : nullptr;
-}
-
 void ThreadableLoader::Trace(Visitor* visitor) const {
   visitor->Trace(execution_context_);
   visitor->Trace(client_);

third_party/blink/renderer/core/loader/threadable_loader.h

@@ -51,9 +51,7 @@
 namespace blink {
 
 class ExecutionContext;
-class LocalFrame;
 class ResourceRequest;
-class SecurityOrigin;
 class ThreadableLoaderClient;
 
 // Useful for doing loader operations from any thread (not threadsafe, just able
@@ -79,7 +77,6 @@ class CORE_EXPORT ThreadableLoader final
   // Loading completes when one of the following methods are called:
   // - DidFinishLoading()
   // - DidFail()
-  // - DidFailAccessControlCheck()
   // - DidFailRedirectCheck()
   // After any of these methods is called, the loader won't call any of the
   // ThreadableLoaderClient methods.
@@ -128,20 +125,10 @@ class CORE_EXPORT ThreadableLoader final
  private:
   void Clear();
 
-  // Notify Inspector and log to console about resource response. Use this
-  // method if response is not going to be finished normally.
-  void ReportResponseReceived(uint64_t identifier, const ResourceResponse&);
-
   void DidTimeout(TimerBase*);
 
   void DispatchDidFail(const ResourceError&);
 
-  const SecurityOrigin* GetSecurityOrigin() const;
-
-  // Returns null if the loader is not associated with a frame.
-  // TODO(kinuko): Remove dependency to frame.
-  LocalFrame* GetFrame() const;
-
   // ResourceClient implementation:
   void NotifyFinished(Resource*) override;
   String DebugName() const override { return "ThreadableLoader"; }
@@ -165,14 +152,8 @@ class CORE_EXPORT ThreadableLoader final
   Member<ExecutionContext> execution_context_;
   Member<ResourceFetcher> resource_fetcher_;
 
-  // Some items may be overridden by m_forceDoNotAllowStoredCredentials and
-  // m_securityOrigin. In such a case, build a ResourceLoaderOptions with
-  // up-to-date values from them and this variable, and use it.
   const ResourceLoaderOptions resource_loader_options_;
 
-  // Corresponds to the CORS flag in the Fetch spec.
-  scoped_refptr<const SecurityOrigin> security_origin_;
-
   // Saved so that we can use the original mode in ResponseReceived() where
   // |resource| might be a reused one (e.g. preloaded resource) which can have a
   // different mode.

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

@@ -420,54 +420,6 @@ TEST_F(ThreadableLoaderTest, ClearInDidFail) {
   ServeRequests();
 }
 
-TEST_F(ThreadableLoaderTest, DidFailInStart) {
-  InSequence s;
-  EXPECT_CALL(GetCheckpoint(), Call(1));
-  CreateLoader();
-  CallCheckpoint(1);
-
-  EXPECT_CALL(
-      *Client(),
-      DidFail(ResourceError(
-          ErrorURL(), network::CorsErrorStatus(
-                          network::mojom::CorsError::kDisallowedByMode))));
-  EXPECT_CALL(GetCheckpoint(), Call(2));
-
-  StartLoader(ErrorURL(), network::mojom::RequestMode::kSameOrigin);
-  CallCheckpoint(2);
-  ServeRequests();
-}
-
-TEST_F(ThreadableLoaderTest, CancelInDidFailInStart) {
-  InSequence s;
-  EXPECT_CALL(GetCheckpoint(), Call(1));
-  CreateLoader();
-  CallCheckpoint(1);
-
-  EXPECT_CALL(*Client(), DidFail(_))
-      .WillOnce(InvokeWithoutArgs(this, &ThreadableLoaderTest::CancelLoader));
-  EXPECT_CALL(GetCheckpoint(), Call(2));
-
-  StartLoader(ErrorURL(), network::mojom::RequestMode::kSameOrigin);
-  CallCheckpoint(2);
-  ServeRequests();
-}
-
-TEST_F(ThreadableLoaderTest, ClearInDidFailInStart) {
-  InSequence s;
-  EXPECT_CALL(GetCheckpoint(), Call(1));
-  CreateLoader();
-  CallCheckpoint(1);
-
-  EXPECT_CALL(*Client(), DidFail(_))
-      .WillOnce(InvokeWithoutArgs(this, &ThreadableLoaderTest::ClearLoader));
-  EXPECT_CALL(GetCheckpoint(), Call(2));
-
-  StartLoader(ErrorURL(), network::mojom::RequestMode::kSameOrigin);
-  CallCheckpoint(2);
-  ServeRequests();
-}
-
 TEST_F(ThreadableLoaderTest, RedirectDidFinishLoading) {
   InSequence s;
   EXPECT_CALL(GetCheckpoint(), Call(1));