scoped_refptr<T>::operator T*

[David Michael]

Hi all,

I noticed last week in a code review that scoped_refptr has the following conversion operator:

operator T*() const { return ptr_; }

...which allows a scoped_refptr to be silently & implicitly converted to a raw pointer. In the CL I was reviewing, this was a mistake, and would have caused (in that case) a memory leak. In other cases, it could easily lead to a use-after-free.

It's generally a bad idea to mix & match raw pointers with smart pointers, hence why there is no such conversion in C++'s shared_ptr, or scoped_ptr (either C++'s or Chromium's). I would like to remove this conversion operator. If you *really, really* mean to convert to a raw pointer, you should explicitly call get().

There seem to be a lot of places where a scoped_refptr is evaluated in a context where a boolean is needed, so I'm happy to add a safe way to do that.

Thoughts? Is there any objection to removing the conversion operator?

-Dave

[Daniel Cheng]

For what it's worth, I think base::Callback uses this implicit conversion at the moment. Not sure what the best way to address this is, since the callback system allows all of the following to work:

void Example1(RefCountedT* x);

// Results in extra refcount churn; please avoid in normal code.

void Example2(scoped_refptr<RefCountedT> x);

void Example3(const scoped_refptr<RefCountedT>& x);

using base::Bind;

using base::Closure;

int main() {

  scoped_refptr<RefCountedT> instance(RefCountedT::CreateInstance());

  Closure c1 = Bind(&Example1, instance);

  Closure c2 = Bind(&Example2, instance);

  Closure c3 = Bind(&Example3, instance);

}

Daniel

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

[Albert J. Wong (王重傑)]

willchan@ also made this observation a while ago.  I think the main argument against using a scoped_refptr in all the parameter and return types is that it may cause extra AddRef/Release calls.  But I betcha that in almost all code, this isn't really a serious cost.  If it really bugs people, we could probably use move semantics to remove a large chunk of the extra copies.

For base::Callback, the change would be that the "Closure c1 = Bind(&Example1, instance);" call would not compile.  That's not horrible as the extra AddRef/Release pair seem unlikely to really cost you much if you're already dispatching through a Callback.

IMO, removing the implicit conversion would be nice though it might take a lot of work since I think lots of code depends on it.

-Albert

[Daniel Cheng]

It's probably not a huge cost. However, the proposed change would make it impossible to avoid the extra AddRef/Release--from what I remember, example 3 is currently discouraged/banned for being too subtle. It'd be nice to make this change while still allowing people to easily avoid refcount churn if they wanted, whether it involves allowing example 3 or adding move semantics.

Daniel

[Albert J. Wong (王重傑)]

On Tue, Jan 17, 2012 at 6:06 PM, Daniel Cheng <dch...@chromium.org> wrote:

It's probably not a huge cost. However, the proposed change would make it impossible to avoid the extra AddRef/Release--from what I remember, example 3 is currently discouraged/banned for being too subtle. It'd be nice to make this change while still allowing people to easily avoid refcount churn if they wanted, whether it involves allowing example 3 or adding move semantics.

Huh...I didn't know example 3 was discouraged.  For the record, it works completely correctly.  The Callback object owns the actual ref-pointer, and the function invocation only gets a const-reference to it.

[David Michael]

On Tue, Jan 17, 2012 at 7:10 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

On Tue, Jan 17, 2012 at 6:06 PM, Daniel Cheng <dch...@chromium.org> wrote:

It's probably not a huge cost. However, the proposed change would make it impossible to avoid the extra AddRef/Release--from what I remember, example 3 is currently discouraged/banned for being too subtle. It'd be nice to make this change while still allowing people to easily avoid refcount churn if they wanted, whether it involves allowing example 3 or adding move semantics.

Huh...I didn't know example 3 was discouraged.  For the record, it works completely correctly.  The Callback object owns the actual ref-pointer, and the function invocation only gets a const-reference to it.

...and the reference counting overhead should be equivalent to using a raw pointer, since the Callback has to take a reference either way. Glancing through bind_helpers.h, I see a partial specialization for scoped_refptr that avoids doing any unnecessary AddRef/RemoveRef when a scoped_refptr is used.

Is it true that passing scoped_refptr by const-ref is discouraged (officially or by consensus)? If so, what's the reasoning?

I'll continue hacking on the change and see if I turn up real bugs (and to see how deep this rabbit hole goes).

[Daniel Cheng]

On Jan 18, 2012 8:08 AM, "David Michael" <dmichael@chromium.org> wrote:
>
> On Tue, Jan 17, 2012 at 7:10 PM, Albert J. Wong (王重傑) <ajwong@chromium.org> wrote:

>>
>> On Tue, Jan 17, 2012 at 6:06 PM, Daniel Cheng <dcheng@chromium.org> wrote:
>>>
>>> It's probably not a huge cost. However, the proposed change would make it impossible to avoid the extra AddRef/Release--from what I remember, example 3 is currently discouraged/banned for being too subtle. It'd be nice to make this change while still allowing people to easily avoid refcount churn if they wanted, whether it involves allowing example 3 or adding move semantics.
>>
>>
>> Huh...I didn't know example 3 was discouraged.  For the record, it works completely correctly.  The Callback object owns the actual ref-pointer, and the function invocation only gets a const-reference to it.
>
> ...and the reference counting overhead should be equivalent to using a raw pointer, since the Callback has to take a reference either way. Glancing through bind_helpers.h, I see a partial specialization for scoped_refptr that avoids doing any unnecessary AddRef/RemoveRef when a scoped_refptr is used.

With a raw pointer, only the bind state holds a ref. With scoped_refptr<T>, an additional

>
> Is it true that passing scoped_refptr by const-ref is discouraged (officially or by consensus)? If so, what's the reasoning?

It looks the discussion was at https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/9e83addf85d2b445/56905ed52bcc70e9?lnk=gst

>>>>>> Chromium Developers mailing list: chromium-dev@chromium.org

>>>>>> View archives, change email options, or unsubscribe:
>>>>>> http://groups.google.com/a/chromium.org/group/chromium-dev
>>>>>
>>>>>
>>>>> --

>>>>> Chromium Developers mailing list: chromium-dev@chromium.org

[Daniel Cheng]

Argh, the first inline comment should read:
Right, that's why I think we should consider allowing it if we remove the implicit conversion.

(Sorry for the spam)

Daniel

[David Michael]

Quick update, looks like there are about 450 unique parts of the code that rely on the implicit conversion. I started a bug here to track it:
http://code.google.com/p/chromium/issues/detail?id=110610

On Wed, Jan 18, 2012 at 9:23 AM, Daniel Cheng <dch...@chromium.org> wrote:

Argh, the first inline comment should read:

Right, that's why I think we should consider allowing it if we remove the implicit conversion.

Can you clarify what you're asking for? Do you want Bind to allow you to send raw pointers when scoped_refptrs are expected by the bound method, and/or vice-versa? That should be possible, though it still carries some risk that it will be too easy to mix and match and make a mistake. I think it should be acceptable to force developers to either call .get() or wrap their raw pointer in a scoped_ptr. Having the compiler complain about it if you forget at least makes you have to stop and think about what you're doing.

(Sorry for the spam)

Daniel

On Jan 18, 2012 8:21 AM, "Daniel Cheng" <dch...@chromium.org> wrote:

On Jan 18, 2012 8:08 AM, "David Michael" <dmichael@chromium.org> wrote:
>
> On Tue, Jan 17, 2012 at 7:10 PM, Albert J. Wong (王重傑) <ajwong@chromium.org> wrote:
>>
>> On Tue, Jan 17, 2012 at 6:06 PM, Daniel Cheng <dcheng@chromium.org> wrote:
>>>
>>> It's probably not a huge cost. However, the proposed change would make it impossible to avoid the extra AddRef/Release--from what I remember, example 3 is currently discouraged/banned for being too subtle. It'd be nice to make this change while still allowing people to easily avoid refcount churn if they wanted, whether it involves allowing example 3 or adding move semantics.
>>
>>
>> Huh...I didn't know example 3 was discouraged.  For the record, it works completely correctly.  The Callback object owns the actual ref-pointer, and the function invocation only gets a const-reference to it.
>
> ...and the reference counting overhead should be equivalent to using a raw pointer, since the Callback has to take a reference either way. Glancing through bind_helpers.h, I see a partial specialization for scoped_refptr that avoids doing any unnecessary AddRef/RemoveRef when a scoped_refptr is used.

With a raw pointer, only the bind state holds a ref. With scoped_refptr<T>, an additional
>
> Is it true that passing scoped_refptr by const-ref is discouraged (officially or by consensus)? If so, what's the reasoning?

It looks the discussion was at https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/9e83addf85d2b445/56905ed52bcc70e9?lnk=gst

Thanks for the link. I was a n00b at the time or I would have joined up with willchan with dissenting opinion :-)  I just don't see the harm in "const scoped_ptr<X>&" (especially as compared to X*), though I'm fine with passing the scoped_refptr "by value" too, since the refcounting overhead is pretty small. If there's no ownership transfer or change to the pointed-to object, then "const X&" is probably best.

Maybe it would help to provide a "get_ref()" function that returns a reference instead of a pointer? Then, if you're passing a ref-counted object but the recipient should not affect the ref-count, you would make it look like this:

void Foo::DoSomething(const Bar& bar);

And call it like this:

my_foo.DoSomething(ptr_to_bar.get_ref());

To me, passing a raw pointer is an indication of either ownership transfer, out-param, or both. Passing scoped_refptr carries rougly the same baggage, whether you pass it by const-ref or not. const-ref is just a small optimization.

[Daniel Cheng]

On Wed, Jan 18, 2012 at 11:14, David Michael <dmic...@chromium.org> wrote:

Can you clarify what you're asking for? Do you want Bind to allow you to send raw pointers when scoped_refptrs are expected by the bound method, and/or vice-versa? That should be possible, though it still carries some risk that it will be too easy to mix and match and make a mistake. I think it should be acceptable to force developers to either call .get() or wrap their raw pointer in a scoped_ptr. Having the compiler complain about it if you forget at least makes you have to stop and think about what you're doing.

base::Bind() does some special things with RefCountedT's:

- It has a compile assert if you try to bind to a raw pointer of a ref-counted type, so developers are already forced to wrap their raw pointer to RefCountedT in a scoped_refptr. That means we can't bind an argument to the result of .get().

- It takes a copy of the scoped_refptr<RefCountedT> argument so that the object is guaranteed to be alive when the callback is dispatched.

Today, there's two ways of avoiding extra refcounts--using a raw pointer or using const scoped_refptr<>&. You proposed removing the first, and the second isn't allowed. Ultimately, I don't really care which option is preferred/remains, but I do think that there should be a way to avoid extra refcounts when binding arguments since it's never needed there.

Daniel

[Albert J. Wong (王重傑)]

FWIW, I would avoid requiring that if you did Bind(&Foo, a_scoped_refptr_to_T), that Foo must be "void Foo(T*)".  That adds more magic into Bind() which is already pretty magical.

Also, reading the last thread and this one, it feels like we're trading compile-time checks for a non-universal style preference.  That seems odd.

Throw my vote in favor of allowing "const scoped_refptr<>&" and killing off the implicit raw-pointer conversion.

-Albert