Make WeakPtr thread-safe, i.e. allow cross-thread copying of WeakPtr

Make WeakPtr thread-safe, i.e. allow cross-thread copying of WeakPtr
and destruction on a thread other than the one where the original 
reference was created.

The problem with the current implementation is modifying the flag pointer
from WeakPtr::~WeakPtr which might happen on a different thread (potentially
racing with somebody invalidating the flag on the original thread).

For compatibility reasons, creating the initial reference attaches to the
thread and governs the thread-safety checking logic with respect to checking
a reference to be valid and invalidating it, which should both only be done
on the same thread.

BUG=82509
TEST=added unit tests

Added memleak suppression: http://crbug.com/94345

TBR=timurrrr@chromium.org

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

[darin (slow to review), 2011-08-18 18:29:06]

Looks good, but I think we need some unit tests.

Will, Daniel and I spent some time discussing this solution today.  I think he's
found a way to make it safe to pass WeakPtr<T> to another thread.  This is
exciting since it means that WeakHandle<T> is not necessary.

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode39
base/memory/weak_ptr.cc:39: WeakReferenceOwner::WeakReferenceOwner() :
flag_(NULL) {
nit: delete the explicit initializer

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
perhaps it would be better to reuse the HasRefs() implementation?

  if (!HasRefs())
    flag_ = new WeakReference::Flag();

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

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.h#newcode85
base/memory/weak_ptr.h:85: bool valid_;
nit: valid_ -> is_valid_

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.h#newcode89
base/memory/weak_ptr.h:89: WeakReference(scoped_refptr<Flag> flag);
nit: WeakReference(const scoped_refptr<Flag>& flag);

[willchan no longer on Chromium, 2011-08-18 18:49:52]

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
On 2011/08/18 18:29:06, darin wrote:
> perhaps it would be better to reuse the HasRefs() implementation?
> 
>   if (!HasRefs())
>     flag_ = new WeakReference::Flag();

Sorry, it's not clear to me how this is safe.

If base::WeakPtr is destroyed on another thread, then its WeakReference will go
away on that other thread, right? That means the refcount for
WeakReference::Flag may be decremented on another thread (or incremented if the
WeakPtr is copied on another thread). So I don't understand why an
implementation that uses HasOneRef() where refcounts are modified in multiple
threads can be safe. Is this race benign somehow? Sorry, didn't sleep on the
plane and stuck at a transfer in Dubai. Comments here explaining the
implementation detail would be nice.

[willchan no longer on Chromium, 2011-08-18 19:14:12]

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

http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.h#newcode89
base/memory/weak_ptr.h:89: WeakReference(scoped_refptr<Flag> flag);
On 2011/08/18 18:29:06, darin wrote:
> nit: WeakReference(const scoped_refptr<Flag>& flag);

While I'm glad you seem to have this preference here (I share this preference),
I think chromium-dev disagreed. We should be using Flag* here instead. See
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre....

[darin (slow to review), 2011-08-18 19:17:40]

On 2011/08/18 18:49:52, willchan wrote:
> http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
> File base/memory/weak_ptr.cc (right):
> 
>
http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
> base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
> On 2011/08/18 18:29:06, darin wrote:
> > perhaps it would be better to reuse the HasRefs() implementation?
> > 
> >   if (!HasRefs())
> >     flag_ = new WeakReference::Flag();
> 
> Sorry, it's not clear to me how this is safe.
> 
> If base::WeakPtr is destroyed on another thread, then its WeakReference will
go
> away on that other thread, right? That means the refcount for
> WeakReference::Flag may be decremented on another thread (or incremented if
the
> WeakPtr is copied on another thread). So I don't understand why an
> implementation that uses HasOneRef() where refcounts are modified in multiple
> threads can be safe. Is this race benign somehow?

Yes, you don't need to worry.  The trick here is that WeakReferenceOwner now
also owns a reference to the Flag.  If the Flag has only one reference, then
the owner can know that he is the one holding the reference.  This means that
no background thread can be simultaneously mutating the reference count when
the reference count is 1.  The HasOneRef trick is also used in WebKit in some
instances.

-Darin

[darin (slow to review), 2011-08-18 19:18:39]

On 2011/08/18 19:14:12, willchan wrote:
> http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.h
> File base/memory/weak_ptr.h (right):
> 
> http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.h#newcode89
> base/memory/weak_ptr.h:89: WeakReference(scoped_refptr<Flag> flag);
> On 2011/08/18 18:29:06, darin wrote:
> > nit: WeakReference(const scoped_refptr<Flag>& flag);
> 
> While I'm glad you seem to have this preference here (I share this
preference),
> I think chromium-dev disagreed. We should be using Flag* here instead. See
>
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre....

Oh, I see.  I don't have a preference between 'const Foo<T>&' and 'T*'.  They
both avoid generating an intermediate AddRef and Release.  That's all I cared
about.  We should follow the style guide.

[willchan no longer on Chromium, 2011-08-18 19:33:07]

On 2011/08/18 19:17:40, darin wrote:
> On 2011/08/18 18:49:52, willchan wrote:
> > http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
> > File base/memory/weak_ptr.cc (right):
> > 
> >
>
http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
> > base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
> > On 2011/08/18 18:29:06, darin wrote:
> > > perhaps it would be better to reuse the HasRefs() implementation?
> > > 
> > >   if (!HasRefs())
> > >     flag_ = new WeakReference::Flag();
> > 
> > Sorry, it's not clear to me how this is safe.
> > 
> > If base::WeakPtr is destroyed on another thread, then its WeakReference will
> go
> > away on that other thread, right? That means the refcount for
> > WeakReference::Flag may be decremented on another thread (or incremented if
> the
> > WeakPtr is copied on another thread). So I don't understand why an
> > implementation that uses HasOneRef() where refcounts are modified in
multiple
> > threads can be safe. Is this race benign somehow?
> 
> Yes, you don't need to worry.  The trick here is that WeakReferenceOwner now
> also owns a reference to the Flag.  If the Flag has only one reference, then
> the owner can know that he is the one holding the reference.  This means that
> no background thread can be simultaneously mutating the reference count when
> the reference count is 1.  The HasOneRef trick is also used in WebKit in some
> instances.

Ah, clever! OK, I'm onboard with this :) Please add comments explaining the
trick.

The one issue that becomes a bit problematic is what to do about
WeakPtrFactory::empty(), since it's fundamentally racy, as I noted on
http://code.google.com/p/chromium/issues/detail?id=82509. I advocated removing
support for WeakPtrFactory::empty(), which is doable but not trivial since many
places use it (primarily via ScopedCallbackFactory or
ScopedRunnableMethodFactory) as a way to check if a scoped callback or a scoped
RunnableMethod are pending. If these scoped callbacks and scoped RunnableMethods
can be deleted on other threads, then using WeakPtrFactory::empty() to check for
existence of pending callbacks/RunnableMethods is racy.

> 
> -Darin

[akalin, 2011-08-18 19:40:35]

On Thu, Aug 18, 2011 at 11:29 AM,  <darin@chromium.org> wrote:
> Looks good, but I think we need some unit tests.
>
> Will, Daniel and I spent some time discussing this solution today.  I think
> he's
> found a way to make it safe to pass WeakPtr<T> to another thread.  This is
> exciting since it means that WeakHandle<T> is not necessary.

I don't think so.  WeakHandle<T> is needed whether or not WeakPtr<T>
is safe to copy/use from other threads.  Even if WeakPtr<T> was
completely safe, the snippet:

if (weak_ptr.get()) {
  weak_ptr->DoFoo();
}

is still racy if ran on another thread.  You still want:

weak_handle.Call(&DoFoo);  // calls on owner thread.

It'd enable me to clean up the contortions I go through in WeakHandle
to make sure WeakPtr is cleaned up on the right thread, though!

[willchan no longer on Chromium, 2011-08-18 19:45:23]

On Thu, Aug 18, 2011 at 12:40 PM, Fred Akalin <akalin@chromium.org> wrote:
> On Thu, Aug 18, 2011 at 11:29 AM,  <darin@chromium.org> wrote:
>> Looks good, but I think we need some unit tests.
>>
>> Will, Daniel and I spent some time discussing this solution today.  I think
>> he's
>> found a way to make it safe to pass WeakPtr<T> to another thread.  This is
>> exciting since it means that WeakHandle<T> is not necessary.
>
> I don't think so.  WeakHandle<T> is needed whether or not WeakPtr<T>
> is safe to copy/use from other threads.  Even if WeakPtr<T> was
> completely safe, the snippet:
>
> if (weak_ptr.get()) {
>  weak_ptr->DoFoo();
> }

I don't believe this changelist is intended to address this use case.
Just the destruction.

>
> is still racy if ran on another thread.  You still want:
>
> weak_handle.Call(&DoFoo);  // calls on owner thread.

Are there other gains to WeakHandle? This is synactic sugar, since if
WeakPtr is safe to destroy on another thread, then weak_handle.Call()
is basically just a PostTask, right? base::Bind() will already do the
WeakPtr check for us. I think the argument would then be, do we need a
new class for this single use case? Am I missing other things that
WeakHandle solves? I think it's nice not to have both a WeakPtr and
WeakHandle since it becomes confusing which one should be used.

>
> It'd enable me to clean up the contortions I go through in WeakHandle
> to make sure WeakPtr is cleaned up on the right thread, though!
>

[akalin, 2011-08-18 19:48:50]

On Thu, Aug 18, 2011 at 12:45 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> Are there other gains to WeakHandle? This is synactic sugar, since if
> WeakPtr is safe to destroy on another thread, then weak_handle.Call()
> is basically just a PostTask, right? base::Bind() will already do the
> WeakPtr check for us. I think the argument would then be, do we need a
> new class for this single use case? Am I missing other things that
> WeakHandle solves? I think it's nice not to have both a WeakPtr and
> WeakHandle since it becomes confusing which one should be used.

It's more than just sugar; if there is no way to use WeakHandle other
than posting to the owner thread, then that makes it safer than just a
{ WeakPtr, MessageLoop } struct, since with the latter, you can still
(mis)use the WeakPtr to do whatever you want, on whatever thread you
want.

[darin (slow to review), 2011-08-18 19:54:00]

On Thu, Aug 18, 2011 at 12:47 PM, Fred Akalin <akalin@chromium.org> wrote:

> On Thu, Aug 18, 2011 at 12:45 PM, William Chan (陈智昌)
> <willchan@chromium.org> wrote:
> > Are there other gains to WeakHandle? This is synactic sugar, since if
> > WeakPtr is safe to destroy on another thread, then weak_handle.Call()
> > is basically just a PostTask, right? base::Bind() will already do the
> > WeakPtr check for us. I think the argument would then be, do we need a
> > new class for this single use case? Am I missing other things that
> > WeakHandle solves? I think it's nice not to have both a WeakPtr and
> > WeakHandle since it becomes confusing which one should be used.
>
> It's more than just sugar; if there is no way to use WeakHandle other
> than posting to the owner thread, then that makes it safer than just a
> { WeakPtr, MessageLoop } struct, since with the latter, you can still
> (mis)use the WeakPtr to do whatever you want, on whatever thread you
> want.
>

The WeakPtr will assert that you only use it on the proper thread.  This
makes WeakPtr equivalent to the WeakHandle that I was proposing.

If you recall from the review, I was arguing against WeakHandle having
a Call method.

-Darin

[darin (slow to review), 2011-08-18 19:59:06]

On Thu, Aug 18, 2011 at 12:33 PM, <willchan@chromium.org> wrote:

> On 2011/08/18 19:17:40, darin wrote:
>
>> On 2011/08/18 18:49:52, willchan wrote:
>> > http://codereview.chromium.**org/7677028/diff/1/base/**
>>
memory/weak_ptr.cc<http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc>
>> > File base/memory/weak_ptr.cc (right):
>> >
>> >
>>
>
> http://codereview.chromium.**org/7677028/diff/1/base/**
>
memory/weak_ptr.cc#newcode49<http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49>
>
>> > base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
>> > On 2011/08/18 18:29:06, darin wrote:
>> > > perhaps it would be better to reuse the HasRefs() implementation?
>> > >
>> > >   if (!HasRefs())
>> > >     flag_ = new WeakReference::Flag();
>> >
>> > Sorry, it's not clear to me how this is safe.
>> >
>> > If base::WeakPtr is destroyed on another thread, then its WeakReference
>> will
>> go
>> > away on that other thread, right? That means the refcount for
>> > WeakReference::Flag may be decremented on another thread (or incremented
>> if
>> the
>> > WeakPtr is copied on another thread). So I don't understand why an
>> > implementation that uses HasOneRef() where refcounts are modified in
>>
> multiple
>
>> > threads can be safe. Is this race benign somehow?
>>
>
>  Yes, you don't need to worry.  The trick here is that WeakReferenceOwner
>> now
>> also owns a reference to the Flag.  If the Flag has only one reference,
>> then
>> the owner can know that he is the one holding the reference.  This means
>> that
>> no background thread can be simultaneously mutating the reference count
>> when
>> the reference count is 1.  The HasOneRef trick is also used in WebKit in
>> some
>> instances.
>>
>
> Ah, clever! OK, I'm onboard with this :) Please add comments explaining the
> trick.
>
> The one issue that becomes a bit problematic is what to do about
> WeakPtrFactory::empty(), since it's fundamentally racy, as I noted on
>
http://code.google.com/p/**chromium/issues/detail?id=**82509<http://code.goog....
> I advocated removing
> support for WeakPtrFactory::empty(), which is doable but not trivial since
> many
> places use it (primarily via ScopedCallbackFactory or
> ScopedRunnableMethodFactory) as a way to check if a scoped callback or a
> scoped
> RunnableMethod are pending. If these scoped callbacks and scoped
> RunnableMethods
> can be deleted on other threads, then using WeakPtrFactory::empty() to
> check for
> existence of pending callbacks/RunnableMethods is racy.
>

Yes, that is a potential issue.  However, Scoped*Factory cannot be used to
post tasks across threads since that would involve dereferencing the WeakPtr
on another thread.

But it really is true that this could come back to bight us at some point.
 If
we could kill the HasWeakPtrs() function that would be nice.

-Darin


>
>
>
http://codereview.chromium.**org/7677028/<http://codereview.chromium.org/7677...
>

[akalin, 2011-08-18 19:59:16]

On Thu, Aug 18, 2011 at 12:53 PM, Darin Fisher <darin@chromium.org> wrote:
> The WeakPtr will assert that you only use it on the proper thread.  This
> makes WeakPtr equivalent to the WeakHandle that I was proposing.
> If you recall from the review, I was arguing against WeakHandle having
> a Call method.

Fair enough.  Although if WeakPtr now has knowledge of its owner
thread, some syntactic sugar would be nice.  I'd rather type:

weak_ptr.Call(FROM_HERE, &MyClass::DoFoo, bar);

than:

weak_ptr.message_loop_proxy()->PostTask(FROM_HERE,
base::Bind(&MyClass::DoFoo, weak_ptr, bar));

[willchan no longer on Chromium, 2011-08-18 20:04:30]

On Thu, Aug 18, 2011 at 12:59 PM, Darin Fisher <darin@chromium.org> wrote:
>
>
> On Thu, Aug 18, 2011 at 12:33 PM, <willchan@chromium.org> wrote:
>>
>> On 2011/08/18 19:17:40, darin wrote:
>>>
>>> On 2011/08/18 18:49:52, willchan wrote:
>>> > http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
>>> > File base/memory/weak_ptr.cc (right):
>>> >
>>> >
>>
>>
>>
http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
>>>
>>> > base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
>>> > On 2011/08/18 18:29:06, darin wrote:
>>> > > perhaps it would be better to reuse the HasRefs() implementation?
>>> > >
>>> > >   if (!HasRefs())
>>> > >     flag_ = new WeakReference::Flag();
>>> >
>>> > Sorry, it's not clear to me how this is safe.
>>> >
>>> > If base::WeakPtr is destroyed on another thread, then its WeakReference
>>> > will
>>> go
>>> > away on that other thread, right? That means the refcount for
>>> > WeakReference::Flag may be decremented on another thread (or
>>> > incremented if
>>> the
>>> > WeakPtr is copied on another thread). So I don't understand why an
>>> > implementation that uses HasOneRef() where refcounts are modified in
>>
>> multiple
>>>
>>> > threads can be safe. Is this race benign somehow?
>>
>>> Yes, you don't need to worry.  The trick here is that WeakReferenceOwner
>>> now
>>> also owns a reference to the Flag.  If the Flag has only one reference,
>>> then
>>> the owner can know that he is the one holding the reference.  This means
>>> that
>>> no background thread can be simultaneously mutating the reference count
>>> when
>>> the reference count is 1.  The HasOneRef trick is also used in WebKit in
>>> some
>>> instances.
>>
>> Ah, clever! OK, I'm onboard with this :) Please add comments explaining
>> the
>> trick.
>>
>> The one issue that becomes a bit problematic is what to do about
>> WeakPtrFactory::empty(), since it's fundamentally racy, as I noted on
>> http://code.google.com/p/chromium/issues/detail?id=82509. I advocated
>> removing
>> support for WeakPtrFactory::empty(), which is doable but not trivial since
>> many
>> places use it (primarily via ScopedCallbackFactory or
>> ScopedRunnableMethodFactory) as a way to check if a scoped callback or a
>> scoped
>> RunnableMethod are pending. If these scoped callbacks and scoped
>> RunnableMethods
>> can be deleted on other threads, then using WeakPtrFactory::empty() to
>> check for
>> existence of pending callbacks/RunnableMethods is racy.
>
> Yes, that is a potential issue.  However, Scoped*Factory cannot be used to
> post tasks across threads since that would involve dereferencing the WeakPtr
> on another thread.

That's true, but the case that I can envision people doing is
basically the PostTaskAndReply() case. There's also the more
complicated case of: I create X on thread 1. I pass WeakPtr<X> to
thread 2. Whenever some event happens on thread 2, I post WeakPtr<X>
back to thread 1 to do something. WeakPtr<X> may be deleted on thread
2 at some point.

> But it really is true that this could come back to bight us at some point.
>  If
> we could kill the HasWeakPtrs() function that would be nice.

+1

> -Darin
>
>>
>> http://codereview.chromium.org/7677028/
>
>

[darin (slow to review), 2011-08-18 20:08:11]

On Thu, Aug 18, 2011 at 1:04 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> On Thu, Aug 18, 2011 at 12:59 PM, Darin Fisher <darin@chromium.org> wrote:
> >
> >
> > On Thu, Aug 18, 2011 at 12:33 PM, <willchan@chromium.org> wrote:
> >>
> >> On 2011/08/18 19:17:40, darin wrote:
> >>>
> >>> On 2011/08/18 18:49:52, willchan wrote:
> >>> >
> http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc
> >>> > File base/memory/weak_ptr.cc (right):
> >>> >
> >>> >
> >>
> >>
> >>
>
http://codereview.chromium.org/7677028/diff/1/base/memory/weak_ptr.cc#newcode49
> >>>
> >>> > base/memory/weak_ptr.cc:49: if (!flag_ || flag_->HasOneRef())
> >>> > On 2011/08/18 18:29:06, darin wrote:
> >>> > > perhaps it would be better to reuse the HasRefs() implementation?
> >>> > >
> >>> > >   if (!HasRefs())
> >>> > >     flag_ = new WeakReference::Flag();
> >>> >
> >>> > Sorry, it's not clear to me how this is safe.
> >>> >
> >>> > If base::WeakPtr is destroyed on another thread, then its
> WeakReference
> >>> > will
> >>> go
> >>> > away on that other thread, right? That means the refcount for
> >>> > WeakReference::Flag may be decremented on another thread (or
> >>> > incremented if
> >>> the
> >>> > WeakPtr is copied on another thread). So I don't understand why an
> >>> > implementation that uses HasOneRef() where refcounts are modified in
> >>
> >> multiple
> >>>
> >>> > threads can be safe. Is this race benign somehow?
> >>
> >>> Yes, you don't need to worry.  The trick here is that
> WeakReferenceOwner
> >>> now
> >>> also owns a reference to the Flag.  If the Flag has only one reference,
> >>> then
> >>> the owner can know that he is the one holding the reference.  This
> means
> >>> that
> >>> no background thread can be simultaneously mutating the reference count
> >>> when
> >>> the reference count is 1.  The HasOneRef trick is also used in WebKit
> in
> >>> some
> >>> instances.
> >>
> >> Ah, clever! OK, I'm onboard with this :) Please add comments explaining
> >> the
> >> trick.
> >>
> >> The one issue that becomes a bit problematic is what to do about
> >> WeakPtrFactory::empty(), since it's fundamentally racy, as I noted on
> >> http://code.google.com/p/chromium/issues/detail?id=82509. I advocated
> >> removing
> >> support for WeakPtrFactory::empty(), which is doable but not trivial
> since
> >> many
> >> places use it (primarily via ScopedCallbackFactory or
> >> ScopedRunnableMethodFactory) as a way to check if a scoped callback or a
> >> scoped
> >> RunnableMethod are pending. If these scoped callbacks and scoped
> >> RunnableMethods
> >> can be deleted on other threads, then using WeakPtrFactory::empty() to
> >> check for
> >> existence of pending callbacks/RunnableMethods is racy.
> >
> > Yes, that is a potential issue.  However, Scoped*Factory cannot be used
> to
> > post tasks across threads since that would involve dereferencing the
> WeakPtr
> > on another thread.
>
> That's true, but the case that I can envision people doing is
> basically the PostTaskAndReply() case. There's also the more
> complicated case of: I create X on thread 1. I pass WeakPtr<X> to
> thread 2. Whenever some event happens on thread 2, I post WeakPtr<X>
> back to thread 1 to do something. WeakPtr<X> may be deleted on thread
> 2 at some point.
>
>
Fortunately, PostTaskAndReply avoids this problem.  Hopefully, people will
just use that.  However, Daniel's use case, which prompted him to write this
patch, needed something like what you are describing.



> > But it really is true that this could come back to bight us at some
> point.
> >  If
> > we could kill the HasWeakPtrs() function that would be nice.
>
> +1
>
> > -Darin
> >
> >>
> >> http://codereview.chromium.org/7677028/
> >
> >
>

[darin (slow to review), 2011-08-18 20:10:19]

On Thu, Aug 18, 2011 at 12:59 PM, Fred Akalin <akalin@chromium.org> wrote:

> On Thu, Aug 18, 2011 at 12:53 PM, Darin Fisher <darin@chromium.org> wrote:
> > The WeakPtr will assert that you only use it on the proper thread.  This
> > makes WeakPtr equivalent to the WeakHandle that I was proposing.
> > If you recall from the review, I was arguing against WeakHandle having
> > a Call method.
>
> Fair enough.  Although if WeakPtr now has knowledge of its owner
> thread, some syntactic sugar would be nice.  I'd rather type:
>
> weak_ptr.Call(FROM_HERE, &MyClass::DoFoo, bar);
>

What if I want the call to happen after a delay?  or, what if I want it to
happen
only in a non-nested event loop?  What if I want to generate a reply after
the
task is run.  MessageLoopProxy / BrowserThread / MessageLoop have a very
rich API that would be unfortunate to replicate.

I'd really like to move us to a world where all of this API surface is
captured
by a single TaskRunner interface.



>
> than:
>
> weak_ptr.message_loop_proxy()->PostTask(FROM_HERE,
> base::Bind(&MyClass::DoFoo, weak_ptr, bar));
>


WeakPtr only contains thread information in debug builds to implement the
assertions.  I think this means that we will have to externally keep track
of
what thread the WeakPtr belongs to.

Maybe there is room for generalizing the pair (WeakPtr, MessageLoopProxy).

-Darin

[no sievers, 2011-08-18 20:18:31]

I think I addressed all the comments. I added one unit tests, but am still
accepting ideas for other ones :)

I put a comment in the header file with notes about thread safety and what to do
and not to do.

[no sievers, 2011-08-18 20:25:37]

On 2011/08/18 19:59:16, akalin wrote:
> On Thu, Aug 18, 2011 at 12:53 PM, Darin Fisher <mailto:darin@chromium.org>
wrote:
> > The WeakPtr will assert that you only use it on the proper thread.
&nbsp;This
> > makes WeakPtr equivalent to the WeakHandle that I was proposing.
> > If you recall from the review, I was arguing against WeakHandle having
> > a Call method.
> 
> Fair enough.  Although if WeakPtr now has knowledge of its owner
> thread, some syntactic sugar would be nice.  I'd rather type:
> 
> weak_ptr.Call(FROM_HERE, &MyClass::DoFoo, bar);
> 
> than:
> 
> weak_ptr.message_loop_proxy()->PostTask(FROM_HERE,
> base::Bind(&MyClass::DoFoo, weak_ptr, bar));

In fact, something like you describe is what I wanted to do and is how I ended
up here:

http://codereview.chromium.org/7634019/ (see latest patch set)

[darin (slow to review), 2011-08-18 20:29:52]

LGTM

http://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

http://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.cc#newc...
base/memory/weak_ptr.cc:49: if (!flag_ || !HasRefs())
HasRefs() already null checks flag_, right?

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

http://codereview.chromium.org/7677028/diff/5002/base/memory/weak_ptr.h#newco...
base/memory/weak_ptr.h:48: // Thread-safety notes: Weak pointers may only be
dereferenced on the thread
This passage is a bit complicated.

I think you might want to rework it like so:

  When you get a WeakPtr (from a WeakPtrFactory or SupportsWeakPtr), 
  the WeakPtr becomes bound to the current thread.  You may only 
  dereference the WeakPtr on that thread.  However, it is safe to 
  destroy the WeakPtr object on another thread.

  Since a WeakPtr object may be destroyed on a background thread, 
  querying WeakPtrFactory's HasWeakPtrs() method can be racy.

That last bit may be better placed above the HasWeakPtrs() definition.  I also
don't think you need to document HasRefs() since that is not part of the public
API.

[no sievers, 2011-08-18 22:33:25]

Will, Fred?

[willchan no longer on Chromium, 2011-08-18 22:39:10]

I'm about to catch my transfer flight, so I don't have time to look.
Darin's LGTM is good enough for me!

On Thu, Aug 18, 2011 at 3:33 PM,  <sievers@chromium.org> wrote:
> Will, Fred?
>
> http://codereview.chromium.org/7677028/
>

[ajwong, 2011-08-18 22:40:43]

will...if you're on vacation, you're the worst vacation taker evar.

On Thu, Aug 18, 2011 at 3:39 PM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> I'm about to catch my transfer flight, so I don't have time to look.
> Darin's LGTM is good enough for me!
>
> On Thu, Aug 18, 2011 at 3:33 PM,  <sievers@chromium.org> wrote:
>> Will, Fred?
>>
>> http://codereview.chromium.org/7677028/
>>
>

[akalin, 2011-08-18 22:51:54]

LGTM

So just to confirm, we've all agreed that WeakPtrs will be valid to pass (but
not use) across threads, etc, forevermore?

[no sievers, 2011-08-18 23:00:43]

I think that makes sense, since you cannot do anything with an object anyway if
it might get deleted on a different thread at any given time.
Any operation on the object itself would have to be done on the owning thread,
so the only thing making sense is some thread-safe task posting semantics like
you mentioned. But that is probably orthogonal to this specific patch/problem.


On 2011/08/18 22:51:54, akalin wrote:
> LGTM
> 
> So just to confirm, we've all agreed that WeakPtrs will be valid to pass (but
> not use) across threads, etc, forevermore?

[awong, 2011-08-18 23:58:19]

Sorry for coming late to the game.  Looks like a good improvement. Yay!

I have a few nits and some suggestions for unittests.

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

http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
base/memory/weak_ptr.h:79: explicit Flag();
nit: no need for explicit on 0-arity constructors.

http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
base/memory/weak_ptr.h:96: WeakReference(const Flag* flag);
this should have explicit on it though I think.

http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
File base/memory/weak_ptr_unittest.cc (right):

http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
base/memory/weak_ptr_unittest.cc:159: TEST(WeakPtrTest, ReattachThread) {
I feel like we should have more unittest to ensure WeakPtr is safe to delete on
other threads.  Off the top of my head, I think we should test:

(1) Originating thread has a WeakPtr that outlives others.
  - Thread A creates WeakPtr<>
  - Make a copy and pass to Thread B.
  - B destructs its copy.
   - A Destructs its copy
no crash.

(2) Originating thread drops all references before another thread.
  - Thread A creates WeakPtr<>
  - Make a copy and pass to Thread B.
   - A Destructs its copy
  - B destructs its copy.
no crash.

(3) Originating thread invalidates WeakPtrs while its held by other thread.
  - Thread A creates WeakPtr<> and passes Copy to Thread B.
  - Thread A destructs the WeakReferenceOwner.
  - B destructs WeakPtr.
no crash.

(4) WeakPtr can be passed alway the way through and back:
  - Thread A creates WeakPtr<> and passes Copy to Thread B.
  - Thread A destructs the WeakReferenceOwner.
  - B passes WeakPtr<> back to A
  - A inspects WeakPtr<> and correctly sees that it is invalidated.
no crash.

[darin (slow to review), 2011-08-19 03:22:29]

From my phone...

I thought Will had already added some of these tests.
On Aug 18, 2011 4:58 PM, <ajwong@chromium.org> wrote:
> Sorry for coming late to the game. Looks like a good improvement. Yay!
>
> I have a few nits and some suggestions for unittests.
>
>
> http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h
> File base/memory/weak_ptr.h (right):
>
>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
> base/memory/weak_ptr.h:79: explicit Flag();
> nit: no need for explicit on 0-arity constructors.
>
>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
> base/memory/weak_ptr.h:96: WeakReference(const Flag* flag);
> this should have explicit on it though I think.
>
>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
> File base/memory/weak_ptr_unittest.cc (right):
>
>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
> base/memory/weak_ptr_unittest.cc:159: TEST(WeakPtrTest, ReattachThread)
> {
> I feel like we should have more unittest to ensure WeakPtr is safe to
> delete on other threads. Off the top of my head, I think we should
> test:
>
> (1) Originating thread has a WeakPtr that outlives others.
> - Thread A creates WeakPtr<>
> - Make a copy and pass to Thread B.
> - B destructs its copy.
> - A Destructs its copy
> no crash.
>
> (2) Originating thread drops all references before another thread.
> - Thread A creates WeakPtr<>
> - Make a copy and pass to Thread B.
> - A Destructs its copy
> - B destructs its copy.
> no crash.
>
> (3) Originating thread invalidates WeakPtrs while its held by other
> thread.
> - Thread A creates WeakPtr<> and passes Copy to Thread B.
> - Thread A destructs the WeakReferenceOwner.
> - B destructs WeakPtr.
> no crash.
>
> (4) WeakPtr can be passed alway the way through and back:
> - Thread A creates WeakPtr<> and passes Copy to Thread B.
> - Thread A destructs the WeakReferenceOwner.
> - B passes WeakPtr<> back to A
> - A inspects WeakPtr<> and correctly sees that it is invalidated.
> no crash.
>
> http://codereview.chromium.org/7677028/

[ajwong, 2011-08-19 05:09:28]

Oh you're right!  SingleThreaded1 and SingleThreaded2 do indeed cover
a couple of these cases.  I was thrown off by the naming and had
skipped reading them.  Feel free to ignore my unittest suggestions if
they seem overkill or are already covered.

-A


On Thu, Aug 18, 2011 at 8:22 PM, Darin Fisher <darin@chromium.org> wrote:
> From my phone...
>
> I thought Will had already added some of these tests.
>
> On Aug 18, 2011 4:58 PM, <ajwong@chromium.org> wrote:
>> Sorry for coming late to the game. Looks like a good improvement. Yay!
>>
>> I have a few nits and some suggestions for unittests.
>>
>>
>> http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h
>> File base/memory/weak_ptr.h (right):
>>
>>
>>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
>> base/memory/weak_ptr.h:79: explicit Flag();
>> nit: no need for explicit on 0-arity constructors.
>>
>>
>>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr.h#newco...
>> base/memory/weak_ptr.h:96: WeakReference(const Flag* flag);
>> this should have explicit on it though I think.
>>
>>
>>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
>> File base/memory/weak_ptr_unittest.cc (right):
>>
>>
>>
http://codereview.chromium.org/7677028/diff/4005/base/memory/weak_ptr_unittes...
>> base/memory/weak_ptr_unittest.cc:159: TEST(WeakPtrTest, ReattachThread)
>> {
>> I feel like we should have more unittest to ensure WeakPtr is safe to
>> delete on other threads. Off the top of my head, I think we should
>> test:
>>
>> (1) Originating thread has a WeakPtr that outlives others.
>> - Thread A creates WeakPtr<>
>> - Make a copy and pass to Thread B.
>> - B destructs its copy.
>> - A Destructs its copy
>> no crash.
>>
>> (2) Originating thread drops all references before another thread.
>> - Thread A creates WeakPtr<>
>> - Make a copy and pass to Thread B.
>> - A Destructs its copy
>> - B destructs its copy.
>> no crash.
>>
>> (3) Originating thread invalidates WeakPtrs while its held by other
>> thread.
>> - Thread A creates WeakPtr<> and passes Copy to Thread B.
>> - Thread A destructs the WeakReferenceOwner.
>> - B destructs WeakPtr.
>> no crash.
>>
>> (4) WeakPtr can be passed alway the way through and back:
>> - Thread A creates WeakPtr<> and passes Copy to Thread B.
>> - Thread A destructs the WeakReferenceOwner.
>> - B passes WeakPtr<> back to A
>> - A inspects WeakPtr<> and correctly sees that it is invalidated.
>> no crash.
>>
>> http://codereview.chromium.org/7677028/
>

[no sievers, 2011-08-19 18:21:48]

Don't all your suggestions involve deleting either weak reference owners or
pointers on the 'off thread', which the existing cases don't cover?


On 2011/08/19 05:09:28, ajwong wrote:
> Oh you're right!  SingleThreaded1 and SingleThreaded2 do indeed cover
> a couple of these cases.  I was thrown off by the naming and had
> skipped reading them.  Feel free to ignore my unittest suggestions if
> they seem overkill or are already covered.
> 
> -A
>

[no sievers, 2011-08-19 19:45:08]

Also, one more thing, consider this case:

1) Thread A creates one or more weak reference to an object
2) Thread A deletes all weak references to the object
3) Thread B deletes the object

Before this change, with 2) happening we were already deleting the flag, which
means that WeakReference::Flag::Invalidate() would not be called from the owner
upon destruction. In other words, we would only hit the helpful
DCHECK(CalledOnValidThread()) in the (potentially rare case) of 2) actually
losing the race against 3).

Now, after my change I am already hitting this DCHECK() with GpuMessageFilter
trying to be deleted on the UI thread, while we use weak references on the IO
thread (to send back replies from GPU process to the renderer). Still need to
see if this is a bug.

I suggest for backwards compatibility that I relax the check in Invalidate() to
'DCHECK(thread_checker_.CalledOnValidThread())|| HasOneRef())' with a follow-up
change removing the second part again.

I suspect that hitting this might help uncover bugs. If this is intentional,
i.e. the code explicitly synchronizes deletion of all weak pointers with the
deletion of the object on another thread, we should require a call to
WeakReferenceOwner::DetachFromThread().

Does that make sense?

[darin (slow to review), 2011-08-19 21:27:31]

On Fri, Aug 19, 2011 at 12:45 PM, <sievers@chromium.org> wrote:

> Also, one more thing, consider this case:
>
> 1) Thread A creates one or more weak reference to an object
> 2) Thread A deletes all weak references to the object
> 3) Thread B deletes the object
>
> Before this change, with 2) happening we were already deleting the flag,
> which
> means that WeakReference::Flag::**Invalidate() would not be called from
> the owner
> upon destruction. In other words, we would only hit the helpful
> DCHECK(CalledOnValidThread()) in the (potentially rare case) of 2) actually
> losing the race against 3).
>
> Now, after my change I am already hitting this DCHECK() with
> GpuMessageFilter
> trying to be deleted on the UI thread, while we use weak references on the
> IO
> thread (to send back replies from GPU process to the renderer). Still need
> to
> see if this is a bug.
>
> I suggest for backwards compatibility that I relax the check in
> Invalidate() to
> 'DCHECK(thread_checker_.**CalledOnValidThread())|| HasOneRef())' with a
> follow-up
> change removing the second part again.
>

^^^ Makes sense to be compatible like that.



>
> I suspect that hitting this might help uncover bugs. If this is
> intentional,
> i.e. the code explicitly synchronizes deletion of all weak pointers with
> the
> deletion of the object on another thread, we should require a call to
> WeakReferenceOwner::**DetachFromThread().
>

It is hard to say.  We might also make things too complicated for users if
we did that.  Our goal should be to make the obvious thing correct so that
people don't have to understand the implementation.

-Darin



>
> Does that make sense?
>
>
>
>
>
>
http://codereview.chromium.**org/7677028/<http://codereview.chromium.org/7677...
>

[ajwong, 2011-08-19 21:41:21]

On Fri, Aug 19, 2011 at 2:27 PM, Darin Fisher <darin@chromium.org> wrote:
>
>
> On Fri, Aug 19, 2011 at 12:45 PM, <sievers@chromium.org> wrote:
>>
>> Also, one more thing, consider this case:
>>
>> 1) Thread A creates one or more weak reference to an object
>> 2) Thread A deletes all weak references to the object
>> 3) Thread B deletes the object
>>
>> Before this change, with 2) happening we were already deleting the flag,
>> which
>> means that WeakReference::Flag::Invalidate() would not be called from the
>> owner
>> upon destruction. In other words, we would only hit the helpful
>> DCHECK(CalledOnValidThread()) in the (potentially rare case) of 2)
>> actually
>> losing the race against 3).
>>
>> Now, after my change I am already hitting this DCHECK() with
>> GpuMessageFilter
>> trying to be deleted on the UI thread, while we use weak references on the
>> IO
>> thread (to send back replies from GPU process to the renderer). Still need
>> to
>> see if this is a bug.
>>
>> I suggest for backwards compatibility that I relax the check in
>> Invalidate() to
>> 'DCHECK(thread_checker_.CalledOnValidThread())|| HasOneRef())' with a
>> follow-up
>> change removing the second part again.
>
> ^^^ Makes sense to be compatible like that.
>
>>
>> I suspect that hitting this might help uncover bugs. If this is
>> intentional,
>> i.e. the code explicitly synchronizes deletion of all weak pointers with
>> the
>> deletion of the object on another thread, we should require a call to
>> WeakReferenceOwner::DetachFromThread().
>
> It is hard to say.  We might also make things too complicated for users if
> we did that.  Our goal should be to make the obvious thing correct so that
> people don't have to understand the implementation.

I don't think they need to dive into implementation details.  Can we
solve this with some documentation like this:

THREAD SAFETY

WeakPtrFactory:
  * By default WeakPtrFactory must only be constructed/used/destructed
on the thread that created it.
  * If WeakPtrFactory is meant to be deleted on another thread, call
DetachFromThread(). External synchronization must be used to ensure no
WeakPtrs from this factory are outstanding before deletion.

WeakPtr:
  * WeakPtrs may be only be dereferenced or inspected on the thread
that created them.
  * However, WeakPtrs may be deleted on any other thread. This allows
passing them out and back.

...or something similar?

It'd be nice to retain the stricker DCHECK.

> -Darin
>
>>
>> Does that make sense?
>>
>>
>>
>>
>> http://codereview.chromium.org/7677028/
>
>

[darin (slow to review), 2011-08-19 23:21:20]

On Fri, Aug 19, 2011 at 2:40 PM, Albert Wong (王重傑) <ajwong@google.com>wrote:

> On Fri, Aug 19, 2011 at 2:27 PM, Darin Fisher <darin@chromium.org> wrote:
> >
> >
> > On Fri, Aug 19, 2011 at 12:45 PM, <sievers@chromium.org> wrote:
> >>
> >> Also, one more thing, consider this case:
> >>
> >> 1) Thread A creates one or more weak reference to an object
> >> 2) Thread A deletes all weak references to the object
> >> 3) Thread B deletes the object
> >>
> >> Before this change, with 2) happening we were already deleting the flag,
> >> which
> >> means that WeakReference::Flag::Invalidate() would not be called from
> the
> >> owner
> >> upon destruction. In other words, we would only hit the helpful
> >> DCHECK(CalledOnValidThread()) in the (potentially rare case) of 2)
> >> actually
> >> losing the race against 3).
> >>
> >> Now, after my change I am already hitting this DCHECK() with
> >> GpuMessageFilter
> >> trying to be deleted on the UI thread, while we use weak references on
> the
> >> IO
> >> thread (to send back replies from GPU process to the renderer). Still
> need
> >> to
> >> see if this is a bug.
> >>
> >> I suggest for backwards compatibility that I relax the check in
> >> Invalidate() to
> >> 'DCHECK(thread_checker_.CalledOnValidThread())|| HasOneRef())' with a
> >> follow-up
> >> change removing the second part again.
> >
> > ^^^ Makes sense to be compatible like that.
> >
> >>
> >> I suspect that hitting this might help uncover bugs. If this is
> >> intentional,
> >> i.e. the code explicitly synchronizes deletion of all weak pointers with
> >> the
> >> deletion of the object on another thread, we should require a call to
> >> WeakReferenceOwner::DetachFromThread().
> >
> > It is hard to say.  We might also make things too complicated for users
> if
> > we did that.  Our goal should be to make the obvious thing correct so
> that
> > people don't have to understand the implementation.
>
> I don't think they need to dive into implementation details.  Can we
> solve this with some documentation like this:
>
> THREAD SAFETY
>
> WeakPtrFactory:
>  * By default WeakPtrFactory must only be constructed/used/destructed
> on the thread that created it.
>  * If WeakPtrFactory is meant to be deleted on another thread, call
> DetachFromThread(). External synchronization must be used to ensure no
> WeakPtrs from this factory are outstanding before deletion.
>
> WeakPtr:
>  * WeakPtrs may be only be dereferenced or inspected on the thread
> that created them.
>  * However, WeakPtrs may be deleted on any other thread. This allows
> passing them out and back.
>
> ...or something similar?
>

^^^ This is pretty good text.

-Darin




>
> It'd be nice to retain the stricker DCHECK.
>
> > -Darin
> >
> >>
> >> Does that make sense?
> >>
> >>
> >>
> >>
> >> http://codereview.chromium.org/7677028/
> >
> >
>

[no sievers, 2011-08-20 00:10:30]

I have added the unit tests you suggested (the last 3), while I combined the
last two of your scenarios, since they were only different in an extra copy.

I have also added MoveOwnerShip and DetachFromThread. The latter one shows the
DCHECK() problem i mentioned *if* you were to omit the DetachFromThread() (and
the DCHECK() was strict).

It's quite possible that there is a more elegant solution to working with the
background thread than the interface I slammed in there :)

[awong, 2011-08-23 00:11:15]

LGTM w/ style nits

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
File base/memory/weak_ptr_unittest.cc (right):

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:48: Start();
This constitutes work in a constructor which the styleguide disallows. :(

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:51: void CreateConsumer(Consumer** consumer,
Producer* producer) {
Overloaded functions are discouraged by the style guide. :(

How about CreateConsumerFromProducer and CreateConsumerFromConsumer?

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:251: TEST(WeakPtrTest, MoveOwnerShip) {
OwnerShip -> Ownership

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:260: Producer* producer = new Producer;
new Producer();

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:292: // - Destruct the pointer on thread B
Capitalize "thread"

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:307: // - Destruct the pointer on thread A
Capitalize "thread."

http://codereview.chromium.org/7677028/diff/16001/base/memory/weak_ptr_unitte...
base/memory/weak_ptr_unittest.cc:324: // - WeakReferenceOwner gets destroyed on
thread A
Capitalize "thread."

[Timur Iskhodzhanov, 2011-08-23 08:20:58]

Are you aware this change has introduced (or uncovered?) some leaks that made
the memory waterfall red

[cbentzel, 2011-08-23 12:16:52]

He replied personally. I reverted this morning.

On Tue, Aug 23, 2011 at 4:20 AM,  <timurrrr@chromium.org> wrote:
> Are you aware this change has introduced (or uncovered?) some leaks that
> made
> the memory waterfall red
>
> http://codereview.chromium.org/7677028/
>

[Timur Iskhodzhanov, 2011-08-23 12:51:46]

Thanks!

[no sievers, 2011-08-23 18:01:10]

All the supposed leaks look the same:

The leak is a WeakReference::Flag allocated through ObserverListBase::AsWeakPtr
for an ObserverListIterator used before and after running a task. The list
itself is MessageLoop::task_observers_.

With my change the deletion of the flag is delayed until task_observers_ goes
away and since this is a member of MessageLoop, I'm wondering if either
MessageLoop is leaking or if it is deleted in a way that valgrind does not see.
There seem to be some valgrind issues (also on ChromeOS) that might look
related.

Also, interesting that this does not affect Mac, it shows up on ChromeOS all
over the place, and on Linux only in some tests.

[Timur Iskhodzhanov, 2011-08-24 05:40:26]

On Tue, Aug 23, 2011 at 10:01 PM,  <sievers@chromium.org> wrote:
> Also, interesting that this does not affect Mac, it shows up on ChromeOS all
> over the place, and on Linux only in some tests.
Unfortunately, we don't run any reasonable UI tests under Valgrind on
Mac since recently [if you open the UI Mac bots output you'll notice
it's 0 tests]

[no sievers, 2011-08-24 23:08:30]

Ok, I have investigated the problem with the unit_tests.
It is indeed a more-or-less bogus leak and I can also reproduce it with adding
some other scoped_ptr<> object to MessageLoop.

It is specific to a combination of tests run in certain order:
First a test that relies on AtExitManager() to clean up a MessageLoop instance
(here: AudioRendererHostTest with a singleton msg loop created by AudioManager,
see audio_manager.cc) and a test that intentionally crashes through EXPECT_DEATH
or EXPECT_DEBUG_DEATH (here: BrowserMainTest.WarmConnectionFieldTrial_Invalid or
BrowserAboutHandlerTest.WillHandleBrowserAboutURL).

Running more than test in a single run/shard creates a single process and AtExit
callbacks don't happen until the last test finishes. This itself is fine with
regard to this problem, but when a following test crashes, which in gtest
happens through forking first, we end up leaking in the child(!) process. 

Judging from the logs, the Mac unit_tests don't show the leak because the
affected tests happen not to be run in the specific order needed for this to
happen.

I assume that the ui_test leaks are also coming from some sort of MessageLoop
leak. I'll take a look, since there is at least one or two  that show it on
Linux.

The unit_tests leak can be avoided by cleaning up in the test itself, but a more
general solution to the problem would also be nice.

[awong, 2011-08-24 23:15:48]

It sounds to me like a DEATH test should somehow not be heapchecked.
Am I wrong?

On Wed, Aug 24, 2011 at 4:08 PM,  <sievers@chromium.org> wrote:
> Ok, I have investigated the problem with the unit_tests.
> It is indeed a more-or-less bogus leak and I can also reproduce it with
> adding
> some other scoped_ptr<> object to MessageLoop.
>
> It is specific to a combination of tests run in certain order:
> First a test that relies on AtExitManager() to clean up a MessageLoop
> instance
> (here: AudioRendererHostTest with a singleton msg loop created by
> AudioManager,
> see audio_manager.cc) and a test that intentionally crashes through
> EXPECT_DEATH
> or EXPECT_DEBUG_DEATH (here:
> BrowserMainTest.WarmConnectionFieldTrial_Invalid or
> BrowserAboutHandlerTest.WillHandleBrowserAboutURL).
>
> Running more than test in a single run/shard creates a single process and
> AtExit
> callbacks don't happen until the last test finishes. This itself is fine
> with
> regard to this problem, but when a following test crashes, which in gtest
> happens through forking first, we end up leaking in the child(!) process.
>
> Judging from the logs, the Mac unit_tests don't show the leak because the
> affected tests happen not to be run in the specific order needed for this to
> happen.
>
> I assume that the ui_test leaks are also coming from some sort of
> MessageLoop
> leak. I'll take a look, since there is at least one or two  that show it on
> Linux.
>
> The unit_tests leak can be avoided by cleaning up in the test itself, but a
> more
> general solution to the problem would also be nice.
>
>
> http://codereview.chromium.org/7677028/
>

[ajwong, 2011-08-24 23:16:20]

(btw, nice job tracking that down...that is a tricky sequence)

On Wed, Aug 24, 2011 at 4:15 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org> wrote:
> It sounds to me like a DEATH test should somehow not be heapchecked.
> Am I wrong?
>
> On Wed, Aug 24, 2011 at 4:08 PM,  <sievers@chromium.org> wrote:
>> Ok, I have investigated the problem with the unit_tests.
>> It is indeed a more-or-less bogus leak and I can also reproduce it with
>> adding
>> some other scoped_ptr<> object to MessageLoop.
>>
>> It is specific to a combination of tests run in certain order:
>> First a test that relies on AtExitManager() to clean up a MessageLoop
>> instance
>> (here: AudioRendererHostTest with a singleton msg loop created by
>> AudioManager,
>> see audio_manager.cc) and a test that intentionally crashes through
>> EXPECT_DEATH
>> or EXPECT_DEBUG_DEATH (here:
>> BrowserMainTest.WarmConnectionFieldTrial_Invalid or
>> BrowserAboutHandlerTest.WillHandleBrowserAboutURL).
>>
>> Running more than test in a single run/shard creates a single process and
>> AtExit
>> callbacks don't happen until the last test finishes. This itself is fine
>> with
>> regard to this problem, but when a following test crashes, which in gtest
>> happens through forking first, we end up leaking in the child(!) process.
>>
>> Judging from the logs, the Mac unit_tests don't show the leak because the
>> affected tests happen not to be run in the specific order needed for this to
>> happen.
>>
>> I assume that the ui_test leaks are also coming from some sort of
>> MessageLoop
>> leak. I'll take a look, since there is at least one or two  that show it on
>> Linux.
>>
>> The unit_tests leak can be avoided by cleaning up in the test itself, but a
>> more
>> general solution to the problem would also be nice.
>>
>>
>> http://codereview.chromium.org/7677028/
>>
>

[no sievers, 2011-08-24 23:21:50]

Yes, I'd also think the child process which is just created to be killed should
not be checked. 

Generally making valgrind not follow fork'ed() children does not seem to be
possible (maybe also not what we want?).

I have not looked at how memcheck.py works, but you can have valgrind log to
different files by using the pid in the logfilename.

Create a different issue for this I guess?



On 2011/08/24 23:15:48, awong wrote:
> It sounds to me like a DEATH test should somehow not be heapchecked.
> Am I wrong?
> 
> On Wed, Aug 24, 2011 at 4:08 PM,  <mailto:sievers@chromium.org> wrote:
> > Ok, I have investigated the problem with the unit_tests.
> > It is indeed a more-or-less bogus leak and I can also reproduce it with
> > adding
> > some other scoped_ptr<> object to MessageLoop.
> >
> > It is specific to a combination of tests run in certain order:
> > First a test that relies on AtExitManager() to clean up a MessageLoop
> > instance
> > (here: AudioRendererHostTest with a singleton msg loop created by
> > AudioManager,
> > see audio_manager.cc) and a test that intentionally crashes through
> > EXPECT_DEATH
> > or EXPECT_DEBUG_DEATH (here:
> > BrowserMainTest.WarmConnectionFieldTrial_Invalid or
> > BrowserAboutHandlerTest.WillHandleBrowserAboutURL).
> >
> > Running more than test in a single run/shard creates a single process and
> > AtExit
> > callbacks don't happen until the last test finishes. This itself is fine
> > with
> > regard to this problem, but when a following test crashes, which in gtest
> > happens through forking first, we end up leaking in the child(!) process.
> >
> > Judging from the logs, the Mac unit_tests don't show the leak because the
> > affected tests happen not to be run in the specific order needed for this to
> > happen.
> >
> > I assume that the ui_test leaks are also coming from some sort of
> > MessageLoop
> > leak. I'll take a look, since there is at least one or two &nbsp;that show
it on
> > Linux.
> >
> > The unit_tests leak can be avoided by cleaning up in the test itself, but a
> > more
> > general solution to the problem would also be nice.
> >
> >
> > http://codereview.chromium.org/7677028/
> >

[awong, 2011-08-24 23:33:58]

On Wed, Aug 24, 2011 at 4:21 PM,  <sievers@chromium.org> wrote:
>
> Yes, I'd also think the child process which is just created to be killed
> should
> not be checked.
>
> Generally making valgrind not follow fork'ed() children does not seem to be
> possible (maybe also not what we want?).
>
> I have not looked at how memcheck.py works, but you can have valgrind log to
> different files by using the pid in the logfilename.
>
> Create a different issue for this I guess?

Yeah, this sounds like a separate problem without a simple solution.
You mentioned that you could avoid the specific issue with unit_tests
here.  Maybe just submit with that an file a bug for the broader issue
of bad interaction between valgrind and death tests.

>
>
>
> On 2011/08/24 23:15:48, awong wrote:
>>
>> It sounds to me like a DEATH test should somehow not be heapchecked.
>> Am I wrong?
>
>> On Wed, Aug 24, 2011 at 4:08 PM,  <mailto:sievers@chromium.org> wrote:
>> > Ok, I have investigated the problem with the unit_tests.
>> > It is indeed a more-or-less bogus leak and I can also reproduce it with
>> > adding
>> > some other scoped_ptr<> object to MessageLoop.
>> >
>> > It is specific to a combination of tests run in certain order:
>> > First a test that relies on AtExitManager() to clean up a MessageLoop
>> > instance
>> > (here: AudioRendererHostTest with a singleton msg loop created by
>> > AudioManager,
>> > see audio_manager.cc) and a test that intentionally crashes through
>> > EXPECT_DEATH
>> > or EXPECT_DEBUG_DEATH (here:
>> > BrowserMainTest.WarmConnectionFieldTrial_Invalid or
>> > BrowserAboutHandlerTest.WillHandleBrowserAboutURL).
>> >
>> > Running more than test in a single run/shard creates a single process
>> > and
>> > AtExit
>> > callbacks don't happen until the last test finishes. This itself is fine
>> > with
>> > regard to this problem, but when a following test crashes, which in
>> > gtest
>> > happens through forking first, we end up leaking in the child(!)
>> > process.
>> >
>> > Judging from the logs, the Mac unit_tests don't show the leak because
>> > the
>> > affected tests happen not to be run in the specific order needed for
>> > this to
>> > happen.
>> >
>> > I assume that the ui_test leaks are also coming from some sort of
>> > MessageLoop
>> > leak. I'll take a look, since there is at least one or two &nbsp;that
>> > show
>
> it on
>>
>> > Linux.
>> >
>> > The unit_tests leak can be avoided by cleaning up in the test itself,
>> > but a
>> > more
>> > general solution to the problem would also be nice.
>> >
>> >
>> > http://codereview.chromium.org/7677028/
>> >
>
>
>
> http://codereview.chromium.org/7677028/
>

[no sievers, 2011-08-24 23:56:15]

http://code.google.com/p/chromium/issues/detail?id=94174 filed.

I'll try to work around the unit_tests leak in the meantime and will also take a
look at ui_tests, as far as I can reproduce that (but might suppress those for
now and file a different bug, so I can land this change).

[Timur Iskhodzhanov, 2011-08-25 07:48:44]

> It sounds to me like a DEATH test should somehow not be heapchecked.
Why? If some memory is not free'd but still reachable it's not regarded as a
leak.

> Generally making valgrind not follow fork'ed() children does
> not seem to be possible (maybe also not what we want?).
Definitely not what we want. Think ui_tests spawning a few chrome processes per
test.

> but you can have valgrind log to
> different files by using the pid in the logfilename.
This is exactly what we do already

On 2011/08/24 23:56:15, Daniel Sievers wrote:
> http://code.google.com/p/chromium/issues/detail?id=94174 filed.
Let's continue the discussion there

> I'll try to work around the unit_tests leak in the meantime and will also take
a
> look at ui_tests, as far as I can reproduce that (but might suppress those for
> now and file a different bug, so I can land this change).

[awong, 2011-08-25 21:10:23]

On 2011/08/25 07:48:44, Timur Iskhodzhanov wrote:
> > It sounds to me like a DEATH test should somehow not be heapchecked.
> Why? If some memory is not free'd but still reachable it's not regarded as a
> leak.

With an ASSERT_DEATH, you're basically saying you're going to cause a process to
un-nicely terminate.  This might be SIGSEGV, or otherwise, which is going to
skip any sort of cleanup code.  I don't think having valgrind check for leaks
when you are forcing a crash is really useful is it?

 
> > Generally making valgrind not follow fork'ed() children does
> > not seem to be possible (maybe also not what we want?).
> Definitely not what we want. Think ui_tests spawning a few chrome processes
per
> test.
> 
> > but you can have valgrind log to
> > different files by using the pid in the logfilename.
> This is exactly what we do already
> 
> On 2011/08/24 23:56:15, Daniel Sievers wrote:
> > http://code.google.com/p/chromium/issues/detail?id=94174 filed.
> Let's continue the discussion there
> 
> > I'll try to work around the unit_tests leak in the meantime and will also
take
> a
> > look at ui_tests, as far as I can reproduce that (but might suppress those
for
> > now and file a different bug, so I can land this change).

[awong, 2011-08-26 00:50:23]

LGTM

I think I've convinced myself that this isn't nearly as general as it looks
since WeakPtr + friends are all smartpointers making this leak almost impossible
to code up programmatically.

[no sievers, 2011-08-26 01:06:29]

For reference, here is a demangled stack:

operator new(unsigned int)
base::internal::WeakReferenceOwner::GetRef() const 
base::SupportsWeakPtr<ObserverListBase<MessageLoop::TaskObserver>>::AsWeakPtr()

ObserverListBase<MessageLoop::TaskObserver>::Iterator::Iterator(ObserverListBase<MessageLoop::TaskObserver>&)
MessageLoop::RunTask(MessageLoop::PendingTask const&)

[Timur Iskhodzhanov, 2011-08-26 09:56:28]

On 2011/08/25 21:10:23, awong wrote:
> On 2011/08/25 07:48:44, Timur Iskhodzhanov wrote:
> > > It sounds to me like a DEATH test should somehow not be heapchecked.
> > Why? If some memory is not free'd but still reachable it's not regarded as a
> > leak.
> With an ASSERT_DEATH, you're basically saying you're going to cause a process
to
> un-nicely terminate.  This might be SIGSEGV, or otherwise, which is going to
> skip any sort of cleanup code.
What I'm saying is that Valgrind is not treating an allocated memory as a leak
if there are pointers referencing it.
If there are - it's not a leak. If there are no pointers to the allocated memory
then you can't free it.

You don't remove any pointers intentionally right before SIGSEGV'ing, do you? :)

> I don't think having valgrind check for leaks when you are forcing a crash is
really useful is it?
That's a good question btw
I've updated https://bugs.kde.org/show_bug.cgi?id=265371 accordingly.

[awong, 2011-08-26 15:50:23]

On Fri, Aug 26, 2011 at 2:56 AM,  <timurrrr@chromium.org> wrote:
> On 2011/08/25 21:10:23, awong wrote:
>>
>> On 2011/08/25 07:48:44, Timur Iskhodzhanov wrote:
>> > > It sounds to me like a DEATH test should somehow not be heapchecked.
>> > Why? If some memory is not free'd but still reachable it's not regarded
>> > as a
>> > leak.
>> With an ASSERT_DEATH, you're basically saying you're going to cause a
>> process
>
> to
>>
>> un-nicely terminate.  This might be SIGSEGV, or otherwise, which is going
>> to
>> skip any sort of cleanup code.
>
> What I'm saying is that Valgrind is not treating an allocated memory as a
> leak
> if there are pointers referencing it.
> If there are - it's not a leak. If there are no pointers to the allocated
> memory
> then you can't free it.
>
> You don't remove any pointers intentionally right before SIGSEGV'ing, do
> you? :)

Ah, I get what you're saying now.  Yes...there probably shouldn't be
leaks then in our injected crashes. Ugh. :-/

Though as a separate point, if it's a random SIGSEGV (ostensibly cause
someone is doing mistakes with their pointers), who knows what's
happened to memory before hand...

>
>> I don't think having valgrind check for leaks when you are forcing a crash
>> is
>
> really useful is it?
> That's a good question btw
> I've updated https://bugs.kde.org/show_bug.cgi?id=265371 accordingly.

I think we also die with SIGABRT on a CHECK, and maybe SIGTRAP if the
CHECK happens under a debugger.  I'm pretty sure it's not SIGTERM, or
SIGSEGV.  So if we're asserting a CHECK, there are other signals that
will kill the program.

-Albert

[no sievers, 2011-08-26 17:42:26]

> What I'm saying is that Valgrind is not treating an allocated memory as a leak
> if there are pointers referencing it.

Maybe that explains why my naive test in http://crbug.com/94174 does not show a
leak, while this issue does. Should probably continue the discussion in that
bug. 

Since this is a 10-headed hydra (i.e. there are many ui tests apparently with a
similar msg loop leak and the same leakl stack), and it is really not an issue
with this change itself at all, I added a suppression for now. Could you
double-check it?