Let WeakPtrFactory operations fail once its dtor is called

Let WeakPtrFactory operations fail once its dtor is called

We had a weird bug (crbug.com/127534) where the task was
posted and executed even after WeakPtrFactory has been deleted.
It turns out that someone was posting a task AFTER its detor was
called while executing destructor, and bots (including valgrind)
couldn't catch this because the memory stays valid until the
hosting object is deleted. This CL is to catch such case early by
resetting the ptr_.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137963

[darin (slow to review), 2012-05-17 22:00:52]

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:234: ~WeakPtrFactory(){
nit: add new line above the destructor

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:235: ptr_ = NULL;
i wonder... should we null this out only in debug builds?

[oshima, 2012-05-17 22:16:41]

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:234: ~WeakPtrFactory(){
On 2012/05/17 22:00:52, darin wrote:
> nit: add new line above the destructor

Done.

http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newc...
base/memory/weak_ptr.h:235: ptr_ = NULL;
On 2012/05/17 22:00:52, darin wrote:
> i wonder... should we null this out only in debug builds?
I wanted this to fail on trybots, which is release build but
dcheck enabled. How about this then?

#if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)

[darin (slow to review), 2012-05-17 22:22:18]

On Thu, May 17, 2012 at 3:16 PM, <oshima@chromium.org> wrote:

>
> http://codereview.chromium.**org/10387063/diff/6002/base/**
>
memory/weak_ptr.h<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h>
> File base/memory/weak_ptr.h (right):
>
> http://codereview.chromium.**org/10387063/diff/6002/base/**
>
memory/weak_ptr.h#newcode234<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newcode234>
> base/memory/weak_ptr.h:234: ~WeakPtrFactory(){
> On 2012/05/17 22:00:52, darin wrote:
>
>> nit: add new line above the destructor
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10387063/diff/6002/base/**
>
memory/weak_ptr.h#newcode235<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newcode235>
> base/memory/weak_ptr.h:235: ptr_ = NULL;
> On 2012/05/17 22:00:52, darin wrote:
>
>> i wonder... should we null this out only in debug builds?
>>
> I wanted this to fail on trybots, which is release build but
> dcheck enabled. How about this then?
>
> #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
>

Sorry, I didn't mean for my "i wonder" to suggest that you must
make this change.  It might still be good in official builds to null
the pointer out in case we happen to encounter similar bugs in
the field.  The failures would be more clear if the WeakPtr held
a null pointer instead of a junk pointer.  It might even help avoid
security bugs.  Hmm...



>
>
http://codereview.chromium.**org/10387063/<http://codereview.chromium.org/103...
>

[oshima, 2012-05-18 00:43:34]

On 2012/05/17 22:22:18, darin wrote:
> On Thu, May 17, 2012 at 3:16 PM, <mailto:oshima@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/10387063/diff/6002/base/**
> >
>
memory/weak_ptr.h<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h>
> > File base/memory/weak_ptr.h (right):
> >
> > http://codereview.chromium.**org/10387063/diff/6002/base/**
> >
>
memory/weak_ptr.h#newcode234<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newcode234>
> > base/memory/weak_ptr.h:234: ~WeakPtrFactory(){
> > On 2012/05/17 22:00:52, darin wrote:
> >
> >> nit: add new line above the destructor
> >>
> >
> > Done.
> >
> >
> > http://codereview.chromium.**org/10387063/diff/6002/base/**
> >
>
memory/weak_ptr.h#newcode235<http://codereview.chromium.org/10387063/diff/6002/base/memory/weak_ptr.h#newcode235>
> > base/memory/weak_ptr.h:235: ptr_ = NULL;
> > On 2012/05/17 22:00:52, darin wrote:
> >
> >> i wonder... should we null this out only in debug builds?
> >>
> > I wanted this to fail on trybots, which is release build but
> > dcheck enabled. How about this then?
> >
> > #if !defined(NDEBUG) || defined(DCHECK_ALWAYS_ON)
> >
> 
> Sorry, I didn't mean for my "i wonder" to suggest that you must
> make this change.  It might still be good in official builds to null
> the pointer out in case we happen to encounter similar bugs in
> the field.  The failures would be more clear if the WeakPtr held
> a null pointer instead of a junk pointer.  It might even help avoid
> security bugs.  Hmm...

Ok, in that case, I prefer explicit check failure. I think this is better
because it fails when these methods is called and you get full stack,
instead of having shallow stack from message loop.

- oshima


> 
> 
> 
> >
> >
>
http://codereview.chromium.**org/10387063/%3Chttp://codereview.chromium.org/1...>
> >

[darin (slow to review), 2012-05-18 17:45:13]

I think your first patch is probably best, but I've asked jschuh to comment too
from a security point of view.

http://codereview.chromium.org/10387063/diff/14002/base/memory/weak_ptr.h
File base/memory/weak_ptr.h (right):

http://codereview.chromium.org/10387063/diff/14002/base/memory/weak_ptr.h#new...
base/memory/weak_ptr.h:240: CHECK(ptr_);
I think it is a bit bloaty to inline this into every callsite.  If you want to
null-check ptr_ in release builds, then we should find a way to de-inline.

[jschuh, 2012-05-18 18:11:38]

On 2012/05/18 17:45:13, darin wrote:
> I think your first patch is probably best, but I've asked jschuh to comment
too
> from a security point of view.

Yes, from a security perspective the NULL guarantee is much better than a
potential stale pointer. So, it's worth whatever minimal overhead it introduces
in release.

 
> http://codereview.chromium.org/10387063/diff/14002/base/memory/weak_ptr.h
> File base/memory/weak_ptr.h (right):
> 
>
http://codereview.chromium.org/10387063/diff/14002/base/memory/weak_ptr.h#new...
> base/memory/weak_ptr.h:240: CHECK(ptr_);
> I think it is a bit bloaty to inline this into every callsite.  If you want to
> null-check ptr_ in release builds, then we should find a way to de-inline.

[darin (slow to review), 2012-05-18 18:12:15]

Justin wrote: "Just for reference, from a security perspective a NULL pointer is
always better than a dangling pointer."

Let's go ahead with Path Set 2.

[oshima, 2012-05-18 19:26:53]

Uploaded new patch. PTAL.

On 2012/05/18 18:12:15, darin wrote:
> Justin wrote: "Just for reference, from a security perspective a NULL pointer
is
> always better than a dangling pointer."
> 
> Let's go ahead with Path Set 2.

[darin (slow to review), 2012-05-18 20:20:42]

LGTM

[commit-bot: I haz the power, 2012-05-18 20:21:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oshima@chromium.org/10387063/5

[commit-bot: I haz the power, 2012-05-18 21:33:05]

Change committed as 137963