Legacy cookie access content settings: Only construct GURL if needed

This change delays the construction of the GURL (representing the
cookie domain, which is used to match against the ContentSettingPatterns
in the Legacy cookie access content setting), until it is determined
that it is actually needed.

In particular, if the content settings for that type do not contain any
domain-specific settings, we don't actually need to do any URL matching
since any URL will match. In these cases, we can skip constructing the
GURL since that can be expensive.

This optimization only works for the network implementation of
CookieSettings, not for the components/content_settings implementation
of CookieSettings.

Bug: 1014715
Change-Id: If6535290671b28811f84922ec05c0be58e00bc6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1865754
Commit-Queue: Lily Chen <chlily@chromium.org>
Reviewed-by: Maksim Orlovich <morlovich@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707179}

components/content_settings/core/browser/cookie_settings.cc

@@ -17,6 +17,7 @@
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_service.h"
 #include "extensions/buildflags/buildflags.h"
+#include "net/cookies/cookie_util.h"
 #include "url/gurl.h"
 
 namespace content_settings {
@@ -122,12 +123,17 @@ bool CookieSettings::IsStorageDurable(const GURL& origin) const {
 }
 
 void CookieSettings::GetSettingForLegacyCookieAccess(
-    const GURL& cookie_domain,
+    const std::string& cookie_domain,
     ContentSetting* setting) const {
   DCHECK(setting);
 
+  // The content setting patterns are treated as domains, not URLs, so the
+  // scheme is irrelevant (so we can just arbitrarily pass false).
+  GURL cookie_domain_url = net::cookie_util::CookieOriginToURL(
+      cookie_domain, false /* secure scheme */);
+
   *setting = host_content_settings_map_->GetContentSetting(
-      cookie_domain, GURL(), CONTENT_SETTINGS_TYPE_LEGACY_COOKIE_ACCESS,
+      cookie_domain_url, GURL(), CONTENT_SETTINGS_TYPE_LEGACY_COOKIE_ACCESS,
       std::string() /* resource_identifier */);
 }
 

components/content_settings/core/browser/cookie_settings.h

@@ -115,7 +115,7 @@ class CookieSettings : public CookieSettingsBase,
   bool ShouldBlockThirdPartyCookies() const;
 
   // content_settings::CookieSettingsBase:
-  void GetSettingForLegacyCookieAccess(const GURL& cookie_domain,
+  void GetSettingForLegacyCookieAccess(const std::string& cookie_domain,
                                        ContentSetting* setting) const override;
 
   // Detaches the |CookieSettings| from |PrefService|. This methods needs to be

components/content_settings/core/browser/cookie_settings_unittest.cc

@@ -18,7 +18,6 @@
 #include "extensions/buildflags/buildflags.h"
 #include "net/base/features.h"
 #include "net/cookies/cookie_constants.h"
-#include "net/cookies/cookie_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -475,27 +474,19 @@ TEST_F(CookieSettingsTest, ThirdPartySettingObserver) {
 TEST_F(CookieSettingsTest, LegacyCookieAccessAllowAll) {
   settings_map_->SetDefaultContentSetting(
       CONTENT_SETTINGS_TYPE_LEGACY_COOKIE_ACCESS, CONTENT_SETTING_ALLOW);
-  EXPECT_EQ(
-      net::CookieAccessSemantics::LEGACY,
-      cookie_settings_->GetCookieAccessSemanticsForDomain(
-          net::cookie_util::CookieOriginToURL(kDomain, true /* is_https */)));
   EXPECT_EQ(net::CookieAccessSemantics::LEGACY,
-            cookie_settings_->GetCookieAccessSemanticsForDomain(
-                net::cookie_util::CookieOriginToURL(kDotDomain,
-                                                    true /* is_https */)));
+            cookie_settings_->GetCookieAccessSemanticsForDomain(kDomain));
+  EXPECT_EQ(net::CookieAccessSemantics::LEGACY,
+            cookie_settings_->GetCookieAccessSemanticsForDomain(kDotDomain));
 }
 
 TEST_F(CookieSettingsTest, LegacyCookieAccessBlockAll) {
   settings_map_->SetDefaultContentSetting(
       CONTENT_SETTINGS_TYPE_LEGACY_COOKIE_ACCESS, CONTENT_SETTING_BLOCK);
-  EXPECT_EQ(
-      net::CookieAccessSemantics::NONLEGACY,
-      cookie_settings_->GetCookieAccessSemanticsForDomain(
-          net::cookie_util::CookieOriginToURL(kDomain, true /* is_https */)));
   EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
-            cookie_settings_->GetCookieAccessSemanticsForDomain(
-                net::cookie_util::CookieOriginToURL(kDotDomain,
-                                                    false /* is_https */)));
+            cookie_settings_->GetCookieAccessSemanticsForDomain(kDomain));
+  EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
+            cookie_settings_->GetCookieAccessSemanticsForDomain(kDotDomain));
 }
 
 // Test SameSite-by-default disabled (default semantics is LEGACY)
@@ -525,11 +516,7 @@ TEST_F(CookieSettingsTest,
       {net::CookieAccessSemantics::LEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
     EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+                               test.cookie_domain));
   }
 }
 
@@ -560,11 +547,7 @@ TEST_F(CookieSettingsTest,
       {net::CookieAccessSemantics::LEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
     EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+                               test.cookie_domain));
   }
 }
 
@@ -606,11 +589,7 @@ TEST_F(SameSiteByDefaultCookieSettingsTest,
       {net::CookieAccessSemantics::NONLEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
     EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+                               test.cookie_domain));
   }
 }
 
@@ -640,11 +619,7 @@ TEST_F(SameSiteByDefaultCookieSettingsTest,
       {net::CookieAccessSemantics::NONLEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
     EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, cookie_settings_->GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+                               test.cookie_domain));
   }
 }
 

components/content_settings/core/common/cookie_settings_base.cc

@@ -110,7 +110,7 @@ bool CookieSettingsBase::IsCookieSessionOnly(const GURL& origin) const {
 
 net::CookieAccessSemantics
 CookieSettingsBase::GetCookieAccessSemanticsForDomain(
-    const GURL& cookie_domain) const {
+    const std::string& cookie_domain) const {
   ContentSetting setting;
   GetSettingForLegacyCookieAccess(cookie_domain, &setting);
   DCHECK(IsValidSettingForLegacyAccess(setting));

components/content_settings/core/common/cookie_settings_base.h

@@ -108,16 +108,20 @@ class CookieSettingsBase {
                         ContentSetting* cookie_setting) const;
 
   // Returns the cookie access semantics (legacy or nonlegacy) to be applied for
-  // cookies on the given domain.
+  // cookies on the given domain. The |cookie_domain| can be provided as the
+  // direct output of CanonicalCookie::Domain(), i.e. any leading dot does not
+  // have to be removed.
   //
   // This may be called on any thread.
   net::CookieAccessSemantics GetCookieAccessSemanticsForDomain(
-      const GURL& cookie_domain) const;
+      const std::string& cookie_domain) const;
 
   // Gets the setting that controls whether legacy access is allowed for a given
-  // cookie domain (provided as a URL).
+  // cookie domain. The |cookie_domain| can be provided as the direct output of
+  // CanonicalCookie::Domain(), i.e. any leading dot does not have to be
+  // removed.
   virtual void GetSettingForLegacyCookieAccess(
-      const GURL& cookie_domain,
+      const std::string& cookie_domain,
       ContentSetting* setting) const = 0;
 
   // Determines whether |setting| is a valid content setting for cookies.

components/content_settings/core/common/cookie_settings_base_unittest.cc

@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "net/cookies/cookie_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -43,9 +44,11 @@ class CallbackCookieSettings : public CookieSettingsBase {
                                 ContentSetting* cookie_setting) const override {
     *cookie_setting = callback_.Run(url);
   }
-  void GetSettingForLegacyCookieAccess(const GURL& cookie_domain,
+  void GetSettingForLegacyCookieAccess(const std::string& cookie_domain,
                                        ContentSetting* setting) const override {
-    *setting = callback_.Run(cookie_domain);
+    GURL cookie_domain_url =
+        net::cookie_util::CookieOriginToURL(cookie_domain, false);
+    *setting = callback_.Run(cookie_domain_url);
   }
 
  private:
@@ -148,11 +151,11 @@ TEST(CookieSettingsBaseTest, LegacyCookieAccessSemantics) {
   CallbackCookieSettings settings1(
       base::BindRepeating([](const GURL&) { return CONTENT_SETTING_ALLOW; }));
   EXPECT_EQ(net::CookieAccessSemantics::LEGACY,
-            settings1.GetCookieAccessSemanticsForDomain(GURL()));
+            settings1.GetCookieAccessSemanticsForDomain(std::string()));
   CallbackCookieSettings settings2(
       base::BindRepeating([](const GURL&) { return CONTENT_SETTING_BLOCK; }));
   EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
-            settings2.GetCookieAccessSemanticsForDomain(GURL()));
+            settings2.GetCookieAccessSemanticsForDomain(std::string()));
 }
 
 TEST(CookieSettingsBaseTest, IsCookieSessionOnlyWithAllowSetting) {

services/network/cookie_access_delegate_impl.cc

@@ -16,9 +16,7 @@ CookieAccessDelegateImpl::~CookieAccessDelegateImpl() = default;
 
 net::CookieAccessSemantics CookieAccessDelegateImpl::GetAccessSemantics(
     const net::CanonicalCookie& cookie) const {
-  GURL cookie_domain =
-      net::cookie_util::CookieOriginToURL(cookie.Domain(), cookie.IsSecure());
-  return cookie_settings_->GetCookieAccessSemanticsForDomain(cookie_domain);
+  return cookie_settings_->GetCookieAccessSemanticsForDomain(cookie.Domain());
 }
 
 }  // namespace network

services/network/cookie_settings.cc

@@ -32,7 +32,7 @@ CookieSettings::CreateDeleteCookieOnExitPredicate() const {
 }
 
 void CookieSettings::GetSettingForLegacyCookieAccess(
-    const GURL& cookie_domain,
+    const std::string& cookie_domain,
     ContentSetting* setting) const {
   DCHECK(setting);
 
@@ -41,8 +41,36 @@ void CookieSettings::GetSettingForLegacyCookieAccess(
                  ? CONTENT_SETTING_BLOCK
                  : CONTENT_SETTING_ALLOW;
 
+  if (settings_for_legacy_cookie_access_.empty())
+    return;
+
+  // If there are no domain-specific settings, return early to avoid the cost of
+  // constructing a GURL to match against.
+  bool has_non_wildcard_setting = false;
+  for (const auto& entry : settings_for_legacy_cookie_access_) {
+    if (!entry.primary_pattern.MatchesAllHosts()) {
+      has_non_wildcard_setting = true;
+      break;
+    }
+  }
+  if (!has_non_wildcard_setting) {
+    // Take the first entry because we know all entries match any host.
+    *setting = settings_for_legacy_cookie_access_[0].GetContentSetting();
+    DCHECK(IsValidSettingForLegacyAccess(*setting));
+    return;
+  }
+
+  // The content setting patterns are treated as domains, not URLs, so the
+  // scheme is irrelevant (so we can just arbitrarily pass false).
+  GURL cookie_domain_url = net::cookie_util::CookieOriginToURL(
+      cookie_domain, false /* secure scheme */);
+
   for (const auto& entry : settings_for_legacy_cookie_access_) {
-    if (entry.primary_pattern.Matches(cookie_domain)) {
+    // TODO(crbug.com/1015611): This should ignore scheme and port, but
+    // currently takes them into account. It says in the policy description that
+    // specifying a scheme or port in the pattern may lead to undefined
+    // behavior, but this is not ideal.
+    if (entry.primary_pattern.Matches(cookie_domain_url)) {
       *setting = entry.GetContentSetting();
       DCHECK(IsValidSettingForLegacyAccess(*setting));
       return;

services/network/cookie_settings.h

@@ -65,7 +65,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CookieSettings
   CreateDeleteCookieOnExitPredicate() const;
 
   // content_settings::CookieSettingsBase:
-  void GetSettingForLegacyCookieAccess(const GURL& cookie_domain,
+  void GetSettingForLegacyCookieAccess(const std::string& cookie_domain,
                                        ContentSetting* setting) const override;
 
  private:

services/network/cookie_settings_unittest.cc

@@ -198,17 +198,15 @@ TEST(CookieSettingsTest, GetCookieSettingMatchingSchemeCookiesAllowed) {
 TEST(CookieSettingsTest, LegacyCookieAccessDefault) {
   CookieSettings settings;
   ContentSetting setting;
-  GURL cookie_domain =
-      net::cookie_util::CookieOriginToURL(kDomain, true /* is_https */);
 
   // Test SameSite-by-default enabled (default semantics is NONLEGACY)
   {
     base::test::ScopedFeatureList feature_list;
     feature_list.InitAndEnableFeature(net::features::kSameSiteByDefaultCookies);
-    settings.GetSettingForLegacyCookieAccess(cookie_domain, &setting);
+    settings.GetSettingForLegacyCookieAccess(kDomain, &setting);
     EXPECT_EQ(setting, CONTENT_SETTING_BLOCK);
     EXPECT_EQ(net::CookieAccessSemantics::NONLEGACY,
-              settings.GetCookieAccessSemanticsForDomain(cookie_domain));
+              settings.GetCookieAccessSemanticsForDomain(kDomain));
   }
 
   // Test SameSite-by-default disabled (default semantics is LEGACY)
@@ -217,10 +215,10 @@ TEST(CookieSettingsTest, LegacyCookieAccessDefault) {
     base::test::ScopedFeatureList feature_list;
     feature_list.InitAndDisableFeature(
         net::features::kSameSiteByDefaultCookies);
-    settings.GetSettingForLegacyCookieAccess(cookie_domain, &setting);
+    settings.GetSettingForLegacyCookieAccess(kDomain, &setting);
     EXPECT_EQ(setting, CONTENT_SETTING_ALLOW);
     EXPECT_EQ(net::CookieAccessSemantics::LEGACY,
-              settings.GetCookieAccessSemanticsForDomain(cookie_domain));
+              settings.GetCookieAccessSemanticsForDomain(kDomain));
   }
 }
 
@@ -245,12 +243,8 @@ TEST(CookieSettingsTest,
       {net::CookieAccessSemantics::LEGACY, kSubDomain},
       {net::CookieAccessSemantics::LEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+    EXPECT_EQ(test.status,
+              settings.GetCookieAccessSemanticsForDomain(test.cookie_domain));
   }
 }
 
@@ -274,12 +268,8 @@ TEST(CookieSettingsTest,
       {net::CookieAccessSemantics::NONLEGACY, kSubDomain},
       {net::CookieAccessSemantics::NONLEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+    EXPECT_EQ(test.status,
+              settings.GetCookieAccessSemanticsForDomain(test.cookie_domain));
   }
 }
 
@@ -304,12 +294,8 @@ TEST(CookieSettingsTest,
       // This test case defaults into LEGACY.
       {net::CookieAccessSemantics::LEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+    EXPECT_EQ(test.status,
+              settings.GetCookieAccessSemanticsForDomain(test.cookie_domain));
   }
 }
 
@@ -333,12 +319,8 @@ TEST(CookieSettingsTest,
       // This test case defaults into NONLEGACY.
       {net::CookieAccessSemantics::NONLEGACY, kOtherDomain}};
   for (const auto& test : kTestCases) {
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, true /* is_https */)));
-    EXPECT_EQ(test.status, settings.GetCookieAccessSemanticsForDomain(
-                               net::cookie_util::CookieOriginToURL(
-                                   test.cookie_domain, false /* is_https */)));
+    EXPECT_EQ(test.status,
+              settings.GetCookieAccessSemanticsForDomain(test.cookie_domain));
   }
 }