Make Supplement and Supplementable live and die together

Make Supplement and Supplementable live and die together

Conceptually a Supplement object and a Supplementable object are one object.
Hence the two objects should live and die together.

However, currently Supplement can die before Supplementable dies.
This is because Supplement doesn't have a strong reference to Supplementable.
This CL adds the strong reference.

The real intention of this CL is to introduce Supplement::host().
This is necessary to deprecate LocalFrameLifecycleObserver.
(To deprecate LocalFrameLifecycleObserver, Supplement<LocalFrame> needs to provide
a method that returns a host frame instead of LocalFrameLifecycleObserver.)

BUG=

[haraken, 2016-07-08 06:37:29]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-07-08 06:37:30]

PTAL

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Supplementable.h (right):

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Supplementable.h:96: // TODO(haraken): Remove
this default constructor.

I'll fix the TODO before committing. But I want to confirm if you're okay with
this change before spending time updating all Supplement constructors.

[sof, 2016-07-08 07:07:02]

lgtm to extending Supplements with the extra back pointer.

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Supplementable.h (right):

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Supplementable.h:96: // TODO(haraken): Remove
this default constructor.
On 2016/07/08 06:37:29, haraken wrote:
> 
> I'll fix the TODO before committing. But I want to confirm if you're okay with
> this change before spending time updating all Supplement constructors.

Before Oilpan, Supplementable provided OwnPtr<>-ownership of its Supplements
along with on-demand/lazy creation of the supplements.

With Oilpan, we clearly want the on-demand part, but we need to decide how
ownership should be handled. What you're providing here will make them _all_ die
at the same time, but a reference to one Supplement<> will keep everything alive
-- how do you plan to address that?

I would assume the codebase is pretty much well-behaved in its current state wrt
not holding onto supplements, but it seems like something that could easily
slide & regress, without it being noticed (by the leak detector.)

An abrupt Supplementable detach step might help here?

[haraken, 2016-07-08 07:17:37]

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/Supplementable.h (right):

https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/Supplementable.h:96: // TODO(haraken): Remove
this default constructor.
On 2016/07/08 07:07:02, sof wrote:
> On 2016/07/08 06:37:29, haraken wrote:
> > 
> > I'll fix the TODO before committing. But I want to confirm if you're okay
with
> > this change before spending time updating all Supplement constructors.
> 
> Before Oilpan, Supplementable provided OwnPtr<>-ownership of its Supplements
> along with on-demand/lazy creation of the supplements.
> 
> With Oilpan, we clearly want the on-demand part, but we need to decide how
> ownership should be handled. What you're providing here will make them _all_
die
> at the same time, but a reference to one Supplement<> will keep everything
alive
> -- how do you plan to address that?
> 
> I would assume the codebase is pretty much well-behaved in its current state
wrt
> not holding onto supplements, but it seems like something that could easily
> slide & regress, without it being noticed (by the leak detector.)
> 
> An abrupt Supplementable detach step might help here?

Thanks for the details.

I feel that it would make sense to make _all_ Supplements and the Supplementable
live and die together because they're conceptually the same object. So if
someone keeps a strong reference to the Supplement and causes leaks, I'd say
that it's a bug to be detected by the leak detector.

But I don't have a strong opinion here. If you worry about such leaks, I'm fine
with making the reference a WeakMember<T>.

[haraken, 2016-07-08 07:19:23]

On 2016/07/08 07:17:37, haraken wrote:
>
https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/Supplementable.h (right):
> 
>
https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/Supplementable.h:96: // TODO(haraken):
Remove
> this default constructor.
> On 2016/07/08 07:07:02, sof wrote:
> > On 2016/07/08 06:37:29, haraken wrote:
> > > 
> > > I'll fix the TODO before committing. But I want to confirm if you're okay
> with
> > > this change before spending time updating all Supplement constructors.
> > 
> > Before Oilpan, Supplementable provided OwnPtr<>-ownership of its Supplements
> > along with on-demand/lazy creation of the supplements.
> > 
> > With Oilpan, we clearly want the on-demand part, but we need to decide how
> > ownership should be handled. What you're providing here will make them _all_
> die
> > at the same time, but a reference to one Supplement<> will keep everything
> alive
> > -- how do you plan to address that?
> > 
> > I would assume the codebase is pretty much well-behaved in its current state
> wrt
> > not holding onto supplements, but it seems like something that could easily
> > slide & regress, without it being noticed (by the leak detector.)
> > 
> > An abrupt Supplementable detach step might help here?
> 
> Thanks for the details.
> 
> I feel that it would make sense to make _all_ Supplements and the
Supplementable
> live and die together because they're conceptually the same object. So if
> someone keeps a strong reference to the Supplement and causes leaks, I'd say
> that it's a bug to be detected by the leak detector.
> 
> But I don't have a strong opinion here. If you worry about such leaks, I'm
fine
> with making the reference a WeakMember<T>.

Yeah, in terms of keeping the existing behavior as is, WeakMember<T> would be
better. LocalFrameLifecycleObserver<LocalFrame> also uses a weak pointer to
point to the LocalFrame object.

[sof, 2016-07-08 07:42:05]

On 2016/07/08 07:19:23, haraken wrote:
> On 2016/07/08 07:17:37, haraken wrote:
> >
>
https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
> > File third_party/WebKit/Source/platform/Supplementable.h (right):
> > 
> >
>
https://codereview.chromium.org/2136473002/diff/1/third_party/WebKit/Source/p...
> > third_party/WebKit/Source/platform/Supplementable.h:96: // TODO(haraken):
> Remove
> > this default constructor.
> > On 2016/07/08 07:07:02, sof wrote:
> > > On 2016/07/08 06:37:29, haraken wrote:
> > > > 
> > > > I'll fix the TODO before committing. But I want to confirm if you're
okay
> > with
> > > > this change before spending time updating all Supplement constructors.
> > > 
> > > Before Oilpan, Supplementable provided OwnPtr<>-ownership of its
Supplements
> > > along with on-demand/lazy creation of the supplements.
> > > 
> > > With Oilpan, we clearly want the on-demand part, but we need to decide how
> > > ownership should be handled. What you're providing here will make them
_all_
> > die
> > > at the same time, but a reference to one Supplement<> will keep everything
> > alive
> > > -- how do you plan to address that?
> > > 
> > > I would assume the codebase is pretty much well-behaved in its current
state
> > wrt
> > > not holding onto supplements, but it seems like something that could
easily
> > > slide & regress, without it being noticed (by the leak detector.)
> > > 
> > > An abrupt Supplementable detach step might help here?
> > 
> > Thanks for the details.
> > 
> > I feel that it would make sense to make _all_ Supplements and the
> Supplementable
> > live and die together because they're conceptually the same object. So if
> > someone keeps a strong reference to the Supplement and causes leaks, I'd say
> > that it's a bug to be detected by the leak detector.
> > 
> > But I don't have a strong opinion here. If you worry about such leaks, I'm
> fine
> > with making the reference a WeakMember<T>.
> 
> Yeah, in terms of keeping the existing behavior as is, WeakMember<T> would be
> better. LocalFrameLifecycleObserver<LocalFrame> also uses a weak pointer to
> point to the LocalFrame object.

The alternative is to manually clear out m_host on the Supplements when the
Supplementable becomes detached (I think they all do have such a step..?). I've
no strong opinion/preference for which is used atm.

I agree that conceptually they're one object.