Using Singleton with RefCountedThreadBase classes

[Andrew Scherkus]

I'm surprised I've never run into this before, but the combination of Singleton<T1, T2>, with T1 being a RefCountedThreadBase-derived class, and T2 being DefaultSingletonTraits results in a shutdown DCHECK since DefaultSingletonTraits calls delete and doesn't do the AddRef/Release() dance.

Instead of using LeakySingletonTraits to skirt around this problem, does it makes sense to add a new trait to base/memory/singleton.h that calls AddRef/Release()?

Background: the class in question is an IPC message filter which derives from IPC::ChannelProxy::MessageFilter so removing ref-counting doesn't seem to be an option.

Andrew

[Fred Akalin]

On Mon, Jun 20, 2011 at 5:59 PM, Andrew Scherkus <sche...@chromium.org> wrote:
> I'm surprised I've never run into this before, but the combination of
> Singleton<T1, T2>, with T1 being a RefCountedThreadBase-derived class, and
> T2 being DefaultSingletonTraits results in a shutdown DCHECK
> since DefaultSingletonTraits calls delete and doesn't do the
> AddRef/Release() dance.

Shouldn't this not compile? The usual pattern with ref-counted
classes is that the destructor is protected/private.

> Instead of using LeakySingletonTraits to skirt around this problem, does it
> makes sense to add a new trait to base/memory/singleton.h that calls
> AddRef/Release()?
> Background: the class in question is an IPC message filter which derives
> from IPC::ChannelProxy::MessageFilter so removing ref-counting doesn't seem
> to be an option.

Ref-counted base classes make me sad, but I guess making a
RefCountedSingletonTraits isn't so bad.

[Fred Akalin]

> Ref-counted base classes make me sad, but I guess making a
> RefCountedSingletonTraits isn't so bad.

Come to think of it, singletons make me sad too. :( Have you explored
alternatives?

[Andrew Scherkus]

Yes I believe the alternative is to attach the lifetime to an existing singleton (RenderThread, in this case) + have users either:

  a) Have the message filter instance injected via ctor

  b) Grab the instance via RenderThread

...with:

  a) Causing more code change, but makes for easier-to-test code

  b) Cause less code change, but adds dependency on RenderThread for unit tests

(a) seems the "nicest" way but I don't have any issues with (b) either.

Andrew

[John Abd-El-Malek]

When we've had this come up before, we make RenderThread (or equivalent) keep a reference and have a getter (i.e. your b I think).

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

[Darin Fisher]

+1

It makes me sad to see Singleton<T> used so much in Chrome.  It adds hidden costs.

-Darin

[Andrew Scherkus]

http://codereview.chromium.org/7217014