base::Bind behavior with raw pointers of refcounted classes

[Sunny Sachanandani]

Hi chromium-dev,

I'm investigating a crash and the debugging so far points to my misunderstanding base::Bind behavior w.r.t. refcounting. docs/callback.md gives a few examples implying that parameters are retained if caller and callee use scoped_refptrs and to use base::RetainedRef if the callee takes a raw ptr. However, the documentation doesn't say anything about when the callee expects a scoped_refptr and the caller provides a raw ptr.

Is the object retained in these cases? Is the behavior different if the parameter is allocated in place using operator new (because ref counted classes start with ref count 1)?

In the context of the bug, MediaGpuChannel passes a raw MediaGpuChannelMessageFilter ptr to GpuChannel::AddChannel which posts a task to GpuChannelMessageFilter::AddChannelFilter which expects a scoped_refptr. The crash started happening after a change I made, before that the call was AddChannel(new MediaGpuChannelMessageFilter...).

More details are in the linked bug. Thanks for any help!

- Sunny

[Daniel Cheng]

Hmm... it appears this regressed at some point. We have explicit compile-time checks [1] that refcounted objects are bound as a scoped_refptr, but a simple test case (and the linked bug) clearly demonstrate this is not the case.

Daniel

[1] https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=ab62a8b172d7d04bf021d9f010c9a1c9fa92d66b&l=114

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAKqK8M%3DhboVq8XXOv2LwouomVFsxyyb_Z-nD0OTd%2BC_XeFSRcw%40mail.gmail.com.

[Daniel Cheng]

I've narrowed down the failure and filed https://crbug.com/737010.

Daniel

[Taiju Tsuiki]

base::Bind is intended to reject that pattern, and its implementation has a static_assert for it. Let me look into.

2017年6月27日(火) 12:36 Daniel Cheng <dch...@chromium.org>:

[Daniel Cheng]

On Mon, Jun 26, 2017 at 7:52 PM Sunny Sachanandani <sun...@chromium.org> wrote:

Hi chromium-dev,

I'm investigating a crash and the debugging so far points to my misunderstanding base::Bind behavior w.r.t. refcounting. docs/callback.md gives a few examples implying that parameters are retained if caller and callee use scoped_refptrs and to use base::RetainedRef if the callee takes a raw ptr. However, the documentation doesn't say anything about when the callee expects a scoped_refptr and the caller provides a raw ptr.

Is the object retained in these cases? Is the behavior different if the parameter is allocated in place using operator new (because ref counted classes start with ref count 1)?

Also, in Chromium, (most) ref-counted classes begin with a refcount of zero. If not immediately stored into a scoped_refptr, the results can be surprising--if a temporary scoped_refptr to the ref counted object is created (easy to do by accident, since T* implicitly converts to scoped_refptr<T>), then the object will be destroyed as soon as the temporary goes out of scope.

(There's a long-term plan to try fixing this; in the mean time, it's probably best to prefer base::MakeRefCounted where possible).

Daniel

 

In the context of the bug, MediaGpuChannel passes a raw MediaGpuChannelMessageFilter ptr to GpuChannel::AddChannel which posts a task to GpuChannelMessageFilter::AddChannelFilter which expects a scoped_refptr. The crash started happening after a change I made, before that the call was AddChannel(new MediaGpuChannelMessageFilter...).

More details are in the linked bug. Thanks for any help!

- Sunny

--

[Sunny Sachanandani]

Thanks for filing bugs.

I suspect this bug was hidden for a long time because using new without a scoped_refptr caused the object to leak and my CL changed the leak to a crash. I'll change the methods to take scoped_refptr which should fix the immediate issue.

[Matthew Menke]

We clearly need ASSERT_DOES_NOT_COMPILE.

[dan...@chromium.org]

On Tue, Jun 27, 2017 at 3:47 PM, 'Matthew Menke' via Chromium-dev <chromi...@chromium.org> wrote:

We clearly need ASSERT_DOES_NOT_COMPILE.

https://cs.chromium.org/chromium/src/base/bind_unittest.nc?q=f:%5C.nc$&sq=package:chromium&dr

 

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/104f9660-08b1-4233-8a3b-fb125d66cf3a%40chromium.org.

[Daniel Cheng]

We do have one as danakj@ linked: unfortunately, it just happened not to cover the case that regressed.

Daniel