returning a scoped_ptr<> ?

[Alec Flett]

I'm curious what the current thinking is on functions that return refcounted objects.. which style is preferred, if there is any preference at all:

Raw pointers:

X* getX();

vs scoped_ptr return value:

scoped_refptr<X> getX();

?

returning a scoped_refptr seems safer but I have to imagine someone has done the research to figure out if it is too easy to have refcount churn, etc..

[Fred Akalin]

It's unsafe until we get rid of the implicit raw pointer <-> scoped_refptr conversion.

If you did:

X* x = getX();

and getX() returns a scoped_refptr<X>, it'll be silently converted to a raw pointer, which may be invalid by the time you use it (because something else released the last ref).

So unfortunately returning raw pointers is the safer bet, since memory leaks are less bad than use-after-frees. :/

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

[Peter Kasting]

On Wed, Nov 13, 2013 at 3:53 PM, Fred Akalin <aka...@chromium.org> wrote:

It's unsafe until we get rid of the implicit raw pointer <-> scoped_refptr conversion.

Are there plans to actually do this?

On Wed, Nov 13, 2013 at 3:52 PM, Alec Flett <alec...@chromium.org> wrote:

I'm curious what the current thinking is on functions that return refcounted objects..

The lesson here seems to be: try hard to minimize use of refcounting in the first place.

PK

[Fred Akalin]

On Wed, Nov 13, 2013 at 3:55 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Wed, Nov 13, 2013 at 3:53 PM, Fred Akalin <aka...@chromium.org> wrote:

It's unsafe until we get rid of the implicit raw pointer <-> scoped_refptr conversion.

Are there plans to actually do this?

Yes, see crbug.com/110610 . It's tedious, though, and tough to automate (because Windows).

[Dana Jansens]

Unfortuantely we keep regressing the work that had been done over time. It'd be great to have some presubmit checks that we could enable to prevent it in subdirs of the codebase that are/were "clean" to keep them that way. And to make things safer!

[William Chan (陈智昌)]

I agree that whitelisting clean directories would be nice. I think if we could finish this work up to at least all the non-Windows-specific code, then the whitelist would cover most of the code base and we could hopefully manually finish the rest.

--

[Daniel Cheng]

An alternative to whitelisting is to enforce this with the Clang plugin once all the non-Windows-specific code is fixed.

Daniel

[Colin Blundell]

Is there any reason to return raw pointers instead of scoped_refptrs for functions that are not doing a transfer of ownership (e.g., AutofillWebDataService::FromBrowserContext())?

Arguments for returning scoped_refptr instead of a raw pointer in the case where the function is not doing a transfer of ownership:

- It's no less safe than returning a raw pointer (since the callee is holding onto a reference, the increment/decrement pair caused by the caller dropping their reference immediately on the ground won't cause the object to be deleted).

- The API is more clear when the function returns a scoped_refptr.

- In the long term, we will want these APIs returning scoped_refptrs, so doing otherwise now would just result in code churn (or worse, no one coming back around and changing it later).

Is there anything I'm missing? The reason I raise this question is I've received some comments on CLs recently asking whether I should be changing the above-mentioned function to return a raw pointer instead, referencing this thread.

Thanks,

Colin

[Tom Hudson]

On Thu, Nov 21, 2013 at 4:50 PM, Colin Blundell <blun...@chromium.org> wrote:

Is there any reason to return raw pointers instead of scoped_refptrs for functions that are not doing a transfer of ownership (e.g., AutofillWebDataService::FromBrowserContext())?

http://www.chromium.org/developers/smart-pointer-guidelines still advises AGAINST using refptrs for code health reasons, and using them is surprisingly non-performant on Android devices. We had to rip refptrs out of some compositor data structures when it turned out that maintaining the reference counts was showing up as a significant fraction of CPU time.

From the link:

• use scoped_refptr; but better yet, rethink your design. Reference-counted objects make it difficult to understand ownership and destruction order, especially when multiple threads are involved. There is almost always another way to design your object hierarchy to avoid refcounting.

Tom

[Jói Sigurðsson]

The problem with returning a bare pointer is that the caller might not
realize it's a refcounted object, and store it without thinking about
the refcount.

The problem with returning scoepd_refptr seems to be, per Tom, that
this sometimes leads to unacceptable performance on Android.

Perhaps we should have a new wrapper class that documents that
something you're returning is a pointer to a refcounted object, but
does not actually do anything to the reference count? This would be a
class with an operator-> overload, but without a .get() or an
automatic cast operator. So if you just want to call a method on the
reference counted object, you can do that, but if you want to store
it, you will have to pass it to a scoped_refptr (which would be a
friend of the new wrapper and thus could extract the raw pointer).

Cheers,
Jói

[Samuel Huang]

Is the slowness of scoped_refptr caused by thread safety?  If not, is there supply / demand for non-thread-safe scoped_refptr?

--

Samuel Huang

[Ryan Sleevi]

There already is base::RefCounted (non-thread-safe) and
base::RefCountedThreadSafe (as the name implies, thread-safe)

scoped_refptr<> works with either.

[Scott Hess]

On Thu, Nov 21, 2013 at 9:59 AM, Jói Sigurðsson <j...@chromium.org> wrote:

The problem with returning a bare pointer is that the caller might not
realize it's a refcounted object, and store it without thinking about
the refcount.

The caller shouldn't be storing the pointer unless the API specifically documents that that is safe and the caller fully groks the ownership rules.  Caching pointers without understanding the object lifetime is a rich and unending source of crashes and other bugs.

[I assume you mean storing as a member variable or something.]

-scott

[Adam Treat]

Already exists.  base/memory/ref_counted.h has a thread-safe implementation and a non-thread safe one.  You just have to inherit from the right base.

The only real difference between these two is the method used to inc/dec the reference count.  The thread safe one uses atomic operations which are supplied by the OS and are different between WIN and POSIX for instance.  I don't know about the speed of WIN operations, but the atomic operations on POSIX are slower than non-atomic counts, but I don't have numbers for how much.

I'd love to see the Android numbers which show refcounted objects in general causing overmuch CPU usage to maintain the count.  Are these available somewhere?

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Colin Blundell]

Thanks for the responses. I think that the issues being raised, while relevant to a larger discussion of refcounting within Chromium, are largely out of the scope of my question (although that likely wasn't clear from the way I phrased my original question).

I'm not asking about whether to use refcounting vs. some other mechanism to manage an object's lifetime; I'm assuming that refcounting *is* being used to manage the object's lifetime, and that changing that fact is not within the scope of the work at hand. My question is: Given that refcounting is being used to manage a given object foo's lifetime, is there a compelling reason for functions that return a reference to foo but don't transfer ownership to return the reference via raw pointers rather than scoped_refptrs?

I understand that using raw pointers in this case would save some amount of refcount maintenance, at the cost of decreasing the clarity of the API. I think that we should optimize for clarity of API (when it doesn't come at the cost of safety), and if refcounting proves to be too expensive/confusing a mechanism for managing a given object foo's lifetime, change to some other mechanism for managing foo's lifetime (as the link that Tom posted advises).

My question is really pretty narrow: Given an object foo whose lifetime is managed by refcounting and a getter GetFoo() that doesn't transfer ownership of foo, should GetFoo() return a scoped_refptr or a raw pointer?

Thanks,

Colin

On Thu, Nov 21, 2013 at 6:24 PM, Tom Hudson <tomh...@google.com> wrote:

[Scott Hess]

If the object is ref-counted, then returning a scoped_refptr<> transfers an ownership share to the caller.  So this is about potentially changing an object's lifetime.

-scott

[Colin Blundell]

Agreed that if a function returns a scoped_ptr, the caller obtains an ownership share. What I mean by "ownership transfer" is "if the caller doesn't hold on to a reference, the object will immediately die."

e.g.

scoped_refptr<Foo> GetFoo() {

  return scoped_refptr<Foo>(new Foo());

}

What I mean by "doesn't do an ownership transfer" is "the callee retains an ownership share", e.g., the callee has the object stored in a scoped_refptr ivar foo_ and the getter looks like:

scoped_refptr<Foo> GetFoo() {

  return scoped_refptr<Foo>(foo_.get());

} 

The difference between the two cases above lies in what happens if the caller stores the returned value in a raw pointer (the problematic case that this thread started off discussing). In the first case, the pointer will immediately point to a deleted object, whereas if the function had been written just with raw pointers and the caller didn't realize that she had ownership over the returned object, the object would leak instead. In the second case, there is no difference in safety between whether the value is returned as a scoped_refptr or a raw pointer: in either way of writing the function, the object dies when the callee gets destroyed (assuming for simplicity that no one else is holding references to the object).

Thanks,

Colin

[Colin Blundell]

Kindly read "returns a scoped_ptr" as "returns a scoped_refptr".

[Scott Hess]

If you're returning a raw pointer to transfer ownership (your first case), you're doing it wrong.  You should indeed return a scoped_refptr<Foo> (or scoped_ptr<Foo> as appropriate) in that case.

If the callee intends to explicitly control ownership of the object in question, then returning scoped_refptr<Foo> implicitly transfers ownership capabilities to the caller.  I mean that in the sense that the caller can keep around that reference after the callee has died, even though the callee and other code may be written with the intention of the Foo object having been destroyed.  If a raw pointer is returned, the caller can indeed cause a use-after-free error by dereferencing it, but that's the caller's mistake, and it is paying the penalty.  At least the destruction ordering of the Foo was maintained.  If a scoped_refptr<Foo> were returned and kept past the lifetime desired by the callee, then the refptr can be dereferenced successfully, but you can't really reason about what will happen if you access anything within the Foo.  Maybe it will work, maybe it won't, maybe it will throw a use-after-free error somewhere else, etc.  All you can do is move the ambiguity around, you can't eliminate it.

-scott

[Jói Sigurðsson]

Just a note re Scott's comment to say that a share of the ownership is
potentially transferred either way. The fact that the object is
reference counted means that the caller can always add a reference and
store it.

It sounds like the added reference counting of returning a
scoped_refptr potentially adds too much overhead for Android, so you
should probably return a raw pointer, assuming that's what we do in a
lot of our codebase (and I think it is).

Cheers,
Jói

[Scott Hess]

If this were a non-ref-counted object, then these two functions are materially different:

Bar* GetBar();

scoped_ptr<Bar> GetBar();

One says "Here's a pointer to something I have, you can use it for a bit", the other says "Here is something you can own and keep around for as long as you want."

If Foo is a ref-counted object, you _can_ take any old pointer to it that you receive and add or remove references to it.  But IMHO this:

  Foo* GetFoo();

indicates "Here's a pointer to something I have, you can use it for a bit", while this:

  scoped_refptr<Foo> GetFoo();

indicates "Here's a reference to something I have a reference to, you can keep it around for as long as you want."

Unfortunately, mentioned performance concerns probably mean that encoding it into the API signature like this is too costly and instead they'll all want to be raw pointers with an unenforcible API convention to determine whether you should be able to access the ref-counted part of things.

[I'm not going to suggest scoped_nonrefptr<Foo> to document transient non-ref-counted access to the Foo.  That is just crazy.]

-scott

[Jói Sigurðsson]

When you contrast it with the scoped_ptr case like that, I have to concur the raw pointer does imply "don't keep it".

[Erik Wright]

Although undesirable, this is necessary for the reasons that Fred Akalin mentioned above:

It's unsafe until we get rid of the implicit raw pointer <-> scoped_refptr conversion.

If you did:
X* x = getX();
and getX() returns a scoped_refptr<X>, it'll be silently converted to a raw pointer, which may be invalid by the time you use it (because something else released the last ref).
So unfortunately returning raw pointers is the safer bet, since memory leaks are less bad than use-after-frees. :/

[Erik Wright]

And although it would be safe in the case where the function keeps a separate reference to the returned object, for consistency I would argue that all reference counted objects should be returned as raw pointers.

Else, developers not understanding why it's safe in one scenario and not another would follow what they see and unsafely return a refptr in an unsafe case.

[Tom Hudson]

On Thu, Nov 21, 2013 at 9:13 PM, Jói Sigurðsson <j...@chromium.org> wrote:

It sounds like the added reference counting of returning a
scoped_refptr potentially adds too much overhead for Android, so you
should probably return a raw pointer, assuming that's what we do in a
lot of our codebase (and I think it is).

Just to clarify, we DO NOT need to completely avoid scoped_refptrs for performance reasons on Android. We need to think twice about creating large data structures full of scoped_refptrs that are going to be built, sorted, and destroyed twice every frame, for an extreme example.

Tom

[David Turner]

That's correct, avoid using them in tight loops, and you should be fine.

Also I will add that the issue is not limited to Android, but to all ARM CPUs, so it also affects ChromeOS/ARM and Chrome for iOS.

[Colin Blundell]

Thanks for the responses, everyone. Erik's response most directly addressed my question of how we should handle functions that do *not* have a risk of a use-after-free if the caller treats the returned scoped_refptr as a raw pointer:

"And although [returning a scoped_refptr rather than a raw pointer] would be safe in the case where the function keeps a separate reference to the returned object, for consistency I would argue that all reference counted objects should be returned as raw pointers.

Else, developers not understanding why it's safe in one scenario and not another would follow what they see and unsafely return a refptr in an unsafe case."

I'm fine with that reasoning.

Best,

Colin

[Sylvain Defresne]

According to this other thread the implicit conversion from scoped_refptr<T> to T* has been removed. Is the recommendation to always return refcounted object as raw pointer still valid or should we prefer to return scoped_refptr<T> as it is safe now?

-- Sylvain

[Daniel Cheng]

Prefer returning as scoped_refptr<T> if it's important for the caller to take a reference to it. A simple example of this would be a factory method for creating some refcounted object Foo. I think there are still many instances when you might want to pass a T* directly though (for example, for a simple getter where the caller doesn't need to take ownership).

Daniel