Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
Committed: https://crrev.com/4623ce800b2050be83f307f351fb5a8addf3e676
Cr-Commit-Position: refs/heads/master@{#370665}

[tfh, 2016-01-19 21:08:15]

Description was changed from

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
==========

to

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
==========

[tfh, 2016-01-19 21:08:16]

tfh@skip.org changed reviewers:
+ eroman@chromium.org

[tfh, 2016-01-19 21:18:12]

Please take a look :)

I also noticed unusual use of a scoped_ptr<CanonicalCookie>* type in a function
signature (below). Glancing at the callsites I think the pointer's unnecessary.
I'm not sure whether it's best to bundle that improvement with this CL, or how
else to submit it, or whether to do so at all, though.

net/cookies/cookie_monster.h:
  bool SetCanonicalCookie(scoped_ptr<CanonicalCookie>* cc,
                          const base::Time& creation_time,
                          const CookieOptions& options);

[eroman, 2016-01-19 21:24:52]

Regarding SetCanonicalCookie(), save that for a different CL.

At first glance it looks like it being a pointer is used in this line:


https://code.google.com/p/chromium/codesearch#chromium/src/net/cookies/cookie...

Which passes ownership

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

The change looksgood, except for a possible bug in the unit-test.

After fixing please send the change to the ASAN trybots for confirmation

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
net/cookies/cookie_monster_unittest.cc:2823: scoped_ptr<CanonicalCookie>
cookie(CanonicalCookie::Create(
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.

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
net/cookies/cookie_monster_unittest.cc:2826: // This cookie will be freed by the
CookieMonster.
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:14:46]

On 2016/01/19 21:36:51, eroman wrote:
> After fixing please send the change to the ASAN trybots for confirmation

Don't *think* I have permission to do this. I could try on my local machine.

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

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
net/cookies/cookie_monster_unittest.cc:2826: // This cookie will be freed by the
CookieMonster.
On 2016/01/19 21:36:51, eroman wrote:
> 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.

> std::vector<CanonicalCookie*> cookies = {new CanonicalCookie(*cookie)};

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]

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/1602283002/diff/1/net/cookies/cookie_monster_...
net/cookies/cookie_monster_unittest.cc:2826: // This cookie will be freed by the
CookieMonster.
On 2016/01/19 23:15:17, tfh wrote:
> On 2016/01/19 21:36:51, eroman wrote:
> > 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.
> 
> > std::vector<CanonicalCookie*> cookies = {new CanonicalCookie(*cookie)};
> 
> 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?

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!

[tfh, 2016-01-20 00:10:32]

The CQ bit was checked by tfh@skip.org

[commit-bot: I haz the power, 2016-01-20 00:13:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1602283002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1602283002/1

[commit-bot: I haz the power, 2016-01-20 00:26:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 00:26:46]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[tfh, 2016-01-20 22:29:23]

The ios trybots found an extra callsite I missed, please take a look :)

[eroman, 2016-01-20 22:31:04]

patchset2 lgtm

[tfh, 2016-01-20 22:37:21]

The CQ bit was checked by tfh@skip.org

[commit-bot: I haz the power, 2016-01-20 22:38:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1602283002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1602283002/20001

[commit-bot: I haz the power, 2016-01-20 22:59:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-20 22:59:48]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tfh, 2016-01-20 23:41:47]

tfh@skip.org changed reviewers:
+ droger@chromium.org

[tfh, 2016-01-20 23:41:47]

Hey droger, please can you check ios/net/cookies/cookie_store_ios_unittest.mm?

(I have no idea if it's possible to resubmit a patchset, newbie here)

[eroman, 2016-01-21 00:01:23]

> (I have no idea if it's possible to resubmit a patchset, newbie here)

Not sure I understand the question. Once you get signoff from droger you can
check the commit box again and it will work.

In some scenarios it is also acceptable to "TBR" mechanical changes to files in
other components. See also:

http://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...

https://www.chromium.org/developers/testing/commit-queue#TOC-Sending-a-TBR-pa...

[droger, 2016-01-21 08:51:53]

lgtm

[tfh, 2016-01-21 10:31:42]

The CQ bit was checked by tfh@skip.org

[commit-bot: I haz the power, 2016-01-21 10:32:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1602283002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1602283002/20001

[commit-bot: I haz the power, 2016-01-21 10:37:44]

Description was changed from

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
==========

to

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
==========

[commit-bot: I haz the power, 2016-01-21 10:37:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-21 10:39:28]

Description was changed from

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898
==========

to

==========
Convert some raw CanonicalCookie ptrs to scoped_ptr<CanonicalCookie>.

BUG=479898

Committed: https://crrev.com/4623ce800b2050be83f307f351fb5a8addf3e676
Cr-Commit-Position: refs/heads/master@{#370665}
==========

[commit-bot: I haz the power, 2016-01-21 10:39:29]

Patchset 2 (id:??) landed as
https://crrev.com/4623ce800b2050be83f307f351fb5a8addf3e676
Cr-Commit-Position: refs/heads/master@{#370665}