Force SupportsWeakPtr to only issue weak pointers bound to constructor/destructor's thread.

[Tommy]

See this email for discussion last year: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2pRDD0loJ

Using SupportsWeakPtr to issue weak pointers dereferenced on other threads is dangerous. Because derived class members are destroyed before its weak pointers are invalidated, it can lead to use-after-destroy bugs. This danger is documented in the comments, as well as in the above email.

How about enforcing (in code) that SupportsWeakPtr gives weak pointers that can only be dereferenced on the constructor/destructor's thread? Assuming that the destructor runs on the same thread as the constructor, and this should eliminate the use-after-destroy bugs.

Here's a patch: https://codereview.chromium.org/692233003/

The patch seems to break a bunch of tests, but this hard change might still be a 'good thing'.

Tommy

[Wez]

Hi Tommy,

The issue with SupportsWeakPtr isn't to do with threading, but destructor ordering.

The derived class' desrructor will always be executed first, including member destruction, but only after that will the base SupportsWeakPtr destructor run, invalidating WeakPtrs to it - anything in the derived class' destructor, or triggered by a member's destructor, that triggers that WeakPtr to be dereferenced will succeed, but be accessing a partially-destructed class.

The solution is really to replace SupportsWeakPtr w/ WeakPtrFactory.

HTH,

Wez

[Ryan Sleevi]

+1

And a general +1 to the continued phase-out of SupportsWeakPtr - I NACK it in any review that crosses my plate, because even without this behaviour, it's a pure code-smell of "exposing dangerous capabilities". 

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

[Tommy C. Li]

Hi Wez,

I see what you're saying.

Seems like this patch would close some holes (like pending callbacks), but leave others open (weak pointers dereferenced during derived member destruction)...

Do you think there is a way to save SupportsWeakPtr in any way then? It's more concise to use than adding a WeakPtrFactory everywhere.

Tommy

[Wez]

I don't actually think the patch will help at all; it's perfectly valid for a separate thread to be passed WeakPtrs bound in callbacks targeting the object, regardless of which thread the object was created on originally.

What's critical is that WeakPtrs are invalidated on the same thread they are referenced on, and that that happens before the object starts being torn down.

On 6 November 2014 12:19, Tommy Li <tomm...@google.com> wrote:

Hi Wez,

I see what you're saying.

Seems like this patch would close some holes (like pending callbacks), but leave others open (weak pointers dereferenced during derived member destruction)...

[Wez]

I don't think even C++11 provides any way to have a base class express the need to run code *before* other destructors run, so the derived class would have to call down to explicitly deallocate, which ends up being verbose and error-prone.

More fundamentally, weak-pointers are a bit like ref-counted objects, in that they are useful in very specific cases, but ab-useful in most others; Chrome has plenty of objects that expose ref-counting or weak-references via inheritance but are not designed for *callers* to actually use those capabilities, and that's complexity we should be aiming to reduce via alternative patterns (e.g. SupportsWeakPtr->WeakPtrFactory, RefCounted -> caller-owned-and-DeleteSoon, etc) where possible.