New issue
Advanced search Search tips

Issue 647430 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Kill SupportsWeakPtr

Project Member Reported by dcheng@chromium.org, Sep 15 2016

Issue description

See summary.
 

Comment 1 by danakj@chromium.org, Sep 16 2016

Super definitely remove this. Maybe we should say why it's terrible:

A base class may SupportsWeakPtr. A derived class can now generate WeakPtrs too, but they would be WeakPtr<derived> not WeakPtr<base>. However they are not invalidated until after the base class is destructed, so there is this period of time where you have WeakPtr<derived> that are not invalidated but the class is already destructed. The same is true for the base class but its harder to run code between the base class destructor and the SupportsWeakPtr destructor (tho not impossible).

Also its preventing dcheng from making WeakPtr smaller. Can you point @ CLs for references?

Comment 2 by w...@chromium.org, Sep 17 2016

It is also terrible because many uses of SupportsWeakPtr are in places in which WeakPtrs were really only intended to be used internally to the class; exposing GetWeakPtr() makes it easy for later changes to calling code to take WeakPtrs and use them in ways that were never intended.
Project Member

Comment 3 by sheriffbot@chromium.org, Sep 18 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 4 by danakj@chromium.org, Sep 19 2017

Status: Available (was: Untriaged)
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 19

Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: dcheng@chromium.org
Status: Available (was: Untriaged)
Restoring this to Available, since I think it is still a desirable goal.

Issues/work removing SupportsWeakPtr will be:

1. Increased size of classes when migrated to WeakPtrFactory (since SupportsWeakPtr holds a ptr-to-Flag, while WeakPtrFactory will hold both ptr-to-Flag and ptr-to-self).

2. Classes A deriving from a base-class B which derives from SupportsWeakPtr, expecting to be able to dispense both WeakPtr<A> and WeakPtr<B> based on the same underlying Flag.  Do we have enough classes using that capability to justify it as an optimization on the number of Flags in the system?

3. Need to add explicit WeakPtr getters to classes which really do need to be able to dispense WeakPtrs to themselves.  This is actually desirable for clarity, IMO, but it's a load of busy work.

dcheng, danakj: WDYT?
1 and 2 sound like extremely rare corner cases where they would come up? If sharing a flag is super important somewhere maybe we provider WeakPtr downcast from B to A?

I agree 3 seems good.
Re #7: It may be that dcheng@ has been removing these use-cases; you can see some calls to base::AsWeakPtr() in e.g. //components/sync, which are returning a WeakPtr<B> given a B that is an A, is a SupportsWeakPtr<A>.

Sign in to add a comment