PSA: scoped_refptr<T> CreateRefCountedT() is dangerous

[William Chan (陈智昌)]

akalin@ asked me this on IRC. Here's your warning. And yes, it'd be
awesome to catch this with a clang plugin. If you ask me, it'd be even
more awesome if we got rid of the implicit operator T*(), but I may be
in the minority here. I definitely used to be a few years back when I
asked about this.

==========

6:54 PM <akalin> for a RefCounted class with a static constructor-like
function, should that constructor return a scoped_refptr<>?
6:54 PM <akalin> will stuff blow up if it returns a raw pointer?
6:55 PM <akalin> i can never remember this stuff
6:56 PM <akalin> willchan: ^ you should know, right
6:56 PM <willchan> huh, i know nothing
6:56 PM <akalin> waffling illchan
6:57 PM <willchan> it's fine to return a raw pointer
6:57 PM <willchan> the object starts with a refcount of 0
6:57 PM <akalin> really?
6:57 PM <willchan> it's only once you put it into a scoped_refptr that
it's trouble
6:57 PM <willchan> actually, it's better to return a raw pointer
6:57 PM <akalin> what
6:57 PM <akalin> if the caller doesn't store it in a refptr, it'll leak, right
6:58 PM <willchan> since we have this stupid implicit conversion of
scoped_refptr<T> to T*
6:58 PM <willchan> so, if your code returns a scoped_refptr<T> and it
gets assigned to T*
6:58 PM <willchan> it's destroyed
6:58 PM <willchan> use-after-free

==========

willchan@panda:/mnt/hd2/src/chromium3/src$ tail -n 15
base/memory/ref_counted_unittest.cc
class Foo : public base::RefCounted<Foo> {
public:
Foo() { LOG(ERROR) << "Foo::Foo()"; }

static scoped_refptr<Foo> CreateFoo() { return new Foo; }
private:
friend class base::RefCounted<Foo>;
~Foo() { LOG(ERROR) << "Foo::~Foo()"; }
};


TEST(RefCountedUnitTest, ReturnRefPtr) {
Foo* foo = Foo::CreateFoo();
if (foo) LOG(ERROR) << "Got foo";
}
willchan@panda:/mnt/hd2/src/chromium3/src$ out/Debug/base_unittests
--gtest_filter=*ReturnRefPtr*
Note: Google Test filter = *ReturnRefPtr*
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from RefCountedUnitTest
[ RUN ] RefCountedUnitTest.ReturnRefPtr
[26227:26227:1019/162259:4326893578062:ERROR:ref_counted_unittest.cc(69)]
Foo::Foo()
[26227:26227:1019/162259:4326893578134:ERROR:ref_counted_unittest.cc(74)]
Foo::~Foo()
[26227:26227:1019/162259:4326893578157:ERROR:ref_counted_unittest.cc(80)]
Got foo
[ OK ] RefCountedUnitTest.ReturnRefPtr (0 ms)
[----------] 1 test from RefCountedUnitTest (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[ PASSED ] 1 test.

[Darin Fisher]

Sounds like we should work to eliminate the implicit conversion to T*.

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

[Ryan Sleevi]

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

One slight comfort with this is that, in most cases, someone will
eventually pass Foo* foo into an object that stores it in a
scoped_refptr<> it will generally trigger the DCHECK on |is_dtor_| and
be caught.

But yes, it's dangerous. Don't rely on anything I said above.

[Rachel Blum]

+1 on killing implicit conversions. Nothing good comes from invisible magic.

 - rachel

[Adam Barth]

In WebKit, we have operator-> and implicit conversion to bool, which
gets you most of what you want out of implicit conversion to T*.

Adam

[Jochen Eisinger]

Wasn't it better to convert to void* instead of bool? If you have a conversion to bool, you get "conversion" to integer for free...

-jochen

[Jeffrey Yasskin]

Search for "safe bool idiom", but that can be handled in the code
review after "kill operator T*()" is agreed on. +1 from me.

[William Chan (陈智昌)]

enne@ already did this for scoped_ptr, and we can do it for
scoped_refptr after we kill the implicit conversion, which it sounds
like everyone agrees on doing now.

[Wez]

reinterpret_cast<redundant>(+1)

[Tommi]

operator void*() sgtm.

[William Chan (陈智昌)]

I was about to file a bug, but I found that
https://code.google.com/p/chromium/issues/detail?id=110610 exists. And
there's interesting discussion in
https://groups.google.com/a/chromium.org/forum/?fromgroups=#!topic/chromium-dev/WiDO913UT2s
that I'm parsing right now.

[Slavomir Kaslev]

On Fri, Oct 19, 2012 at 5:07 PM, Jochen Eisinger <joc...@chromium.org> wrote:

Wasn't it better to convert to void* instead of bool? If you have a conversion to bool, you get "conversion" to integer for free...

There's

explicit operator bool() const { ... }

in C++11 that prevents the int conversion of bool operator.

if (p) { ... }

and

(p ? foo : bar)

work but all other conversions fail without an explicit bool(p).

[David Michael]

I'd actually started on this a long time ago and gave up due to the size of the job and lack of enthusiasm (mine and everyone else's). I finally uploaded from my git branch here:
http://codereview.chromium.org/11189146/diff/1/base/memory/ref_counted.h

In case it helps the next person who wants to tackle it. This uses the "safe bool" idiom mentioned above and also adds operator< (we apparently use scoped_refptr as a key in maps).

My plan of attack was to do the above part of the patch in a conditionally compiled section behind a gyp flag so we could land partial fixes until it was all cleaned up. If there's more support for this now, I'm happy to pitch in.

[William Chan (陈智昌)]

My plan of attack is to write a Clang plugin and bulk fix everything.

[Wez]

If you send your Clang plugin my way then I'll take care of fixing remoting/.

[David Michael]

On Wed, Oct 24, 2012 at 3:52 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

My plan of attack is to write a Clang plugin and bulk fix everything.

I hope that works. I was suspecting that we'd need human eyeballs on a lot of it. When a member function returns a scoped_refptr member variable as a raw pointer right now, should we just get() it? Or should we change the return type to scoped_refptr<T>? Or "const scoped_refptr<T>&"? If we change the return type, we might also have to change types at the call-site, and maybe their call-site, etc. Same goes for params.

(E.g., http://codereview.chromium.org/11189146/diff/1/ipc/ipc_sync_channel.h, and a few other random examples in that CL where I started poking at it).

If you can collapse the different cases in to a set you can automate, then that would be great. I suspect this is a case where rolling up our sleeves and doing it manually would be more efficient in the end. But I would love to be proven wrong on this.

[William Chan (陈智昌)]

I am only going to bulk fix so we can ditch the implicit conversion operator. I do not believe this will change any function signatures, but may change some invocations to insert the .get(), which hopefully the clang rewriter would take care of. I'm going to consider your example of changing the return type as out of scope. Does that seem reasonable?

[David Michael]

On Fri, Oct 26, 2012 at 12:53 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

I am only going to bulk fix so we can ditch the implicit conversion operator. I do not believe this will change any function signatures, but may change some invocations to insert the .get(), which hopefully the clang rewriter would take care of. I'm going to consider your example of changing the return type as out of scope. Does that seem reasonable?

Yes, that would work and would put us in better shape than we are today.

The only advantage I see to doing it more by hand (or trying to write a smarter Clang plugin) is there may be latent ref-counting bugs out there due to mixing raw pointers with scoped_refptrs incorrectly. The compiler can help us find these hot spots where people might not have thought about the conversion to a raw pointer (since they didn't make it explicit). But it's not clear to me that we would get enough value, given that it is a lot of effort.

[William Chan (陈智昌)]

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something. I don't think we could automate either of these refactors.

Or we can just give up and stick with a PSA, or live with the ugliness of these extra .get() calls.

Any thoughts here? I don't feel like any solution is particularly nice. My personal pref would be a const base::RefPtr<T>&, but I know it's contentious since many people may incorrectly parse it as RefPtr<const T> instead. I fear there won't be any rough consensus here, but let's see :)

[Albert J. Wong (王重傑)]

My bias:

  const-ref scoped_refptr   >   ugly get()   >>  keeping implicit conversions.

Implicit conversions are confusing and have unfun interactions with the type system.

I'd rather just allow "const scoped_refptr<T>&".  However, if we're going to bikeshed it to death, I vote for the uglier .get() translation.  Both are less confusing than unexpected implicit conversion chains.

-Albert

[Antoine Labour]

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Antoine

[William Chan (陈智昌)]

Btw, for reference here's the old discussion on const scoped_refptr<T>&: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/noOt34XStEU/i5qB8OaB504J.

Oh, and I snuck in a rename to base::RefPtr which is entirely optional. Darin was telling me he originally wanted to name it RefPtr instead of scoped_refptr, except it conflicted with WTF::RefPtr. One thing he dislikes about const scoped_refptr<T>& is the verbosity, so const base::RefPtr<T>& was an attempt to mitigate that (I guess it doesn't help much for header files where you can't do a using base::RefPtr). Meh.

[Jeffrey Yasskin]

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:
> So I looked into it. I've uploaded a partial changelist (my clang rewriter
> crapped out due to some overlapping replacements when we have nested
> expressions using the implicit operator T*):
> http://codereview.chromium.org/download/issue11275088_1.diff
>
> One thing I don't like is the stuff like MessageLoopProxy::current().get()
> or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/
> Ignore the if (foo.get()) expressions. I'll fix those with the safe operator
> bool idiom later.
>
> I think to fix this completely, we'd need to either allow |const
> scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to
> revisit) or introduce pass_scoped_refptr<T> or something. I don't think we
> could automate either of these refactors.
> Or we can just give up and stick with a PSA, or live with the ugliness of
> these extra .get() calls.

I looked at the first one of the MessageLoopProxy::current().get(),
and it's passing the result to ImportantFileWriter, which takes a
SequencedTaskRunner*. In that case, I think it would be a bad idea to
change the argument to const scoped_refptr<SequencedTaskRunner>&,
since it would enforce a particular ownership structure on all
callers. .get() is the right fix in that case.

In general, I think C++ programmers ought to be able to distinguish
between a pointer and the object it points to, and realize that either
can be constant independently. Distinguishing "const
scoped_refptr<T>&" from "scoped_refptr<const T>&" shouldn't be
particularly hard, especially compared to "T*const" vs "const T*".

[William Chan (陈智昌)]

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

[William Chan (陈智昌)]

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

[Dana Jansens]

On Wed, Oct 31, 2012 at 8:03 PM, William Chan (陈智昌)

<will...@chromium.org> wrote:
> On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org>
> wrote:
>>
>> On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:
>>>
>>>
>>>
>>> On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌)
>>> <will...@chromium.org> wrote:
>>>>
>>>> So I looked into it. I've uploaded a partial changelist (my clang
>>>> rewriter crapped out due to some overlapping replacements when we have
>>>> nested expressions using the implicit operator T*):
>>>> http://codereview.chromium.org/download/issue11275088_1.diff
>>>>
>>>> One thing I don't like is the stuff like
>>>> MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get().
>>>> Pretty gross if you ask me =/
>>>> Ignore the if (foo.get()) expressions. I'll fix those with the safe
>>>> operator bool idiom later.
>>>>
>>>> I think to fix this completely, we'd need to either allow |const
>>>> scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to
>>>> revisit) or introduce pass_scoped_refptr<T> or something.

Isn't relying on const for type safety much less reliable than using a
pass_scoped_refptr class?

[Antoine Labour]

On Wed, Oct 31, 2012 at 5:03 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

For non-thread-safe refcounting cases, I thought it was commonly agreed that it'd be in the noise perf-wise (most likely hidden under the previous/next memory access on modern OoO CPUs).

For thread-safe, I could see it being a problem in inner loops - is that common though?

Antoine

[William Chan (陈智昌)]

On Wed, Oct 31, 2012 at 5:00 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌)
<will...@chromium.org> wrote:
> So I looked into it. I've uploaded a partial changelist (my clang rewriter
> crapped out due to some overlapping replacements when we have nested
> expressions using the implicit operator T*):
> http://codereview.chromium.org/download/issue11275088_1.diff
>
> One thing I don't like is the stuff like MessageLoopProxy::current().get()
> or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/
> Ignore the if (foo.get()) expressions. I'll fix those with the safe operator
> bool idiom later.
>
> I think to fix this completely, we'd need to either allow |const
> scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to
> revisit) or introduce pass_scoped_refptr<T> or something. I don't think we
> could automate either of these refactors.
> Or we can just give up and stick with a PSA, or live with the ugliness of
> these extra .get() calls.

I looked at the first one of the MessageLoopProxy::current().get(),
and it's passing the result to ImportantFileWriter, which takes a
SequencedTaskRunner*. In that case, I think it would be a bad idea to
change the argument to const scoped_refptr<SequencedTaskRunner>&,
since it would enforce a particular ownership structure on all
callers. .get() is the right fix in that case.

I walked over to talk to jyasskin and discovered that he was under the impression that TaskRunners are not always refcounted. They are. So ownership structure is not being changed. Ignore his above comment.

[William Chan (陈智昌)]

On Wed, Oct 31, 2012 at 5:07 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 5:03 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

For non-thread-safe refcounting cases, I thought it was commonly agreed that it'd be in the noise perf-wise (most likely hidden under the previous/next memory access on modern OoO CPUs).

For thread-safe, I could see it being a problem in inner loops - is that common though?

I agree that performance isn't a large concern in general. Indeed, I am the person who argued we should just make RefCountedThreadSafe the default after all. Do people prefer passing around copies of scoped_refptr instead? That's definitely another option. No one proposed it in the old discussion on const scoped_refptr<T>&, so I was assuming people didn't really consider it. Would you prefer this over const scoped_refptr<T>&?

[Albert J. Wong (王重傑)]

On Wed, Oct 31, 2012 at 5:17 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:07 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 5:03 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

For non-thread-safe refcounting cases, I thought it was commonly agreed that it'd be in the noise perf-wise (most likely hidden under the previous/next memory access on modern OoO CPUs).

For thread-safe, I could see it being a problem in inner loops - is that common though?

I agree that performance isn't a large concern in general. Indeed, I am the person who argued we should just make RefCountedThreadSafe the default after all. Do people prefer passing around copies of scoped_refptr instead? That's definitely another option. No one proposed it in the old discussion on const scoped_refptr<T>&, so I was assuming people didn't really consider it. Would you prefer this over const scoped_refptr<T>&?

I personally prefer const scoped_refptr<T>&.  The extra doesn't matter, but const OCDness here doesn't make much semantic sense to me.  The scoped_refptr is really just a carrier.  Do we believe there is a true readability downside here?  I'm skeptical.

-Albert

[William Chan (陈智昌)]

I'm not sure I understand the part about relying on const for type safety. Can you explain?

As for the pass_scoped_refptr<T> class, Darin points out that PassOwnPtr in WebKit is a bit dangerous:

"""

PassOwnPtr is a bit dangerous... once assigned to an OwnPtr, then PassOwnPtr becomes null.

(or once assigned to another PassOwnPtr)

"""

This is one reason we didn't use it when creating scoped_ptr<T>::Pass().

[Antoine Labour]

On Wed, Oct 31, 2012 at 5:17 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:07 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 5:03 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

For non-thread-safe refcounting cases, I thought it was commonly agreed that it'd be in the noise perf-wise (most likely hidden under the previous/next memory access on modern OoO CPUs).

For thread-safe, I could see it being a problem in inner loops - is that common though?

I agree that performance isn't a large concern in general. Indeed, I am the person who argued we should just make RefCountedThreadSafe the default after all. Do people prefer passing around copies of scoped_refptr instead? That's definitely another option. No one proposed it in the old discussion on const scoped_refptr<T>&, so I was assuming people didn't really consider it. Would you prefer this over const scoped_refptr<T>&?

It's a bit less of a pain. I was thinking it would save on code size and even possibly perf (like is the case for small structs) but thinking about it more, I'm not so convinced anymore because scoped_refptr has side effects on construction/destruction that can't be elided. Ignore me.

Antoine

[Antoine Labour]

On Wed, Oct 31, 2012 at 5:23 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:17 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:07 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 5:03 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 5:02 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

On Wed, Oct 31, 2012 at 4:54 PM, Antoine Labour <pi...@google.com> wrote:

On Wed, Oct 31, 2012 at 4:32 PM, William Chan (陈智昌) <will...@chromium.org> wrote:

So I looked into it. I've uploaded a partial changelist (my clang rewriter crapped out due to some overlapping replacements when we have nested expressions using the implicit operator T*): http://codereview.chromium.org/download/issue11275088_1.diff

One thing I don't like is the stuff like MessageLoopProxy::current().get() or Profile::GetForFactory(foo).get(). Pretty gross if you ask me =/

Ignore the if (foo.get()) expressions. I'll fix those with the safe operator bool idiom later.

I think to fix this completely, we'd need to either allow |const scoped_refptr<T>&| (I know we disallowed this earlier, but maybe we need to revisit) or introduce pass_scoped_refptr<T> or something.

Why? Is it that we don't want to pay for an extra AddRef/Release?

Avoids the .get(). Most function parameters are of the form Foo*. If we could change them to const scoped_refptr<Foo>& instead, that'd avoid the caller needing to invoke .get() to pass a Foo*.

Oops, I misunderstood. You were assuming we'd change the function parameters to be scoped_refptr<T> and do copies. Yes, that'd be an extra AddRef()/Release() pair :(

For non-thread-safe refcounting cases, I thought it was commonly agreed that it'd be in the noise perf-wise (most likely hidden under the previous/next memory access on modern OoO CPUs).

For thread-safe, I could see it being a problem in inner loops - is that common though?

I agree that performance isn't a large concern in general. Indeed, I am the person who argued we should just make RefCountedThreadSafe the default after all. Do people prefer passing around copies of scoped_refptr instead? That's definitely another option. No one proposed it in the old discussion on const scoped_refptr<T>&, so I was assuming people didn't really consider it. Would you prefer this over const scoped_refptr<T>&?

I personally prefer const scoped_refptr<T>&.  The extra doesn't matter, but const OCDness here doesn't make much semantic sense to me.  The scoped_refptr is really just a carrier.  Do we believe there is a true readability downside here?  I'm skeptical.

I guess readability-wise, you take a reference to a pointer (i.e. double-indirection). To pass a simple object it seems overkill and distracting.

Antoine

[Jeffrey Yasskin]

Yeah, I'd agree that if the call-ee is never going to keep a copy of
the scoped_refptr, they should take a T* or const T& instead. If they
are going to keep a copy, then "const scoped_refptr<T>&" or
"scoped_refptr<T>" (or "const scoped_refptr<const T>&" if the object
can be const) is clearer. The first is generally better in C++98, but
the second will often be better in C++11 since it can avoid refcount
operations in some cases. (As you pointed out, avoiding refcount
operations only matters for thread-safe refcounting.)

Jeffrey

[Brett Wilson]

Every time scoped_ptr and scoped_refptr are different it bites me. The
implicit casts vs get(), the way that one uses operator= and one uses
reset drives me batty and I'm always doing the wrong one.

So +1 to get() so at least I only have to remember one thing, even if it's ugly.

Also +1 to "base::RefPtr".

Brett

[Darin Fisher]

I agree with you.  This is what made me a fan of the proposed .get() change.  It is just slightly cumbersome, but perhaps that's still the best way to go.

-Darin

[David Michael]

On Wed, Oct 31, 2012 at 11:38 PM, Darin Fisher <da...@chromium.org> wrote:

I agree with you.  This is what made me a fan of the proposed .get() change.  It is just slightly cumbersome, but perhaps that's still the best way to go.

I want to make sure I understand what you mean by the .get()  change, and what you prefer it over. Are you simply saying you prefer using get() over having implicit conversion?

As for parameters and return types, I don't see a problem allowing both:

a) scoped_refptr<T>

b) const scoped_refptr<T>&

Use (a) if you know it's not at all performance critical and you want to prefer readability. Use (b) when you might care about performance. I would usually prefer (b)...  I don't think the impact on readability is really that high, since most experienced C++ programmers understand the "const-reference" idiom at such a deep level that it doesn't even feel like indirection anymore.

There may be special cases where passing the raw pointer around is the best thing to do. But I think we should discourage that. The basic rules of thumb for smart pointers should be:

1) Wrap the object in a smart pointer as soon as it's created (shared_ptr<T> t_ptr = shared_ptr<T>(new T)).

2) Never convert it to a raw pointer (unless you have to, e.g. for a third-party API that's not aware of ref-counting).

As soon as you go to a raw pointer, you're opening the door for mistakes (see, e.g., the original subject of this thread).

If it looks like it's too much work to manually convert the code, then a Clang plugin to just convert us to using get()s would at least let us get rid of the manual conversion, so may still be worth it. But your CL makes it look difficult, but manageable.

[Darin Fisher]

On Thu, Nov 1, 2012 at 10:53 AM, David Michael <dmic...@google.com> wrote:

On Wed, Oct 31, 2012 at 11:38 PM, Darin Fisher <da...@chromium.org> wrote:

I agree with you.  This is what made me a fan of the proposed .get() change.  It is just slightly cumbersome, but perhaps that's still the best way to go.

I want to make sure I understand what you mean by the .get()  change, and what you prefer it over. Are you simply saying you prefer using get() over having implicit conversion?

Yes

 

As for parameters and return types, I don't see a problem allowing both:

a) scoped_refptr<T>

b) const scoped_refptr<T>&

Use (a) if you know it's not at all performance critical and you want to prefer readability. Use (b) when you might care about performance. I would usually prefer (b)...  I don't think the impact on readability is really that high, since most experienced C++ programmers understand the "const-reference" idiom at such a deep level that it doesn't even feel like indirection anymore.

I prefer (b).  I always have a gut reaction against passing objects by value, and I prefer to trust my gut.  I prefer not having to think about whether it is OK to pass an object by value.  Just always pass objects by const ref, and you should be OK unless you are doing something fancy :-)

There may be special cases where passing the raw pointer around is the best thing to do. But I think we should discourage that. The basic rules of thumb for smart pointers should be:

1) Wrap the object in a smart pointer as soon as it's created (shared_ptr<T> t_ptr = shared_ptr<T>(new T)).

2) Never convert it to a raw pointer (unless you have to, e.g. for a third-party API that's not aware of ref-counting). 

I don't have quite so strong of an opinion on this.  Have we been burned a lot by using raw pointers to reference counted types?

Even in WebCore, where they have PassRefPtr<T>, they still commonly pass raw pointers to reference counted types into functions.

-Darin

[David Michael]

On Thu, Nov 1, 2012 at 12:05 PM, Darin Fisher <da...@chromium.org> wrote:

On Thu, Nov 1, 2012 at 10:53 AM, David Michael <dmic...@google.com> wrote:

On Wed, Oct 31, 2012 at 11:38 PM, Darin Fisher <da...@chromium.org> wrote:

I agree with you.  This is what made me a fan of the proposed .get() change.  It is just slightly cumbersome, but perhaps that's still the best way to go.

I want to make sure I understand what you mean by the .get()  change, and what you prefer it over. Are you simply saying you prefer using get() over having implicit conversion?

Yes

 

As for parameters and return types, I don't see a problem allowing both:

a) scoped_refptr<T>

b) const scoped_refptr<T>&

Use (a) if you know it's not at all performance critical and you want to prefer readability. Use (b) when you might care about performance. I would usually prefer (b)...  I don't think the impact on readability is really that high, since most experienced C++ programmers understand the "const-reference" idiom at such a deep level that it doesn't even feel like indirection anymore.

I prefer (b).  I always have a gut reaction against passing objects by value, and I prefer to trust my gut.  I prefer not having to think about whether it is OK to pass an object by value.  Just always pass objects by const ref, and you should be OK unless you are doing something fancy :-)

There may be special cases where passing the raw pointer around is the best thing to do. But I think we should discourage that. The basic rules of thumb for smart pointers should be:

1) Wrap the object in a smart pointer as soon as it's created (shared_ptr<T> t_ptr = shared_ptr<T>(new T)).

2) Never convert it to a raw pointer (unless you have to, e.g. for a third-party API that's not aware of ref-counting). 

I don't have quite so strong of an opinion on this.  Have we been burned a lot by using raw pointers to reference counted types?

Even in WebCore, where they have PassRefPtr<T>, they still commonly pass raw pointers to reference counted types into functions.

A lot of my prejudice against mixing raw with smart pointers comes from being raised on non-intrusive smart pointers (like shared_ptr or linked_ptr). In those, if you go smart->raw->smart, you now have two groups of smart pointers that think they own the raw pointer, and double deletion is guaranteed.

It's admittedly a lot less dangerous for intrusive ref-counting like ours. But there are still ways to hang yourself (like the CreateRefCountedT() anti-pattern that started the thread). I got interested in this because I barely caught a memory leak in code review due to the conversion to a raw pointer.

I won't be dogmatic about it if my suggested rule of thumb seems too dogmatic. I just like rules that are easy to remember and keep you from making mistakes. In this case, the mistakes it prevents are probably rare. Maybe being forced to call get() is enough to make devs stop and think, and that might be good enough.

[Albert J. Wong (王重傑)]

So to summarize:

  (1) Implicit conversion must die.

  (2) Requiring a a .get() sometimes is completely fine.

  (3) const scoped_refptr<T>& as argument/return

         - Pros: Doesn't look like a copy. In C++98, may avoid extra ref/deref.

         - Cons: Less readable due to clutter.

For (3), I feel like the tide is going for "allow."  In terms of arguments, since we're C++98, and since const& is such a common idiom, I don't think the readability cost is that large.

However, I might be biased. Do any detractors feel strongly enough to keep pushing against const scoped_refptr<T>&?  Or can we settle on this and UNLEASH THE WILLCHAN (patch/plugin/whatever).

-Albert

[Robert Iannucci]

Drive-by noogler here, but having worked on a somewhat large cross platform C++ project before, I just wanted to second that the "const foo&" is a completely readable idiom to me (and I would argue that in general it's a bug to pass by value when pass by const-ref will do).

I'm assuming that a const-ref scoped smart pointer would not let you increment/decrement the refcount (implicitly OR explicitly, i.e. no access to the underlying smart pointer), but allows non-const access to T? That would be the behavior I would expect anyway...

Robbie

[James Robinson]

On Thu, Nov 1, 2012 at 11:05 AM, Darin Fisher <da...@chromium.org> wrote:

There may be special cases where passing the raw pointer around is the best thing to do. But I think we should discourage that. The basic rules of thumb for smart pointers should be:

1) Wrap the object in a smart pointer as soon as it's created (shared_ptr<T> t_ptr = shared_ptr<T>(new T)).

2) Never convert it to a raw pointer (unless you have to, e.g. for a third-party API that's not aware of ref-counting). 

I don't have quite so strong of an opinion on this.  Have we been burned a lot by using raw pointers to reference counted types?

Even in WebCore, where they have PassRefPtr<T>, they still commonly pass raw pointers to reference counted types into functions.

It's somewhat inconsistent, but a convention that I like and that some code sticks to is that you pass a PassRefPtr<T> when you want the force callee to deal with the ref-counted nature of the pointer and use a raw pointer when the callee doesn't necessarily care.  I.e. if you are going to take an owning reference of the parameter, take a PassRefPtr<T>.  If you aren't retaining the pointer beyond the callstack, take a raw pointer.

- James

[Darin Fisher]

On Thu, Nov 1, 2012 at 12:26 PM, James Robinson <jam...@google.com> wrote:

On Thu, Nov 1, 2012 at 11:05 AM, Darin Fisher <da...@chromium.org> wrote:

There may be special cases where passing the raw pointer around is the best thing to do. But I think we should discourage that. The basic rules of thumb for smart pointers should be:

1) Wrap the object in a smart pointer as soon as it's created (shared_ptr<T> t_ptr = shared_ptr<T>(new T)).

2) Never convert it to a raw pointer (unless you have to, e.g. for a third-party API that's not aware of ref-counting). 

I don't have quite so strong of an opinion on this.  Have we been burned a lot by using raw pointers to reference counted types?

Even in WebCore, where they have PassRefPtr<T>, they still commonly pass raw pointers to reference counted types into functions.

It's somewhat inconsistent, but a convention that I like and that some code sticks to is that you pass a PassRefPtr<T> when you want the force callee to deal with the ref-counted nature of the pointer and use a raw pointer when the callee doesn't necessarily care.  I.e. if you are going to take an owning reference of the parameter, take a PassRefPtr<T>.  If you aren't retaining the pointer beyond the callstack, take a raw pointer.

That seems like a pretty good convention.  I think that matches up with my mental model too.  Functions that don't need to take ownership shouldn't care that something is reference counted, etc.

-Darin

[William Chan (陈智昌)]

I'm going to proceed with .get() when I have time to futz around with Clang again. Probably in two weeks since I'll be out at IETF 85 next week and won't be able to do this today.

It's most likely that I'll have to do this in a few phases:

* Add .get() everywhere via Clang rewriter. Since it looks to be on the order of 1000 files or something (need to doublecheck), I'll probably just TBR via CQ by directory.

* Remove the implicit operator T*.

* Add a safe version of the bool operator for the if (scoped_refptr) case.

* Use another Clang rewriter to detect the conditional case and remove the .get(), again TBR via CQ by directory.

* Done

I'll leave it up to individuals and their reviewers as to whether or not they want to change APIs to accept const scoped_refptr<T>&.

[Darin Fisher]

SGTM

Thanks Will !!

[Jonathan Dixon]

Is it OK is we start writing new code under the assumption this change will happen? e.g. by default we now pass const scopde_refptr<>& as inputs, and returning scopde_refptr<> from factory methods?

...The latter is far more interesting to me now than a year ago, as scoped_ptr<>::Pass means I now treat new factory methods that return a raw pointer with suspicion  and so having the return type always be either scoped_ptr or scoped_refptr would be great