Fix CHECK failure that equivalent cookies are not found

If strict secure cookies is enabled, if a cookie is set that matches an
already existing secure cookie, it will mark the insertion as having
found an equivalent cookie. Unfortunately, this is not entirely
accurate, as an "equivalent cookie" more strictly means the domain and
path attributes match as well. Thus, this inadvertently can cause a
failure of a CHECK that verifies that there are not equivalent cookies
in the store.

The fix for this is to just change the strict secure portion of the
matching to only set the insertion as having found equivalent cookies if
it passes an additional IsEquivalent() check.

BUG=569943

Review URL: https://codereview.chromium.org/1525073003

Cr-Commit-Position: refs/heads/master@{#365557}

net/cookies/cookie_monster.cc

@@ -1830,7 +1830,8 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
       // TODO(jww): We need to add metrics here before we add this as a Finch
       // experiment, as our current Cookie.CookieSourceScheme and related
       // metrics make very different assumptions from what this now means.
-      found_equivalent_cookie = true;
+      if (ecc.IsEquivalent(*cc))
+        found_equivalent_cookie = true;
     } else if (ecc.IsEquivalent(*cc)) {
       // We should never have more than one equivalent cookie, since they should
       // overwrite each other, unless secure cookies require secure scheme is

net/cookies/cookie_monster.h

@@ -365,8 +365,7 @@ class NET_EXPORT CookieMonster : public CookieStore {
   FRIEND_TEST_ALL_PREFIXES(CookieMonsterTest, CookieSourceHistogram);
 
   // For kSafeFromGlobalPurgeDays in CookieStore
-  FRIEND_TEST_ALL_PREFIXES(CookieMonsterSecureCookiesRequireSecureSchemeTest,
-                           EvictSecureCookies);
+  FRIEND_TEST_ALL_PREFIXES(CookieMonsterStrictSecureTest, EvictSecureCookies);
 
   // Internal reasons for deletion, used to populate informative histograms
   // and to provide a public cause for onCookieChange notifications.

net/cookies/cookie_monster_unittest.cc

@@ -126,7 +126,7 @@ INSTANTIATE_TYPED_TEST_CASE_P(CookieMonster,
                               MultiThreadedCookieStoreTest,
                               CookieMonsterTestTraits);
 
-INSTANTIATE_TYPED_TEST_CASE_P(CookieMonsterSecureCookiesRequireSecureScheme,
+INSTANTIATE_TYPED_TEST_CASE_P(CookieMonsterStrictSecure,
                               CookieStoreTest,
                               CookieMonsterEnforcingStrictSecure);
 
@@ -667,7 +667,7 @@ class CookieMonsterTestBase : public CookieStoreTest<T> {
 };
 
 using CookieMonsterTest = CookieMonsterTestBase<CookieMonsterTestTraits>;
-using CookieMonsterSecureCookiesRequireSecureSchemeTest =
+using CookieMonsterStrictSecureTest =
     CookieMonsterTestBase<CookieMonsterEnforcingStrictSecure>;
 
 // TODO(erikwright): Replace the other callbacks and synchronous helper methods
@@ -3063,7 +3063,7 @@ TEST_F(CookieMonsterTest, CookieSourceHistogram) {
       CookieMonster::COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME, 1);
 }
 
-TEST_F(CookieMonsterSecureCookiesRequireSecureSchemeTest, SetSecureCookies) {
+TEST_F(CookieMonsterStrictSecureTest, SetSecureCookies) {
   scoped_refptr<CookieMonster> cm(new CookieMonster(NULL, NULL));
   GURL http_url("http://www.google.com");
   GURL http_superdomain_url("http://google.com");
@@ -3129,7 +3129,7 @@ TEST_F(CookieMonsterSecureCookiesRequireSecureSchemeTest, SetSecureCookies) {
 }
 
 // Tests for behavior if strict secure cookies is enabled.
-TEST_F(CookieMonsterSecureCookiesRequireSecureSchemeTest, EvictSecureCookies) {
+TEST_F(CookieMonsterStrictSecureTest, EvictSecureCookies) {
   // Hard-coding limits in the test, but use DCHECK_EQ to enforce constraint.
   DCHECK_EQ(180U, CookieMonster::kDomainMaxCookies);
   DCHECK_EQ(150U, CookieMonster::kDomainMaxCookies -
@@ -3254,6 +3254,27 @@ TEST_F(CookieMonsterSecureCookiesRequireSecureSchemeTest, EvictSecureCookies) {
                            &test14_alt_hosts);
 }
 
+// Tests that strict secure cookies doesn't trip equivalent cookie checks
+// accidentally. Regression test for https://crbug.com/569943.
+TEST_F(CookieMonsterStrictSecureTest, EquivalentCookies) {
+  scoped_refptr<CookieMonster> cm(new CookieMonster(NULL, NULL));
+  GURL http_url("http://www.google.com");
+  GURL http_superdomain_url("http://google.com");
+  GURL https_url("https://www.google.com");
+
+  // Tests that non-equivalent cookies because of the path attribute can be set
+  // successfully.
+  EXPECT_TRUE(SetCookie(cm.get(), https_url, "A=B; Secure"));
+  EXPECT_TRUE(SetCookie(cm.get(), https_url, "A=C; path=/some/other/path"));
+  EXPECT_FALSE(SetCookie(cm.get(), http_url, "A=D; path=/some/other/path"));
+
+  // Tests that non-equivalent cookies because of the domain attribute can be
+  // set successfully.
+  EXPECT_TRUE(SetCookie(cm.get(), https_url, "A=B; Secure"));
+  EXPECT_TRUE(SetCookie(cm.get(), https_url, "A=C; domain=google.com"));
+  EXPECT_FALSE(SetCookie(cm.get(), http_url, "A=D; domain=google.com"));
+}
+
 class CookieMonsterNotificationTest : public CookieMonsterTest {
  public:
   CookieMonsterNotificationTest()