Intent to implement: sanitize scoped_refptr usage.

[Vladimir Levin]

Hi, 

Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.

This is usually confusing, and if we ever decide to switch to std::shared_ptr, it will also lead to memory bugs. 

I want to start looking at sanitizing our scoped_refptr usage to try and eliminate these cases. 

Specifically,

1. Introduce MakeScopedRefptr

I'd like to change scoped_refptr<T> make_scoped_refptr(T* object) to be closer to std::make_shared/MakeUnique in that it would take arguments to the object and create the scoped_refptr. Something along the lines of the following:

template <typename T, typename... Args>

scoped_refptr<T> MakeScopedRefptr(Args&&... args) {

 ...

}

Hopefully, we can work towards eliminating usage of make_scoped_refptr as a result. 

2. Explicit scoped_refptr ctor

I'd like to make scoped_refptr ctor explicit (crbug.com/589048), which should make it clear where the object is owned and reduce (and hopefully eliminate) ref ptr -> raw ptr -> ref ptr churn. There are some difficult cases here where the ownership is actually unclear, which means this might result in non trivial patches... However, I think the effort would be worth it.

As a side note, we also have a pattern where we create a scoped_refptr from |this|. In standard C++, we'd use enable_shared_from_this/shared_from_this for it, but I'm a bit unsure how to align our code incrementally with this pattern. One approach would be something like make_scoped_refptr(this) (although this contradicts 1. above). Alternatively, we could extend RefCounted/RefCountedThreadSafe to provide scoped_refptr_from_this().

Please let me know if you have any thoughts or concerns about this plan.

Thanks,

Vlad

[Tommy Nyquist]

This is something that has been weird to me and that I've had to explain to new people on the project several times, so I'd love for Chromium to be more similar to std::shared_ptr.

--
--
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]

On Tue, Aug 23, 2016 at 12:46 PM, Vladimir Levin <vmp...@chromium.org> wrote:

Hi, 

Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#Object-ownership-and-calling-conventions

 

This is usually confusing, and if we ever decide to switch to std::shared_ptr, it will also lead to memory bugs. 

Is there motivation besides switching to std::shared_ptr? It seems like this is really a discussion related to that goal, and that one is a much more complex and controversial topic of discussion (the performance tradeoffs between intrusive counting vs external counting in particular)

[David Benjamin]

On Tue, Aug 23, 2016 at 6:09 PM Ryan Sleevi <rsl...@chromium.org> wrote:

On Tue, Aug 23, 2016 at 12:46 PM, Vladimir Levin <vmp...@chromium.org> wrote:

Hi, 

Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#Object-ownership-and-calling-conventions

If I recall, those guidelines were recently revised and, in that same thread, we decided to make the scoped_refptr ctor explicit so that it aligns with those guidelines better.

David

 

 

This is usually confusing, and if we ever decide to switch to std::shared_ptr, it will also lead to memory bugs. 

Is there motivation besides switching to std::shared_ptr? It seems like this is really a discussion related to that goal, and that one is a much more complex and controversial topic of discussion (the performance tradeoffs between intrusive counting vs external counting in particular)

--

[Taiju Tsuiki]

+1 to MakeScopedPtr and the explicit conversion.

FYI: base::Bind is another source of an effective implicit conversion from raw pointers to scoped_refptr<>.

I built a clang tool to replace it with a caller-side base::RetainedRef(), and generated a CL with it.

2016年8月24日(水) 10:25 David Benjamin <davi...@chromium.org>:

[Elliott Sprehn]

On Aug 23, 2016 3:46 PM, "Vladimir Levin" <vmp...@chromium.org> wrote:
>
> Hi, 
>
> Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.
>
> This is usually confusing, and if we ever decide to switch to std::shared_ptr,

Fwiw I don't think we should ever do that. It's thread safe which the vast majority of things don't need, it has double free foot guns, it doesn't let choose the storage, and it doesn't let us delegate the ref counting to another object.

It's not a good part of the standard library, just like auto_ptr.

> it will also lead to memory bugs. 
>
> I want to start looking at sanitizing our scoped_refptr usage to try and eliminate these cases. 
>
> Specifically,
> 1. Introduce MakeScopedRefptr
> I'd like to change scoped_refptr<T> make_scoped_refptr(T* object) to be closer to std::make_shared/MakeUnique in that it would take arguments to the object and create the scoped_refptr. Something along the lines of the following:
>
> template <typename T, typename... Args>
> scoped_refptr<T> MakeScopedRefptr(Args&&... args) {
>  ...
> }
>
> Hopefully, we can work towards eliminating usage of make_scoped_refptr as a result. 
>
> 2. Explicit scoped_refptr ctor
> I'd like to make scoped_refptr ctor explicit (crbug.com/589048), which should make it clear where the object is owned and reduce (and hopefully eliminate) ref ptr -> raw ptr -> ref ptr churn. There are some difficult cases here where the ownership is actually unclear, which means this might result in non trivial patches... However, I think the effort would be worth it.
>
> As a side note, we also have a pattern where we create a scoped_refptr from |this|. In standard C++, we'd use enable_shared_from_this/shared_from_this for it, but I'm a bit unsure how to align our code incrementally with this pattern. One approach would be something like make_scoped_refptr(this) (although this contradicts 1. above). Alternatively, we could extend RefCounted/RefCountedThreadSafe to provide scoped_refptr_from_this().

This pattern doesn't really make sense with the intrusive ref counting. There's no danger of double free.

>
> Please let me know if you have any thoughts or concerns about this plan.
>
> Thanks,
> Vlad
>

[Vladimir Levin]

On Wed, Aug 24, 2016 at 9:11 AM, Elliott Sprehn <esp...@chromium.org> wrote:

On Aug 23, 2016 3:46 PM, "Vladimir Levin" <vmp...@chromium.org> wrote:
>
> Hi, 
>
> Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.
>
> This is usually confusing, and if we ever decide to switch to std::shared_ptr,

Fwiw I don't think we should ever do that. It's thread safe which the vast majority of things don't need, it has double free foot guns, it doesn't let choose the storage, and it doesn't let us delegate the ref counting to another object.

It's not a good part of the standard library, just like auto_ptr.

It was perhaps my mistake to mention shared_ptr here :) My end goal here is not to switch to shared_ptr without proper (separate) discussion, measurements, arguments, etc. My goal is to ensure that the ownership of all objects is clear throughout the code. Specifically (and I think this has been mentioned several times on other threads),

- An object that is fully owned by a class should either be a non-pointer, or a unique ptr.

- An object that is partially owned by a class should be a scoped_refptr.

- An object that is not owned by a class should be a raw pointer.

It seems to me that the corollary of this is that an object that is passed as a raw pointer should not be owned (partially or wholly) by the class or by any other class further down the stack. 

Right now it's easy to break this. We can start with a scoped_refptr and pass a raw pointer out of it to some class. That raw pointer can then be passed through several classes and eventually put back into a scoped_refptr. To me, that indicates unclear ownership. When looking at the middle classes, it seems that the object given is not to be owned further down the stack, but it is. 

The proposal here is make it very explicit that a raw pointer is being converted to a scoped_refptr, so that it is easy to spot in reviews. First, using MakeScopedRefptr, creating a scoped_refptr would not involve a "new" for the most part making it impossible to pass a raw pointer here. Second, explicit scoped_refptr ctor would ensure that it's clear where a raw pointer is becoming a scoped_refptr.

It is of course still possible to do scoped_refptr -> raw ptr -> scoped_refptr things here. However, it is at least explicit when we convert raw ptr back to scoped_refptr. Hopefully the authors/reviewers will exercise due diligence and question these cases.

[dan...@chromium.org]

On Wed, Aug 24, 2016 at 11:31 AM, Vladimir Levin <vmp...@chromium.org> wrote:

On Wed, Aug 24, 2016 at 9:11 AM, Elliott Sprehn <esp...@chromium.org> wrote:

On Aug 23, 2016 3:46 PM, "Vladimir Levin" <vmp...@chromium.org> wrote:
>
> Hi, 
>
> Since Chromium uses intrusive ref counting (via RefCounted/RefCountedThreadSafe base classes), there are a lot of cases where object ownership is unclear. That is, one can create a scoped_refptr, then pass a raw pointer around until some other object puts that raw pointer into a scoped_refptr.
>
> This is usually confusing, and if we ever decide to switch to std::shared_ptr,

Fwiw I don't think we should ever do that. It's thread safe which the vast majority of things don't need, it has double free foot guns, it doesn't let choose the storage, and it doesn't let us delegate the ref counting to another object.

It's not a good part of the standard library, just like auto_ptr.

It was perhaps my mistake to mention shared_ptr here :) My end goal here is not to switch to shared_ptr without proper (separate) discussion, measurements, arguments, etc. My goal is to ensure that the ownership of all objects is clear throughout the code. Specifically (and I think this has been mentioned several times on other threads),

- An object that is fully owned by a class should either be a non-pointer, or a unique ptr.

- An object that is partially owned by a class should be a scoped_refptr.

- An object that is not owned by a class should be a raw pointer.

It seems to me that the corollary of this is that an object that is passed as a raw pointer should not be owned (partially or wholly) by the class or by any other class further down the stack. 

Right now it's easy to break this. We can start with a scoped_refptr and pass a raw pointer out of it to some class. That raw pointer can then be passed through several classes and eventually put back into a scoped_refptr. To me, that indicates unclear ownership. When looking at the middle classes, it seems that the object given is not to be owned further down the stack, but it is. 

The proposal here is make it very explicit that a raw pointer is being converted to a scoped_refptr, so that it is easy to spot in reviews. First, using MakeScopedRefptr, creating a scoped_refptr would not involve a "new" for the most part making it impossible to pass a raw pointer here. Second, explicit scoped_refptr ctor would ensure that it's clear where a raw pointer is becoming a scoped_refptr.

It is of course still possible to do scoped_refptr -> raw ptr -> scoped_refptr things here. However, it is at least explicit when we convert raw ptr back to scoped_refptr. Hopefully the authors/reviewers will exercise due diligence and question these cases.

I am a fan of this proposal, and would love to see ownership of refptrs not be passed around via raw pointers sometimes.