One way to resolve this might be to mandate that any class that has a WeakPtrFactory, must be final. (This is just the first thing to come to mind. It's not necessarily the best solution, or even a viable one.)

Added CL to demonstrate the problem: https://chromium-review.googlesource.com/c/chromium/src/+/1033750

when I've encountered this problem in the past I've added a virtual AsWeakPtr() method to the base class and given each leaf class the WeakPtrFactory member and implement that method.

Past discussions for context:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/1umM0irvLAw/t-VpNLlD5fUJ
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/Oy3uuiTSwDk/5UDyRJcpJsUJ


The root issue arises when you have an inheritance hierarchy with WeakPtrs, and that arises most commonly when you're exposing WeakPtrs as part of the contract. In this case, the proof's example of this is its public exposure of GetWeakPtr(), which allows the test to make assessments on the lifetime. Further, the example also illustrates a problematic practice (in which accessing != owning for the WeakPtr). Once a WeakPtr is bound, the Factory itself is bound, and the Factory should be destroyed on the same thread. In the example, it isn't - it's being destroyed on the owning_task_runner, rather than the bound accessing_task_runner.

reillyg@, I agree that this would work; it's in fact what I had imagined would go along with my initial proposed solution (comment #1) of mandating any class that has a WeakPtrFactory would have to be final. Then, only the leaves of the inheritance graph would have a concrete implementation of GetWeakPtr (or equivalent function), which would return the product of their specific factory.

rsleevi@
1. Thanks for the links; I'll read them immediately after posting this response.
2. Exposing GetWeakPtr() appears to be a common pattern in the codebase. Using git-grep, I came across DataUseTabModel::GetWeakPtr. (Please note that this is the first example I could find, not the best example I could find.)
3. Could you please look at the code again? If I am not mistaken, I had both created as well as destroyed the factory on owning_task_runner_, as well as produced the WeakPtr on it - all, as far as I can tell, according to the instructions set forth by the documentation in weak_ptr.h.

Also see issue 647430, which is a more specific version of this bug.

I left a comment on the CL highlighting the lifetime bug, which is why it doesn't demonstrate the issue expected (that is, it's purely a lifetime bug issue, not a DTOR ordering issue)

 rsleevi@, thanks - I see my mistake now. I misread some of the documentation - dereferencing the WeakPtr vs. producing it (by the factory).

Despite my bad example, I believe that the problem that the documentation claims the factory must be the last member, while subclassing would allow other members to appear first, still seems like an issue to me. Either someone else has already shown why this is problematic (I've been given a lot of links; it will take some time to read them all), or I'll have to produce a different example. (At the very least, if it's not a problem, the documentation might have to be amended.)

eladalon: dcheng@'s bug discusses the problem with the 'last member' issue in Comment #1.

Yes, issue #647430 did also mention this. I don't think the problem is only with SupportsWeakPtr, though. AFAICT, anything short of mandating that all classes that have the factory, must be final, would still have the issue, because there's nothing else that could reliably stop similar misuse that would just not use SupportsWeakPtr. Wdyt?

Here's a new example - https://chromium-review.googlesource.com/c/chromium/src/+/1039569
It seems to me that it demonstrates that even putting WeakPtrFactory last is not necessarily enough, and that InvalidateWeakPtrs() should be called from the beginning of the body of the most derived class's destructor.

While that is an extreme that can be taken, I think the principles outlined in Comment #6 offer more foundational guidance around WeakPtr usage. If those principles are applied and followed - such as that WeakPtr's are only bound locally - then any such risks can be evaluated within the context of a single .cc, and their destruction order can be reasoned about.

This is why the guidance in #6 is important - exposing WeakPtr publicly makes it difficult to reason about, but within a translation unit is reasonable.

To be clear: I'm not disagreeing the problem exists, merely that it's not a common use of WeakPtr, and that the triggering behaviour for this issue relies on known-unsafe-and-unsound patterns (for example, Base object exposing its WeakPtr and doing callbacks-in-dtors, both of which are problematic anti-patterns). While mandating Final is a solution (and one enforcible by the clang plugin), I do worry that it's a bit of a thin line for writing plugins for things that reviewers should not be letting through.

Gottcha, rsleevi@. I think it's a grey area, how much needs to be mandated impossible and how much code review should prevent. I will wait to see if your opinion of no-action-necessary is the consensus before I close this bug. However, please see this (apparent) bug, which I've found while looking into WeakPtrs in general (I am not familiar with this area in the code). It seems to me to illustrate (unless I am mistaken in labeling it a bug) that if something can be missed by code review, it occasionally will; anti-patterns will be employed. Any time we can avoid that using automatic tools, I think it good to do so.

Aforementioned bug: https://bugs.chromium.org/p/chromium/issues/detail?id=838873

Wez, shall I consider you as completing the consensus to drop this issue, or do you find any of the arguments here compelling? (Namely - I have pointed out that it's easier to have automatic tools ward off anti-patterns, than it is for reviewers.)

Re #19: We are working (albeit slowly) to harden WeakPtr to apply more run-time checks to ensure Sequence-safety.

I would suggest closing this bug out as WontFix since, as-per the discussion, the original issue you were raising is more one of anti-patterns being dangerous in general than a WeakPtr-specific issue.

If you have specific proposals for static checks you think would provide benefit without applying onerous limits on Chromium code then I would suggest putting those e.g. in a Google Doc on @chromium.org and using that to discuss & agree what checks would make sense.