scoped_refptr::release may lead to memory leaks

[Xiaoming Shi]

scoped_refptr::release (base/ref_counted.h) may lead to memory leaks. It returns the contained pointer without decreasing its reference count. This usually results in a memory leak if the contained pointer is not NULL.

There are places using scoped_refptr::release, for example net/http/http_response_headers.cc and net/dns/dns_client.cc.  Usually what the callers wants is to reset the pointer to NULL.

As "release" on an auto_ptr traditionally means "return the pointer and give up the ownership", it might be hard to implement the same semantic in scoped_refptr. So maybe we should remove the function totally.

I have already discussed this with ajwong and was suggested to post in the group so we can have a better idea on what should do.

[Elliot Glaysher (Chromium)]

+darin, who explained to me why things are the way they are the last
time I came up with this idea.

> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Nico Weber]

Prior art: https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/V3wz3iDCTLA

On Wed, Aug 22, 2012 at 1:33 PM, Xiaoming Shi <xiao...@google.com> wrote:

[Fred Akalin]

If we end up still needing release(), it may suffice to make its
return value non-ignorable to weed out the erroneous uses.

[James Hawkins]

Under which circumstances should scoped_refptr::release be used, i.e., what were the intentions of the authors who added the method?  Is there a way to use it safely?

Thanks,

James

On Wed, Aug 22, 2012 at 1:33 PM, Xiaoming Shi <xiao...@google.com> wrote:

--

[Szymon Jakubczak]

Thanks for catching the leak in DnsClientImpl. That's a bug. It should
be setting it to NULL.

[Xiaohan Wang (王消寒)]

I was bitten by this before and had to fix a memory leak afterwards. Maybe change the name so it won't be misused so easily?

[Albert J. Wong (王重傑)]

I just read the PriorArt thread...maybe I missed something but couldn't the pattern of

   scoped_refptr<Foo> f = getFromSomewhere();

   f->AddRef();

   f.reset();

that shess@ was suggesting basically cover all use cases at the cost of one extra addref/deref?   The previous thread seemed to skip over this option in favor of arguing about leak_ref().

Having release() on scoped_refptr make the bar to mismatching your refcounts just-that-much-lower.  If there isn't evidence that we really need it, can we bias towards killing it?

-Albert

[Ryan Hamilton]

+1.  I too was bitten by this once.  Thankfully, valgrind (I think) caught it.

On Wed, Aug 22, 2012 at 4:22 PM, Xiaohan Wang (王消寒) <xhw...@google.com> wrote:

[Xiaoming Shi]

I did a brief check on the places that call scoped_refptr::release in chrome main projects on Linux. Quite a few of them have possible leaks and some others is not necessary. Of course there are cases that uses it properly, i.e. call Release on the result pointer after using it.

I am going to start a CL (or CLs) to remove the possible leaks and unnecessary use of release. Then we can have a better idea on how much work is left if we decide to remove it.

[Antoine Labour]

On Wed, Aug 22, 2012 at 7:21 PM, Xiaoming Shi <xiao...@google.com> wrote:

I did a brief check on the places that call scoped_refptr::release in chrome main projects on Linux. Quite a few of them have possible leaks and some others is not necessary. Of course there are cases that uses it properly, i.e. call Release on the result pointer after using it.

I am going to start a CL (or CLs) to remove the possible leaks and unnecessary use of release. Then we can have a better idea on how much work is left if we decide to remove it.

Reviving an old thread, got burned by this.

This works as intended:

scoped_ptr<T> a = CreateT();

scoped_ptr<T> b(a.release());

This leaks:

scoped_refptr<T> a = CreateT();

scoped_refptr<T> b(a.release());

Did your CL ever see the light of day? Should we rename scoped_refptr::release into leak_ref as was suggested in the other thread?

Antoine

[Xiaoming Shi]

The CL is https://chromiumcodereview.appspot.com/10874054/. Unfortunately I took the easy way, which only warns you if you don't use the result release. As there are still some code (around 5 places?) that uses scoped_refptr::release and then manage the life time of the returned pointer by themselves.

I had thought that add the warning was enough to solve this issue with minimum effort. Now it seems that I was wrong and the function has to be removed.

[Jonathan Dixon]

+1 for removing it (rather than rename to leak_ref)

if a user wants to do manual ref management, they can just call AddRef() themselves (with benefit that's more immediately obvious they're intentionally doing it manually, and it's easier to spot the balanced AddRef / Release pairs)

[Scott Hess]

Before someone says "A needless AddRef() is super inefficient and I'm
going to die", I suggest we just remove the function and let people
prove that the cost is too much to bear. Most likely 99% of uses are
so seldom that the cost isn't unbearable (after all, ref-counting code
is generally one of the first things systems optimize, right up there
with the malloc library), and the last 1% can be worked around in an
alternative fashion.

-scott

[Antoine Labour]

On Wed, Dec 5, 2012 at 1:50 PM, Scott Hess <sh...@chromium.org> wrote:

Before someone says "A needless AddRef() is super inefficient and I'm
going to die", I suggest we just remove the function and let people
prove that the cost is too much to bear.  Most likely 99% of uses are
so seldom that the cost isn't unbearable (after all, ref-counting code
is generally one of the first things systems optimize, right up there
with the malloc library), and the last 1% can be worked around in an
alternative fashion.

-scott

https://codereview.chromium.org/11458003 (patch 1) is what I intend to land (patch 2 contains another fix, needed for trybots), modulo other uses I didn't catch in my local build. There's not that many users, and at least a couple seem to be leaking (one in tests, and one in prod). The other uses are IMO more readable now (since we have an explicit AddRef/Release pair). I will land it piecemeal.

Antoine

[Tommi]

+1 for removing. It's confusing too that release()  does not do a Release().

[Antoine Labour]

On Wed, Dec 5, 2012 at 2:50 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Dec 5, 2012 at 1:50 PM, Scott Hess <sh...@chromium.org> wrote:

Before someone says "A needless AddRef() is super inefficient and I'm
going to die", I suggest we just remove the function and let people
prove that the cost is too much to bear.  Most likely 99% of uses are
so seldom that the cost isn't unbearable (after all, ref-counting code
is generally one of the first things systems optimize, right up there
with the malloc library), and the last 1% can be worked around in an
alternative fashion.

-scott

https://codereview.chromium.org/11458003 (patch 1) is what I intend to land (patch 2 contains another fix, needed for trybots), modulo other uses I didn't catch in my local build. There's not that many users, and at least a couple seem to be leaking (one in tests, and one in prod). The other uses are IMO more readable now (since we have an explicit AddRef/Release pair). I will land it piecemeal.

As of r171893, scoped_refptr<T>::release() is no more.

Antoine

[Ryan Hamilton]

On Fri, Dec 7, 2012 at 4:42 PM, Antoine Labour <pi...@chromium.org> wrote:

On Wed, Dec 5, 2012 at 2:50 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Dec 5, 2012 at 1:50 PM, Scott Hess <sh...@chromium.org> wrote:

Before someone says "A needless AddRef() is super inefficient and I'm
going to die", I suggest we just remove the function and let people
prove that the cost is too much to bear.  Most likely 99% of uses are
so seldom that the cost isn't unbearable (after all, ref-counting code
is generally one of the first things systems optimize, right up there
with the malloc library), and the last 1% can be worked around in an
alternative fashion.

-scott

https://codereview.chromium.org/11458003 (patch 1) is what I intend to land (patch 2 contains another fix, needed for trybots), modulo other uses I didn't catch in my local build. There's not that many users, and at least a couple seem to be leaking (one in tests, and one in prod). The other uses are IMO more readable now (since we have an explicit AddRef/Release pair). I will land it piecemeal.

As of r171893, scoped_refptr<T>::release() is no more.

Woo hoo!