Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

net/cookies/cookie_monster_unittest.cc

Index: net/cookies/cookie_monster_unittest.cc
diff --git a/net/cookies/cookie_monster_unittest.cc b/net/cookies/cookie_monster_unittest.cc
index efeadd96d5855e84caf83a96005b059d9249e8cb..4097dfe85c9eaa3efa23ef0e0766da928692608a 100644
--- a/net/cookies/cookie_monster_unittest.cc
+++ b/net/cookies/cookie_monster_unittest.cc
@@ -2820,11 +2820,11 @@ TEST_F(MultiThreadedCookieMonsterTest, ThreadCheckDeleteCanonicalCookie) {
 // MultiThreadedCookieMonsterTest in order to test and use
 // GetAllCookiesForURLAsync, but it does not use any additional threads.
 TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) {
-  std::vector<CanonicalCookie*> cookies;
-  // This cookie will be freed by the CookieMonster.
-  cookies.push_back(CanonicalCookie::Create(
+  scoped_ptr<CanonicalCookie> cookie(CanonicalCookie::Create(

[eroman, 2016/01/19 21:36:51]

I believe this is going to result in a double free, per the comment below.

I think instead you should keep the current semantics, and keep a raw pointer in
the std::vector<> since the other code expects that.

       http_www_google_.url(), kValidCookieLine, Time::Now(), CookieOptions()));
-  CanonicalCookie cookie = *cookies[0];
+
+  // This cookie will be freed by the CookieMonster.

[eroman, 2016/01/19 21:36:51]

This comment is key -- if it is correct (and it must be for the test not to have
been leaking before), then that implies CookieMonster will already delete this
pointer, and so |cookie| should not be a scoped_ptr<> and also delete it.

[tfh, 2016/01/19 23:15:17]

I don't immediately see why this is the case, the new CanonicalCookie(*cookie)
is a separate CanonicalCookie, default copy constructed from the first |cookie|
(which CanonicalCookie permits). So the CookieMonster still has its own
CanonicalCookie to delete, no?

[eroman, 2016/01/19 23:35:54]

Ah I misread, it does indeed look OK.

(For some reason I thought the cookies std::vector was holding a pointer to
|cookie| above, but indeed it is a copy).

LGTM!

+  std::vector<CanonicalCookie*> cookies = {new CanonicalCookie(*cookie)};
   scoped_refptr<NewMockPersistentCookieStore> store(
       new NewMockPersistentCookieStore);
   scoped_refptr<CookieMonster> cm(new CookieMonster(store.get(), NULL));
@@ -2856,7 +2856,7 @@ TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) {
   ASSERT_TRUE(callback.did_run());
   // See that the callback was called with the cookies.
   ASSERT_EQ(1u, callback.cookies().size());
-  EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+  EXPECT_TRUE(cookie->IsEquivalent(callback.cookies()[0]));
 
   // All urls in |urls| should share the same cookie domain.
   const GURL kUrls[] = {
@@ -2870,7 +2870,7 @@ TEST_F(MultiThreadedCookieMonsterTest, GetAllCookiesForURLEffectiveDomain) {
     GetAllCookiesForURLTask(cm.get(), url, &callback);
     ASSERT_TRUE(callback.did_run());
     ASSERT_EQ(1u, callback.cookies().size());
-    EXPECT_TRUE(cookie.IsEquivalent(callback.cookies()[0]));
+    EXPECT_TRUE(cookie->IsEquivalent(callback.cookies()[0]));
   }
 }