Call reset(nullptr) in scoped_ptr's.

Call reset(nullptr) in scoped_ptr's destructor.

This behaviour is consistent with libc++.

The original goal of this change is to make Chrome crash more often
and closer to the source when there's a use-after-free bug that
accesses a destroyed scoped_ptr.

BUG=535321
Committed: https://crrev.com/cf9db6ef148d1493dbf6bdc6693f9c79e3ac2afc
Cr-Commit-Position: refs/heads/master@{#357297}

Committed: https://crrev.com/7fa14e3f35b1be8476a2ac61cbad7f3e05a22a18
Cr-Commit-Position: refs/heads/master@{#357756}

[Anand Mistry (off Chromium), 2015-09-17 04:33:25]

amistry@chromium.org changed reviewers:
+ danakj@chromium.org

[Anand Mistry (off Chromium), 2015-09-17 04:33:25]

This might be a bit contentious, but I'm sending it out anyways. Hopefully, this
change will make Chrome more crashy when there's a use-after-free in some cases.

[danakj, 2015-09-17 17:48:48]

What does unique_ptr do? We should match that, since we'll be switching to use
it soon.

[Anand Mistry (off Chromium), 2015-09-18 00:12:26]

On 2015/09/17 17:48:48, danakj wrote:
> What does unique_ptr do? We should match that, since we'll be switching to use
> it soon.

I was expecting this to very much be in the neighbourhood of undefined
behaviour. The spec (I read
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf) doesn't
appear to define this under unique_ptr. But both libc++ and gcc (4.8) set this
to pointer() (which appears to be nullptr based on my reading of the spec and )
which is the default value of unique_ptr::reset() (libc++ calls reset() in the
destructor, libstdc++ sets the value explicitly).

So this change appears to be consistent with two implementations of the STL. I
haven't checked MSVC.

[danakj, 2015-09-21 21:40:52]

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
In libc++ they null the member then call the deleter on a temp var:
https://github.com/llvm-mirror/libcxx/blob/master/include/memory

Shall we do the same?

[danakj, 2015-09-21 21:43:08]

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
On 2015/09/21 21:40:52, danakj wrote:
> In libc++ they null the member then call the deleter on a temp var:
> https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> 
> Shall we do the same?

I guess that is because they reset() in the destructor instead of using another
destructor. Perhaps we could call reset() in the scoped_ptr destructor and
DCHECK that it's null here instead?

Up to you.

[Anand Mistry (off Chromium), 2015-09-22 01:13:53]

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
On 2015/09/21 21:43:08, danakj wrote:
> On 2015/09/21 21:40:52, danakj wrote:
> > In libc++ they null the member then call the deleter on a temp var:
> > https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> > 
> > Shall we do the same?
> 
> I guess that is because they reset() in the destructor instead of using
another
> destructor. Perhaps we could call reset() in the scoped_ptr destructor and
> DCHECK that it's null here instead?
> 
> Up to you.

Since we're in the territory of undefined behaviour (read [class.cdtor]), I
think it's better to be explicit and well commented, so I'd like to keep the
code as-is and not call reset(). Even though this is logically equivalent to
calling reset(), I think some of the nuance of making this explicit might be
lost.

[Anand Mistry (off Chromium), 2015-09-22 01:14:09]

The CQ bit was checked by amistry@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 01:14:47]

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

[tapted, 2015-09-22 02:53:10]

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
On 2015/09/22 01:13:53, Anand Mistry wrote:
> On 2015/09/21 21:43:08, danakj wrote:
> > On 2015/09/21 21:40:52, danakj wrote:
> > > In libc++ they null the member then call the deleter on a temp var:
> > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> > > 
> > > Shall we do the same?
> > 
> > I guess that is because they reset() in the destructor instead of using
> another
> > destructor. Perhaps we could call reset() in the scoped_ptr destructor and
> > DCHECK that it's null here instead?
> > 
> > Up to you.
> 
> Since we're in the territory of undefined behaviour (read [class.cdtor]), I
> think it's better to be explicit and well commented, so I'd like to keep the
> code as-is and not call reset(). Even though this is logically equivalent to
> calling reset(), I think some of the nuance of making this explicit might be
> lost.

(drive-by): the null-before-delete strategy is to handle cycles - the call to
delete above could conceivably destroy |this|, making the `data_.ptr = nullptr`
step a UAF.

However, the way to break a cycle is with reset() and reset() already has a
null-before-delete. I couldn't contrive an example where a loop is created AND
that loop can be broken via the destructor rather than with reset()  (I think
the only way to do this would be to semantically have a double-free, and trying
to cater for it might turn a semantic double-free into a "safe-but-not-really"
`delete nullptr`).

So I don't *think* null-before-delete is needed here, and one could probably
argue that for performance reasons it should be skipped. But there might be
something I've missed.

[Anand Mistry (off Chromium), 2015-09-22 03:14:15]

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
On 2015/09/22 02:53:09, tapted wrote:
> On 2015/09/22 01:13:53, Anand Mistry wrote:
> > On 2015/09/21 21:43:08, danakj wrote:
> > > On 2015/09/21 21:40:52, danakj wrote:
> > > > In libc++ they null the member then call the deleter on a temp var:
> > > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> > > > 
> > > > Shall we do the same?
> > > 
> > > I guess that is because they reset() in the destructor instead of using
> > another
> > > destructor. Perhaps we could call reset() in the scoped_ptr destructor and
> > > DCHECK that it's null here instead?
> > > 
> > > Up to you.
> > 
> > Since we're in the territory of undefined behaviour (read [class.cdtor]), I
> > think it's better to be explicit and well commented, so I'd like to keep the
> > code as-is and not call reset(). Even though this is logically equivalent to
> > calling reset(), I think some of the nuance of making this explicit might be
> > lost.
> 
> (drive-by): the null-before-delete strategy is to handle cycles - the call to
> delete above could conceivably destroy |this|, making the `data_.ptr =
nullptr`
> step a UAF.

This is the defined behaviour for unique_ptr::reset for exactly the reason you
state.

> 
> However, the way to break a cycle is with reset() and reset() already has a
> null-before-delete. I couldn't contrive an example where a loop is created AND
> that loop can be broken via the destructor rather than with reset()  (I think
> the only way to do this would be to semantically have a double-free, and
trying
> to cater for it might turn a semantic double-free into a "safe-but-not-really"
> `delete nullptr`).
> 
> So I don't *think* null-before-delete is needed here, and one could probably
> argue that for performance reasons it should be skipped. But there might be
> something I've missed.

But we're in the destructor here. Deleting |this| in the destructor is even more
in the realm of undefined behaviour and itself a significant bug. If you run
into a situation where the object's deleter deletes |this|, you also potentially
have a double-free of |this|, aside from the potential use-after-free that is
being introduced.

[commit-bot: I haz the power, 2015-09-22 04:02:07]

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

[commit-bot: I haz the power, 2015-09-22 04:02:08]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[danakj, 2015-09-22 18:01:31]

On 2015/09/22 02:53:10, tapted wrote:
> https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
> File base/memory/scoped_ptr.h (right):
> 
>
https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
> base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
> On 2015/09/22 01:13:53, Anand Mistry wrote:
> > On 2015/09/21 21:43:08, danakj wrote:
> > > On 2015/09/21 21:40:52, danakj wrote:
> > > > In libc++ they null the member then call the deleter on a temp var:
> > > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> > > > 
> > > > Shall we do the same?
> > > 
> > > I guess that is because they reset() in the destructor instead of using
> > another
> > > destructor. Perhaps we could call reset() in the scoped_ptr destructor and
> > > DCHECK that it's null here instead?
> > > 
> > > Up to you.
> > 
> > Since we're in the territory of undefined behaviour (read [class.cdtor]), I
> > think it's better to be explicit and well commented, so I'd like to keep the
> > code as-is and not call reset(). Even though this is logically equivalent to
> > calling reset(), I think some of the nuance of making this explicit might be
> > lost.
> 
> (drive-by): the null-before-delete strategy is to handle cycles - the call to
> delete above could conceivably destroy |this|, making the `data_.ptr =
nullptr`
> step a UAF.
> 
> However, the way to break a cycle is with reset() and reset() already has a
> null-before-delete. I couldn't contrive an example where a loop is created AND
> that loop can be broken via the destructor rather than with reset()  (I think
> the only way to do this would be to semantically have a double-free, and
trying
> to cater for it might turn a semantic double-free into a "safe-but-not-really"
> `delete nullptr`).
> 
> So I don't *think* null-before-delete is needed here, and one could probably
> argue that for performance reasons it should be skipped. But there might be
> something I've missed.

Thanks! Those are good points, regarding destroying the scoped_ptr. I have a
couple other reasons that null before delete seems right to me:

1. The object held by the scoped_ptr could in its destructor dereference the
scoped_ptr holding it? It should be null there.
2. unique_ptr does this in libc++, let's not intentionally differ in behaviour
from it.

[danakj, 2015-09-22 18:02:34]

On 2015/09/22 18:01:31, danakj wrote:
> On 2015/09/22 02:53:10, tapted wrote:
> > https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h
> > File base/memory/scoped_ptr.h (right):
> > 
> >
>
https://codereview.chromium.org/1356483002/diff/1/base/memory/scoped_ptr.h#ne...
> > base/memory/scoped_ptr.h:228: data_.ptr = nullptr;
> > On 2015/09/22 01:13:53, Anand Mistry wrote:
> > > On 2015/09/21 21:43:08, danakj wrote:
> > > > On 2015/09/21 21:40:52, danakj wrote:
> > > > > In libc++ they null the member then call the deleter on a temp var:
> > > > > https://github.com/llvm-mirror/libcxx/blob/master/include/memory
> > > > > 
> > > > > Shall we do the same?
> > > > 
> > > > I guess that is because they reset() in the destructor instead of using
> > > another
> > > > destructor. Perhaps we could call reset() in the scoped_ptr destructor
and
> > > > DCHECK that it's null here instead?
> > > > 
> > > > Up to you.
> > > 
> > > Since we're in the territory of undefined behaviour (read [class.cdtor]),
I
> > > think it's better to be explicit and well commented, so I'd like to keep
the
> > > code as-is and not call reset(). Even though this is logically equivalent
to
> > > calling reset(), I think some of the nuance of making this explicit might
be
> > > lost.
> > 
> > (drive-by): the null-before-delete strategy is to handle cycles - the call
to
> > delete above could conceivably destroy |this|, making the `data_.ptr =
> nullptr`
> > step a UAF.
> > 
> > However, the way to break a cycle is with reset() and reset() already has a
> > null-before-delete. I couldn't contrive an example where a loop is created
AND
> > that loop can be broken via the destructor rather than with reset()  (I
think
> > the only way to do this would be to semantically have a double-free, and
> trying
> > to cater for it might turn a semantic double-free into a
"safe-but-not-really"
> > `delete nullptr`).
> > 
> > So I don't *think* null-before-delete is needed here, and one could probably
> > argue that for performance reasons it should be skipped. But there might be
> > something I've missed.
> 
> Thanks! Those are good points, regarding destroying the scoped_ptr. I have a
> couple other reasons that null before delete seems right to me:
> 
> 1. The object held by the scoped_ptr could in its destructor dereference the
> scoped_ptr holding it? It should be null there.

This could even be unit testable, unless I'm totally wrong because it's in a
destructor, but I don't think so?

> 2. unique_ptr does this in libc++, let's not intentionally differ in behaviour
> from it.

[Anand Mistry (off Chromium), 2015-09-23 01:10:15]

PTAL.

[Peter Kasting, 2015-09-23 01:14:38]

pkasting@chromium.org changed reviewers:
+ pkasting@chromium.org

[Peter Kasting, 2015-09-23 01:14:39]

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr....
base/memory/scoped_ptr.h:227: // references |this| after running the deleter.
It looks like reset() was changed to an intermediate state that should flush out
any bugs where code relied on the old ordering about 2.5 years ago.  So it ought
to be safe to go ahead and change reset()'s ordering to match libc++ now, close
bug 176091, and then have this use reset() like libc++ does.

[Anand Mistry (off Chromium), 2015-09-23 01:44:52]

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr....
base/memory/scoped_ptr.h:227: // references |this| after running the deleter.
On 2015/09/23 01:14:39, Peter Kasting wrote:
> It looks like reset() was changed to an intermediate state that should flush
out
> any bugs where code relied on the old ordering about 2.5 years ago.  So it
ought
> to be safe to go ahead and change reset()'s ordering to match libc++ now,
close
> bug 176091, and then have this use reset() like libc++ does.

Despite being small, I think that's a significant enough change to deserve it's
own CL.

[Peter Kasting, 2015-09-23 01:52:58]

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr....
base/memory/scoped_ptr.h:227: // references |this| after running the deleter.
On 2015/09/23 01:44:52, Anand Mistry wrote:
> On 2015/09/23 01:14:39, Peter Kasting wrote:
> > It looks like reset() was changed to an intermediate state that should flush
> out
> > any bugs where code relied on the old ordering about 2.5 years ago.  So it
> ought
> > to be safe to go ahead and change reset()'s ordering to match libc++ now,
> close
> > bug 176091, and then have this use reset() like libc++ does.
> 
> Despite being small, I think that's a significant enough change to deserve
it's
> own CL.

That's fine; why not do that first and then make a change to this function so
you can move directly to that implementation?

[Anand Mistry (off Chromium), 2015-09-23 04:48:04]

I've rebased onto https://codereview.chromium.org/1358373002/ and changed the
destructor to just call reset(), like libc++.

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr.h
File base/memory/scoped_ptr.h (right):

https://codereview.chromium.org/1356483002/diff/20001/base/memory/scoped_ptr....
base/memory/scoped_ptr.h:227: // references |this| after running the deleter.
On 2015/09/23 01:52:58, Peter Kasting wrote:
> On 2015/09/23 01:44:52, Anand Mistry wrote:
> > On 2015/09/23 01:14:39, Peter Kasting wrote:
> > > It looks like reset() was changed to an intermediate state that should
flush
> > out
> > > any bugs where code relied on the old ordering about 2.5 years ago.  So it
> > ought
> > > to be safe to go ahead and change reset()'s ordering to match libc++ now,
> > close
> > > bug 176091, and then have this use reset() like libc++ does.
> > 
> > Despite being small, I think that's a significant enough change to deserve
> it's
> > own CL.
> 
> That's fine; why not do that first and then make a change to this function so
> you can move directly to that implementation?

Done.

[danakj, 2015-09-23 18:53:39]

LGTM

[Peter Kasting, 2015-09-23 20:35:49]

LGTM

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:721: // into the destructors of |a| and
|a.b|. Note, deleting |a| instead will
Nit: a.b -> a->b

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:723: // destructor.
Nit: Maybe change this last sentence to the following, assuming it's
accurate/you agree:

Note that unless code actually calls a destructor directly like this, the only
way to trigger this call chain normally would be to delete one of the pointers,
which would cause a double-free.  Still, a double-free (and then hopefully
crash) is better than having the program hang.

[danakj, 2015-09-23 20:38:10]

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:723: // destructor.
On 2015/09/23 20:35:49, Peter Kasting wrote:
> Nit: Maybe change this last sentence to the following, assuming it's
> accurate/you agree:
> 
> Note that unless code actually calls a destructor directly like this, the only
> way to trigger this call chain normally would be to delete one of the
pointers,
> which would cause a double-free.  Still, a double-free (and then hopefully
> crash) is better than having the program hang.

I think I prefer the original for a unit test comment.

[danakj, 2015-09-23 22:07:40]

Can you point this at BUG=535321 ?

[Anand Mistry (off Chromium), 2015-09-23 23:06:34]

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
File base/memory/scoped_ptr_unittest.cc (right):

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:721: // into the destructors of |a| and
|a.b|. Note, deleting |a| instead will
On 2015/09/23 20:35:49, Peter Kasting wrote:
> Nit: a.b -> a->b

Done.

https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
base/memory/scoped_ptr_unittest.cc:723: // destructor.
On 2015/09/23 20:38:10, danakj wrote:
> On 2015/09/23 20:35:49, Peter Kasting wrote:
> > Nit: Maybe change this last sentence to the following, assuming it's
> > accurate/you agree:
> > 
> > Note that unless code actually calls a destructor directly like this, the
only
> > way to trigger this call chain normally would be to delete one of the
> pointers,
> > which would cause a double-free.  Still, a double-free (and then hopefully
> > crash) is better than having the program hang.
> 
> I think I prefer the original for a unit test comment.

danakj: Do you mean what I wrote, or what pkasting suggest?

[Peter Kasting, 2015-09-23 23:08:49]

On 2015/09/23 23:06:34, Anand Mistry wrote:
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> File base/memory/scoped_ptr_unittest.cc (right):
> 
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> base/memory/scoped_ptr_unittest.cc:721: // into the destructors of |a| and
> |a.b|. Note, deleting |a| instead will
> On 2015/09/23 20:35:49, Peter Kasting wrote:
> > Nit: a.b -> a->b
> 
> Done.
> 
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> base/memory/scoped_ptr_unittest.cc:723: // destructor.
> On 2015/09/23 20:38:10, danakj wrote:
> > On 2015/09/23 20:35:49, Peter Kasting wrote:
> > > Nit: Maybe change this last sentence to the following, assuming it's
> > > accurate/you agree:
> > > 
> > > Note that unless code actually calls a destructor directly like this, the
> only
> > > way to trigger this call chain normally would be to delete one of the
> > pointers,
> > > which would cause a double-free.  Still, a double-free (and then hopefully
> > > crash) is better than having the program hang.
> > 
> > I think I prefer the original for a unit test comment.
> 
> danakj: Do you mean what I wrote, or what pkasting suggest?

Pretty sure she liked yours.

I think what I don't really like is that the comment in the implementation
refers the reader to the unittest for detail, and then the unittest doesn't
explain when this can happen and what the consequence is.

Maybe the right fix is to instead put more detail in the implementation comment,
then in the unittest refer the reader to that comment (if that's even
necessary).

[danakj, 2015-09-23 23:11:04]

On Wed, Sep 23, 2015 at 4:08 PM, <pkasting@chromium.org> wrote:

> On 2015/09/23 23:06:34, Anand Mistry wrote:
>
>
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
>
>> File base/memory/scoped_ptr_unittest.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
>
>> base/memory/scoped_ptr_unittest.cc:721: // into the destructors of |a| and
>> |a.b|. Note, deleting |a| instead will
>> On 2015/09/23 20:35:49, Peter Kasting wrote:
>> > Nit: a.b -> a->b
>>
>
> Done.
>>
>
>
>
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
>
>> base/memory/scoped_ptr_unittest.cc:723: // destructor.
>> On 2015/09/23 20:38:10, danakj wrote:
>> > On 2015/09/23 20:35:49, Peter Kasting wrote:
>> > > Nit: Maybe change this last sentence to the following, assuming it's
>> > > accurate/you agree:
>> > >
>> > > Note that unless code actually calls a destructor directly like this,
>> the
>> only
>> > > way to trigger this call chain normally would be to delete one of the
>> > pointers,
>> > > which would cause a double-free.  Still, a double-free (and then
>> hopefully
>> > > crash) is better than having the program hang.
>> >
>> > I think I prefer the original for a unit test comment.
>>
>
> danakj: Do you mean what I wrote, or what pkasting suggest?
>>
>
> Pretty sure she liked yours.
>
> I think what I don't really like is that the comment in the implementation
> refers the reader to the unittest for detail, and then the unittest doesn't
> explain when this can happen and what the consequence is.
>
> Maybe the right fix is to instead put more detail in the implementation
> comment,
> then in the unittest refer the reader to that comment (if that's even
> necessary).
>

That sounds fine too. I'm ok as is as well.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anand Mistry (off Chromium), 2015-09-24 01:22:29]

On 2015/09/23 23:11:04, danakj wrote:
> On Wed, Sep 23, 2015 at 4:08 PM, <mailto:pkasting@chromium.org> wrote:
> 
> > On 2015/09/23 23:06:34, Anand Mistry wrote:
> >
> >
> >
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> >
> >> File base/memory/scoped_ptr_unittest.cc (right):
> >>
> >
> >
> >
> >
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> >
> >> base/memory/scoped_ptr_unittest.cc:721: // into the destructors of |a| and
> >> |a.b|. Note, deleting |a| instead will
> >> On 2015/09/23 20:35:49, Peter Kasting wrote:
> >> > Nit: a.b -> a->b
> >>
> >
> > Done.
> >>
> >
> >
> >
> >
>
https://codereview.chromium.org/1356483002/diff/40001/base/memory/scoped_ptr_...
> >
> >> base/memory/scoped_ptr_unittest.cc:723: // destructor.
> >> On 2015/09/23 20:38:10, danakj wrote:
> >> > On 2015/09/23 20:35:49, Peter Kasting wrote:
> >> > > Nit: Maybe change this last sentence to the following, assuming it's
> >> > > accurate/you agree:
> >> > >
> >> > > Note that unless code actually calls a destructor directly like this,
> >> the
> >> only
> >> > > way to trigger this call chain normally would be to delete one of the
> >> > pointers,
> >> > > which would cause a double-free.  Still, a double-free (and then
> >> hopefully
> >> > > crash) is better than having the program hang.
> >> >
> >> > I think I prefer the original for a unit test comment.
> >>
> >
> > danakj: Do you mean what I wrote, or what pkasting suggest?
> >>
> >
> > Pretty sure she liked yours.
> >
> > I think what I don't really like is that the comment in the implementation
> > refers the reader to the unittest for detail, and then the unittest doesn't
> > explain when this can happen and what the consequence is.
> >
> > Maybe the right fix is to instead put more detail in the implementation
> > comment,
> > then in the unittest refer the reader to that comment (if that's even
> > necessary).
> >
> 
> That sounds fine too. I'm ok as is as well.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I'll stick with as-is. The comment pointing at the test was intended as a
pointer to an example, and not detailed documentation per-se.

[Anand Mistry (off Chromium), 2015-09-24 02:40:20]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-09-24 02:40:21]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1356483002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-24 02:41:43]

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

[commit-bot: I haz the power, 2015-09-24 03:44:18]

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

[commit-bot: I haz the power, 2015-09-24 03:44:19]

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

[Anand Mistry (off Chromium), 2015-09-24 08:08:55]

On 2015/09/24 03:44:19, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux
(JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Ugh! Well, time to start hunting!

[Anand Mistry (off Chromium), 2015-11-01 23:03:51]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2015-11-01 23:03:52]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1356483002/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2015-11-01 23:03:59]

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

[commit-bot: I haz the power, 2015-11-02 02:13:15]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-02 02:13:53]

Patchset 7 (id:??) landed as
https://crrev.com/cf9db6ef148d1493dbf6bdc6693f9c79e3ac2afc
Cr-Commit-Position: refs/heads/master@{#357297}

[perkj_chrome, 2015-11-02 11:50:04]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1417063006/ by perkj@chromium.org.

The reason for reverting is: Sorry for reverting, I assume there is nothing
really wrong with this cl but it breaks ToolbarControllerTest on Mac 10.10 and
10.11. 
http://build.chromium.org/p/chromium.mac/builders/Mac10.10%20Tests/builds/3607.

[Anand Mistry (off Chromium), 2015-11-03 22:44:00]

Description was changed from

==========
Call reset(nullptr) in scoped_ptr's destructor.

This behaviour is consistent with libc++.

The original goal of this change is to make Chrome crash more often
and closer to the source when there's a use-after-free bug that
accesses a destroyed scoped_ptr.

BUG=535321

Committed: https://crrev.com/cf9db6ef148d1493dbf6bdc6693f9c79e3ac2afc
Cr-Commit-Position: refs/heads/master@{#357297}
==========

to

==========
Call reset(nullptr) in scoped_ptr's destructor.

This behaviour is consistent with libc++.

The original goal of this change is to make Chrome crash more often
and closer to the source when there's a use-after-free bug that
accesses a destroyed scoped_ptr.

BUG=535321

Committed: https://crrev.com/cf9db6ef148d1493dbf6bdc6693f9c79e3ac2afc
Cr-Commit-Position: refs/heads/master@{#357297}
==========

[Anand Mistry (off Chromium), 2015-11-04 00:10:49]

FYI, as requested, re-opening and re-landing this CL.

[danakj, 2015-11-04 00:22:25]

On 2015/11/04 00:10:49, Anand Mistry wrote:
> FYI, as requested, re-opening and re-landing this CL.

sgtm, good luck to us all :)

[Anand Mistry (off Chromium), 2015-11-04 02:28:56]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-11-04 02:29:31]

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

[commit-bot: I haz the power, 2015-11-04 04:35:53]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-04 04:36:42]

Patchset 7 (id:??) landed as
https://crrev.com/7fa14e3f35b1be8476a2ac61cbad7f3e05a22a18
Cr-Commit-Position: refs/heads/master@{#357756}