Make FetchContext::GetSecurityOrigin() non-virtual

This CL introduces FetchContext::fetch_client_settings_object_ and makes
- FetchContext::GetSecurityOrigin()
- FetchContext::GetFetchClientSettingsObject()
non-virtual, to make it clearer that FetchContext's SecurityOrigin
is always the SecurityOrigin of FetchContext's FetchClientSettingsObject.

Bug: 880027
Change-Id: I7ea1cc0edd4d7096b3a0c009c24fcb140d275430
Reviewed-on: https://chromium-review.googlesource.com/c/1352042
Commit-Queue: Hiroshige Hayashizaki <hiroshige@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614282}

third_party/blink/renderer/core/dom/document.cc

@@ -704,7 +704,10 @@ Document::Document(const DocumentInit& initializer,
     ProvideContextFeaturesToDocumentFrom(*this, *frame_->GetPage());
 
     fetcher_ = frame_->Loader().GetDocumentLoader()->Fetcher();
-    FrameFetchContext::ProvideDocumentToContext(fetcher_->Context(), this);
+    FetchContext& fetch_context = fetcher_->Context();
+    CHECK(fetch_context.IsFrameFetchContext());
+    static_cast<FrameFetchContext&>(fetch_context)
+        .ProvideDocumentToContext(this);
 
     // TODO(dcheng): Why does this need to check that DOMWindow is non-null?
     CustomElementRegistry* registry =

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

@@ -45,17 +45,14 @@ class MockBaseFetchContext final : public BaseFetchContext {
   explicit MockBaseFetchContext(ExecutionContext* execution_context)
       : BaseFetchContext(
             execution_context->GetTaskRunner(blink::TaskType::kInternalTest)),
-        execution_context_(execution_context),
-        fetch_client_settings_object_(
-            MakeGarbageCollected<FetchClientSettingsObjectImpl>(
-                *execution_context)) {}
+        execution_context_(execution_context) {
+    SetFetchClientSettingsObject(
+        MakeGarbageCollected<FetchClientSettingsObjectImpl>(
+            *execution_context));
+  }
   ~MockBaseFetchContext() override = default;
 
   // BaseFetchContext overrides:
-  const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const override {
-    return fetch_client_settings_object_.Get();
-  }
   KURL GetSiteForCookies() const override { return KURL(); }
   bool AllowScriptFromSource(const KURL&) const override { return false; }
   SubresourceFilter* GetSubresourceFilter() const override { return nullptr; }
@@ -95,9 +92,6 @@ class MockBaseFetchContext final : public BaseFetchContext {
   }
   const KURL& Url() const override { return execution_context_->Url(); }
 
-  const SecurityOrigin* GetSecurityOrigin() const override {
-    return fetch_client_settings_object_->GetSecurityOrigin();
-  }
   const SecurityOrigin* GetParentSecurityOrigin() const override {
     return nullptr;
   }

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

@@ -293,19 +293,17 @@ FrameFetchContext::FrameFetchContext(DocumentLoader* loader, Document* document)
       save_data_enabled_(GetNetworkStateNotifier().SaveDataEnabled() &&
                          !GetSettings()->GetDataSaverHoldbackWebApi()) {
   if (document_) {
-    fetch_client_settings_object_ =
-        MakeGarbageCollected<FetchClientSettingsObjectImpl>(*document_);
+    SetFetchClientSettingsObject(
+        MakeGarbageCollected<FetchClientSettingsObjectImpl>(*document_));
   }
   DCHECK(GetFrame());
 }
 
-void FrameFetchContext::ProvideDocumentToContext(FetchContext& context,
-                                                 Document* document) {
+void FrameFetchContext::ProvideDocumentToContext(Document* document) {
   DCHECK(document);
-  CHECK(context.IsFrameFetchContext());
-  static_cast<FrameFetchContext&>(context).document_ = document;
-  static_cast<FrameFetchContext&>(context).fetch_client_settings_object_ =
-      MakeGarbageCollected<FetchClientSettingsObjectImpl>(*document);
+  document_ = document;
+  SetFetchClientSettingsObject(
+      MakeGarbageCollected<FetchClientSettingsObjectImpl>(*document));
 }
 
 FrameFetchContext::~FrameFetchContext() {
@@ -363,12 +361,6 @@ FrameScheduler* FrameFetchContext::GetFrameScheduler() const {
   return GetFrame()->GetFrameScheduler();
 }
 
-const FetchClientSettingsObject*
-FrameFetchContext::GetFetchClientSettingsObject() const {
-  DCHECK(fetch_client_settings_object_);
-  return fetch_client_settings_object_.Get();
-}
-
 KURL FrameFetchContext::GetSiteForCookies() const {
   if (IsDetached())
     return frozen_state_->site_for_cookies;
@@ -935,13 +927,6 @@ bool FrameFetchContext::UpdateTimingInfoForIFrameNavigation(
   return true;
 }
 
-const SecurityOrigin* FrameFetchContext::GetSecurityOrigin() const {
-  // This can be called before |fetch_client_settings_object_| is set.
-  if (!fetch_client_settings_object_)
-    return nullptr;
-  return fetch_client_settings_object_->GetSecurityOrigin();
-}
-
 void FrameFetchContext::ModifyRequestForCSP(ResourceRequest& resource_request) {
   if (IsDetached())
     return;
@@ -1452,9 +1437,9 @@ FetchContext* FrameFetchContext::Detach() {
         GetContentSecurityPolicy(), GetSiteForCookies(),
         GetClientHintsPreferences(), GetDevicePixelRatio(), GetUserAgent(),
         IsMainFrame(), IsSVGImageChromeClient());
-    fetch_client_settings_object_ =
+    SetFetchClientSettingsObject(
         MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
-            *GetFetchClientSettingsObject());
+            *GetFetchClientSettingsObject()));
   } else {
     // Some getters are unavailable in this case.
     frozen_state_ = MakeGarbageCollected<FrozenState>(
@@ -1462,10 +1447,10 @@ FetchContext* FrameFetchContext::Detach() {
         GetContentSecurityPolicy(), GetSiteForCookies(),
         GetClientHintsPreferences(), GetDevicePixelRatio(), GetUserAgent(),
         IsMainFrame(), IsSVGImageChromeClient());
-    fetch_client_settings_object_ =
+    SetFetchClientSettingsObject(
         MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
             NullURL(), nullptr, network::mojom::ReferrerPolicy::kDefault,
-            String(), HttpsState::kNone);
+            String(), HttpsState::kNone));
   }
 
   // This is needed to break a reference cycle in which off-heap
@@ -1479,7 +1464,6 @@ void FrameFetchContext::Trace(blink::Visitor* visitor) {
   visitor->Trace(document_loader_);
   visitor->Trace(document_);
   visitor->Trace(frozen_state_);
-  visitor->Trace(fetch_client_settings_object_);
   BaseFetchContext::Trace(visitor);
 }
 

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

@@ -72,7 +72,7 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
     return CreateFetcher(nullptr, document);
   }
 
-  static void ProvideDocumentToContext(FetchContext&, Document*);
+  void ProvideDocumentToContext(Document*);
 
   FrameFetchContext(DocumentLoader*, Document*);
   ~FrameFetchContext() override;
@@ -150,8 +150,6 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
   bool IsLoadComplete() const override;
   bool UpdateTimingInfoForIFrameNavigation(ResourceTimingInfo*) override;
 
-  const SecurityOrigin* GetSecurityOrigin() const override;
-
   void PopulateResourceRequest(ResourceType,
                                const ClientHintsPreferences&,
                                const FetchParameters::ResourceWidth&,
@@ -209,8 +207,6 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
   CreateResourceLoadingTaskRunnerHandle() override;
 
   // BaseFetchContext overrides:
-  const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const override;
   KURL GetSiteForCookies() const override;
   SubresourceFilter* GetSubresourceFilter() const override;
   PreviewsResourceLoadingHints* GetPreviewsResourceLoadingHints()
@@ -279,8 +275,6 @@ class CORE_EXPORT FrameFetchContext final : public BaseFetchContext {
 
   // Non-null only when detached.
   Member<const FrozenState> frozen_state_;
-
-  Member<FetchClientSettingsObject> fetch_client_settings_object_;
 };
 
 }  // namespace blink

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

@@ -140,7 +140,7 @@ class FrameFetchContextTest : public testing::Test {
     fetch_context =
         static_cast<FrameFetchContext*>(&document->Fetcher()->Context());
     owner = DummyFrameOwner::Create();
-    FrameFetchContext::ProvideDocumentToContext(*fetch_context, document.Get());
+    fetch_context->ProvideDocumentToContext(document.Get());
   }
 
   void TearDown() override {
@@ -158,8 +158,7 @@ class FrameFetchContextTest : public testing::Test {
     child_document = child_frame->GetDocument();
     FrameFetchContext* child_fetch_context = static_cast<FrameFetchContext*>(
         &child_frame->Loader().GetDocumentLoader()->Fetcher()->Context());
-    FrameFetchContext::ProvideDocumentToContext(*child_fetch_context,
-                                                child_document.Get());
+    child_fetch_context->ProvideDocumentToContext(child_document.Get());
     return child_fetch_context;
   }
 
@@ -291,7 +290,7 @@ class FrameFetchContextMockedLocalFrameClientTest
     fetch_context =
         static_cast<FrameFetchContext*>(&document->Fetcher()->Context());
     owner = DummyFrameOwner::Create();
-    FrameFetchContext::ProvideDocumentToContext(*fetch_context, document.Get());
+    fetch_context->ProvideDocumentToContext(document.Get());
   }
 
   KURL url;
@@ -432,7 +431,7 @@ TEST_F(FrameFetchContextModifyRequestTest, UpgradeInsecureResourceRequests) {
        "ftp://example.test:1212/image.png"},
   };
 
-  FrameFetchContext::ProvideDocumentToContext(*fetch_context, document.Get());
+  fetch_context->ProvideDocumentToContext(document.Get());
   document->SetInsecureRequestPolicy(kUpgradeInsecureRequests);
 
   for (const auto& test : tests) {
@@ -480,7 +479,7 @@ TEST_F(FrameFetchContextModifyRequestTest, UpgradeInsecureResourceRequests) {
 
 TEST_F(FrameFetchContextModifyRequestTest,
        DoNotUpgradeInsecureResourceRequests) {
-  FrameFetchContext::ProvideDocumentToContext(*fetch_context, document.Get());
+  fetch_context->ProvideDocumentToContext(document.Get());
   document->SetSecurityOrigin(secure_origin);
   document->SetInsecureRequestPolicy(kLeaveInsecureRequestsAlone);
 
@@ -565,7 +564,7 @@ TEST_F(FrameFetchContextModifyRequestTest, SendUpgradeInsecureRequestHeader) {
                                        test.should_prefer);
   }
 
-  FrameFetchContext::ProvideDocumentToContext(*fetch_context, document.Get());
+  fetch_context->ProvideDocumentToContext(document.Get());
 
   for (const auto& test : tests) {
     document->SetInsecureRequestPolicy(kLeaveInsecureRequestsAlone);

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

@@ -19,7 +19,6 @@
 #include "third_party/blink/renderer/core/loader/mixed_content_checker.h"
 #include "third_party/blink/renderer/core/loader/subresource_filter.h"
 #include "third_party/blink/renderer/core/probe/core_probes.h"
-#include "third_party/blink/renderer/core/script/fetch_client_settings_object_impl.h"
 #include "third_party/blink/renderer/core/timing/worker_global_scope_performance.h"
 #include "third_party/blink/renderer/core/workers/worker_clients.h"
 #include "third_party/blink/renderer/core/workers/worker_content_settings_client.h"
@@ -57,15 +56,10 @@ WorkerFetchContext::WorkerFetchContext(
       global_scope_(global_scope),
       web_context_(std::move(web_context)),
       subresource_filter_(subresource_filter),
-      fetch_client_settings_object_(fetch_client_settings_object),
       save_data_enabled_(GetNetworkStateNotifier().SaveDataEnabled()) {
   DCHECK(global_scope.IsContextThread());
   DCHECK(web_context_);
-}
-
-const FetchClientSettingsObject*
-WorkerFetchContext::GetFetchClientSettingsObject() const {
-  return fetch_client_settings_object_.Get();
+  SetFetchClientSettingsObject(fetch_client_settings_object);
 }
 
 KURL WorkerFetchContext::GetSiteForCookies() const {
@@ -191,10 +185,6 @@ void WorkerFetchContext::AddConsoleMessage(ConsoleMessage* message) const {
   return global_scope_->AddConsoleMessage(message);
 }
 
-const SecurityOrigin* WorkerFetchContext::GetSecurityOrigin() const {
-  return GetFetchClientSettingsObject()->GetSecurityOrigin();
-}
-
 std::unique_ptr<WebURLLoader> WorkerFetchContext::CreateURLLoader(
     const ResourceRequest& request,
     const ResourceLoaderOptions& options) {
@@ -393,7 +383,6 @@ WorkerFetchContext::GetWorkerContentSettingsClient() const {
 void WorkerFetchContext::Trace(blink::Visitor* visitor) {
   visitor->Trace(global_scope_);
   visitor->Trace(subresource_filter_);
-  visitor->Trace(fetch_client_settings_object_);
   BaseFetchContext::Trace(visitor);
 }
 

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

@@ -40,8 +40,6 @@ class WorkerFetchContext final : public BaseFetchContext {
   ~WorkerFetchContext() override;
 
   // BaseFetchContext implementation:
-  const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const override;
   KURL GetSiteForCookies() const override;
   SubresourceFilter* GetSubresourceFilter() const override;
   PreviewsResourceLoadingHints* GetPreviewsResourceLoadingHints()
@@ -75,7 +73,6 @@ class WorkerFetchContext final : public BaseFetchContext {
   void AddConsoleMessage(ConsoleMessage*) const override;
 
   // FetchContext implementation:
-  const SecurityOrigin* GetSecurityOrigin() const override;
   std::unique_ptr<WebURLLoader> CreateURLLoader(
       const ResourceRequest&,
       const ResourceLoaderOptions&) override;
@@ -139,8 +136,6 @@ class WorkerFetchContext final : public BaseFetchContext {
   const scoped_refptr<WebWorkerFetchContext> web_context_;
   Member<SubresourceFilter> subresource_filter_;
 
-  const Member<FetchClientSettingsObject> fetch_client_settings_object_;
-
   // The value of |save_data_enabled_| is read once per frame from
   // NetworkStateNotifier, which is guarded by a mutex lock, and cached locally
   // here for performance.

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

@@ -42,30 +42,16 @@ class NullFetchContext final : public FetchContext {
  public:
   explicit NullFetchContext(
       scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-      : FetchContext(std::move(task_runner)),
-        fetch_client_settings_object_(
-            MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
-                KURL(),
-                nullptr /* security_origin */,
-                network::mojom::ReferrerPolicy::kDefault,
-                String(),
-                HttpsState::kNone)) {}
-
-  const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const override {
-    return fetch_client_settings_object_;
+      : FetchContext(std::move(task_runner)) {
+    SetFetchClientSettingsObject(
+        MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
+            KURL(), nullptr /* security_origin */,
+            network::mojom::ReferrerPolicy::kDefault, String(),
+            HttpsState::kNone));
   }
 
   void CountUsage(mojom::WebFeature) const override {}
   void CountDeprecation(mojom::WebFeature) const override {}
-
-  void Trace(blink::Visitor* visitor) override {
-    visitor->Trace(fetch_client_settings_object_);
-    FetchContext::Trace(visitor);
-  }
-
- private:
-  const Member<const FetchClientSettingsObject> fetch_client_settings_object_;
 };
 
 }  // namespace
@@ -85,6 +71,7 @@ FetchContext::FetchContext(
 
 void FetchContext::Trace(blink::Visitor* visitor) {
   visitor->Trace(platform_probe_sink_);
+  visitor->Trace(fetch_client_settings_object_);
 }
 
 void FetchContext::DispatchDidChangeResourcePriority(unsigned long,
@@ -158,6 +145,27 @@ void FetchContext::AddWarningConsoleMessage(const String&, LogSource) const {}
 
 void FetchContext::AddErrorConsoleMessage(const String&, LogSource) const {}
 
+const SecurityOrigin* FetchContext::GetSecurityOrigin() const {
+  // This can be called before |fetch_client_settings_object_| is set in
+  // FrameFetchContext.
+  // TODO(hiroshige): Make |fetch_client_settings_object_| always non-null.
+  if (!fetch_client_settings_object_)
+    return nullptr;
+  return fetch_client_settings_object_->GetSecurityOrigin();
+}
+
+void FetchContext::SetFetchClientSettingsObject(
+    FetchClientSettingsObject* fetch_client_settings_object) {
+  DCHECK(fetch_client_settings_object);
+  fetch_client_settings_object_ = fetch_client_settings_object;
+}
+
+const FetchClientSettingsObject* FetchContext::GetFetchClientSettingsObject()
+    const {
+  DCHECK(fetch_client_settings_object_);
+  return fetch_client_settings_object_.Get();
+}
+
 void FetchContext::PopulateResourceRequest(
     ResourceType,
     const ClientHintsPreferences&,

third_party/blink/renderer/platform/loader/fetch/fetch_context.h

@@ -86,6 +86,9 @@ class PLATFORM_EXPORT FetchContext
   WTF_MAKE_NONCOPYABLE(FetchContext);
 
  public:
+  explicit FetchContext(
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+
   // This enum corresponds to blink::MessageSource. We have this not to
   // introduce any dependency to core/.
   //
@@ -109,8 +112,7 @@ class PLATFORM_EXPORT FetchContext
 
   virtual void AddAdditionalRequestHeaders(ResourceRequest&, FetchResourceType);
 
-  virtual const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const = 0;
+  const FetchClientSettingsObject* GetFetchClientSettingsObject() const;
 
   // Called when the ResourceFetcher observes a data: URI load that contains an
   // octothorpe ('#') character. This is a temporary method to support an Intent
@@ -228,7 +230,7 @@ class PLATFORM_EXPORT FetchContext
   virtual void CountUsage(mojom::WebFeature) const = 0;
   virtual void CountDeprecation(mojom::WebFeature) const = 0;
 
-  virtual const SecurityOrigin* GetSecurityOrigin() const { return nullptr; }
+  const SecurityOrigin* GetSecurityOrigin() const;
 
   // Populates the ResourceRequest using the given values and information
   // stored in the FetchContext implementation. Used by ResourceFetcher to
@@ -312,12 +314,12 @@ class PLATFORM_EXPORT FetchContext
   virtual void DispatchNetworkQuiet() {}
 
  protected:
-  explicit FetchContext(
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+  void SetFetchClientSettingsObject(FetchClientSettingsObject*);
 
  private:
   Member<PlatformProbeSink> platform_probe_sink_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  Member<FetchClientSettingsObject> fetch_client_settings_object_;
 };
 
 }  // namespace blink

third_party/blink/renderer/platform/loader/testing/mock_fetch_context.h

@@ -50,11 +50,12 @@ class MockFetchContext : public FetchContext {
                          ? std::move(loading_task_runner)
                          : base::MakeRefCounted<scheduler::FakeTaskRunner>()),
         load_policy_(load_policy),
-        security_origin_(SecurityOrigin::CreateUniqueOpaque()),
         frame_scheduler_(new MockFrameScheduler(GetLoadingTaskRunner())),
         url_loader_factory_(std::move(url_loader_factory)),
         complete_(false),
-        transfer_size_(-1) {}
+        transfer_size_(-1) {
+    SetSecurityOrigin(SecurityOrigin::CreateUniqueOpaque());
+  }
   ~MockFetchContext() override = default;
 
   void SetLoadComplete(bool complete) { complete_ = complete; }
@@ -63,19 +64,12 @@ class MockFetchContext : public FetchContext {
   void CountUsage(mojom::WebFeature) const override {}
   void CountDeprecation(mojom::WebFeature) const override {}
 
-  const SecurityOrigin* GetSecurityOrigin() const override {
-    return security_origin_.get();
-  }
-
   void SetSecurityOrigin(scoped_refptr<const SecurityOrigin> security_origin) {
-    security_origin_ = security_origin;
-  }
-
-  const FetchClientSettingsObject* GetFetchClientSettingsObject()
-      const override {
-    return MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
-        KURL(), security_origin_, network::mojom::ReferrerPolicy::kDefault,
-        String(), HttpsState::kNone);
+    SetFetchClientSettingsObject(
+        MakeGarbageCollected<FetchClientSettingsObjectSnapshot>(
+            KURL(), std::move(security_origin),
+            network::mojom::ReferrerPolicy::kDefault, String(),
+            HttpsState::kNone));
   }
 
   // The last ResourceRequest passed to DispatchWillSendRequest.
@@ -164,7 +158,6 @@ class MockFetchContext : public FetchContext {
   };
 
   enum LoadPolicy load_policy_;
-  scoped_refptr<const SecurityOrigin> security_origin_;
   std::unique_ptr<FrameScheduler> frame_scheduler_;
   std::unique_ptr<WebURLLoaderFactory> url_loader_factory_;
   bool complete_;