Expose scroll customization for touch to JS (behind REF).

Expose scroll customization for touch to JS (behind REF).

This is a reland of https://codereview.chromium.org/1209183004/, which
was reverted because it caused both a memory leak and a sizes
regression.

This enables scroll customization of touch scrolls for JS.  See
http://goo.gl/xCBm7L for details. This is a followup to
https://codereview.chromium.org/988823003, and completes landing
https://codereview.chromium.org/850443002/.

This approach replaces the previous approach, from
https://codereview.chromium.org/1057603002.

BUG=410974
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201736

[tdresser, 2015-07-17 14:26:11]

Patchset #2 (id:20001) has been deleted

[tdresser, 2015-07-28 19:23:34]

The previous implementation used a map from RawPtrWillBeWeakMember<Element> to
Member<ScrollStateCallback>.

However, the ScrollStateCallback may have a strong reference to the associated
Element, which causes the Element to never be collected, which prevents the
ScrollStateCallback from being collected.

This circular dependency appears to be solved for normal event listeners via
some custom bindings logic
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
This isn't an issue for Custom Element callbacks, as they're contained by the
element.

I need a way to indicate that ScrollStateCallback should be collected whenever
its associated Element is freed. This is complicated by the fact that Element
isn't garbage collected in non-Oilpan builds.

rbyers@ pointed out that the .NET framework uses a "dependent handle"
(https://github.com/dotnet/coreclr/blob/4cf8a6b082d9bb1789facd996d8265d3908757...)
for exactly this purpose
(http://blogs.msdn.com/b/dotnet/archive/2009/05/18/the-conditional-weak-table-...).

Any idea how I can accomplish this? Do I need to author some custom bindings
code to avoid leaking?

Thanks!

[tdresser, 2015-07-29 13:40:30]

+jochen, who might have some thoughts on the right way to solve this problem.

[Rick Byers, 2015-07-29 14:45:50]

On 2015/07/29 13:40:30, tdresser wrote:
> +jochen, who might have some thoughts on the right way to solve this problem.

High-level summary: we're trying to add a field to Element without taking up
space in every single Element (since it's rarely used).  Conceptually in oil-pan
this shouldn't be too hard: we just need Element::trace to be the thing that
traces the callback objects (as opposed to the map itself).  But this may set
off alarms in the tooling.  Plus we need to support non-oilpan Element too, so I
guess Element would have to hold the ref for the callback object.

Doing this manually for either oilpan or ref counting doesn't seem too hard to
get right (if we can convince the tooling it's not a leak).  But it'll be uglier
to do it cleanly for both.  Ideally there's a pattern for this already in blink.
 If not, maybe we need to add a ConditionalWeakTable (or ExpandoObject) facility
to blink.

Unlike the previous problems with scroll customization and bindings, this one is
fundamental to any API design we choose I think.  So definitely worth investing
in to do right.

[haraken, 2015-07-29 14:58:27]

On 2015/07/29 14:45:50, Rick Byers wrote:
> On 2015/07/29 13:40:30, tdresser wrote:
> > +jochen, who might have some thoughts on the right way to solve this
problem.
> 
> High-level summary: we're trying to add a field to Element without taking up
> space in every single Element (since it's rarely used).  Conceptually in
oil-pan
> this shouldn't be too hard: we just need Element::trace to be the thing that
> traces the callback objects (as opposed to the map itself).  But this may set
> off alarms in the tooling.  Plus we need to support non-oilpan Element too, so
I
> guess Element would have to hold the ref for the callback object.
> 
> Doing this manually for either oilpan or ref counting doesn't seem too hard to
> get right (if we can convince the tooling it's not a leak).  But it'll be
uglier
> to do it cleanly for both.  Ideally there's a pattern for this already in
blink.
>  If not, maybe we need to add a ConditionalWeakTable (or ExpandoObject)
facility
> to blink.
> 
> Unlike the previous problems with scroll customization and bindings, this one
is
> fundamental to any API design we choose I think.  So definitely worth
investing
> in to do right.

First of all, oilpan will solve that problem because a reference cycle is
allowed in oilpan.

Without oilpan, we normally breaks the cycle by representing the DOM-side
reference as a JS-side reference (i.e., a hidden value of a V8 object). In this
particular case, we can do something like:

1) Make sure that ScrollCallback always has a wrapper.
2) Create the Element's wrapper if it doesn't exist.
3) Store the Element's wrapper into the hidden value of the ScrollCallback's
wrapper.

The hidden value guarantees that the Element's wrapper is kept alive while the
ScrollCallback's wrapper is alive. This guarantees that the Element is kept
alive while the ScrollCallback is alive. Thus you don't need to add a RefPtr
from the ScrollCallback to the Element.

I agree that this is ugly. We have the ugliness in multiple places in the code
base. Our plan is to fix them by shipping oilpan.

[tdresser, 2015-07-29 15:27:47]

On 2015/07/29 14:58:27, haraken wrote:
> On 2015/07/29 14:45:50, Rick Byers wrote:
> > On 2015/07/29 13:40:30, tdresser wrote:
> > > +jochen, who might have some thoughts on the right way to solve this
> problem.
> > 
> > High-level summary: we're trying to add a field to Element without taking up
> > space in every single Element (since it's rarely used).  Conceptually in
> oil-pan
> > this shouldn't be too hard: we just need Element::trace to be the thing that
> > traces the callback objects (as opposed to the map itself).  But this may
set
> > off alarms in the tooling.  Plus we need to support non-oilpan Element too,
so
> I
> > guess Element would have to hold the ref for the callback object.
> > 
> > Doing this manually for either oilpan or ref counting doesn't seem too hard
to
> > get right (if we can convince the tooling it's not a leak).  But it'll be
> uglier
> > to do it cleanly for both.  Ideally there's a pattern for this already in
> blink.
> >  If not, maybe we need to add a ConditionalWeakTable (or ExpandoObject)
> facility
> > to blink.
> > 
> > Unlike the previous problems with scroll customization and bindings, this
one
> is
> > fundamental to any API design we choose I think.  So definitely worth
> investing
> > in to do right.
> 
> First of all, oilpan will solve that problem because a reference cycle is
> allowed in oilpan.
> 
> Without oilpan, we normally breaks the cycle by representing the DOM-side
> reference as a JS-side reference (i.e., a hidden value of a V8 object). In
this
> particular case, we can do something like:
> 
> 1) Make sure that ScrollCallback always has a wrapper.
> 2) Create the Element's wrapper if it doesn't exist.
> 3) Store the Element's wrapper into the hidden value of the ScrollCallback's
> wrapper.
> 
> The hidden value guarantees that the Element's wrapper is kept alive while the
> ScrollCallback's wrapper is alive. This guarantees that the Element is kept
> alive while the ScrollCallback is alive. Thus you don't need to add a RefPtr
> from the ScrollCallback to the Element.
> 
> I agree that this is ugly. We have the ugliness in multiple places in the code
> base. Our plan is to fix them by shipping oilpan.

We want to keep the callback around while either:
- it's associated element exists, or
- javascript has a reference to it

We aren't trying to keep the element alive for the lifetime of the callback,
we're trying to keep the callback alive for the lifetime of the element.

Your proposal sounds like it may work if reversed though. I'd need to store the
ScrollStateCallback's wrapper into the hidden value of the Element's wrapper.
Then we should be able to collect both simultaneously. I could then use a
HeapHashMap<RawPtrWillBeWeakMember<Element>,
RawPtrWillBeWeakMember<ScrollStateCallback>> to perform lookups that don't
require conversion to and from v8::Handles, without leaking memory.

> First of all, oilpan will solve that problem because a reference cycle is
> allowed in oilpan.

I don't think the previously suggested approach works in oilpan. The map is
essentially persistent, and has a strong reference to the callbacks, which may
have strong references to their Elements. Although there is essentially a
reference cycle, the map will prevent the cycle from being collected.

[haraken, 2015-07-29 16:41:55]

On 2015/07/29 15:27:47, tdresser wrote:
> On 2015/07/29 14:58:27, haraken wrote:
> > On 2015/07/29 14:45:50, Rick Byers wrote:
> > > On 2015/07/29 13:40:30, tdresser wrote:
> > > > +jochen, who might have some thoughts on the right way to solve this
> > problem.
> > > 
> > > High-level summary: we're trying to add a field to Element without taking
up
> > > space in every single Element (since it's rarely used).  Conceptually in
> > oil-pan
> > > this shouldn't be too hard: we just need Element::trace to be the thing
that
> > > traces the callback objects (as opposed to the map itself).  But this may
> set
> > > off alarms in the tooling.  Plus we need to support non-oilpan Element
too,
> so
> > I
> > > guess Element would have to hold the ref for the callback object.
> > > 
> > > Doing this manually for either oilpan or ref counting doesn't seem too
hard
> to
> > > get right (if we can convince the tooling it's not a leak).  But it'll be
> > uglier
> > > to do it cleanly for both.  Ideally there's a pattern for this already in
> > blink.
> > >  If not, maybe we need to add a ConditionalWeakTable (or ExpandoObject)
> > facility
> > > to blink.
> > > 
> > > Unlike the previous problems with scroll customization and bindings, this
> one
> > is
> > > fundamental to any API design we choose I think.  So definitely worth
> > investing
> > > in to do right.
> > 
> > First of all, oilpan will solve that problem because a reference cycle is
> > allowed in oilpan.
> > 
> > Without oilpan, we normally breaks the cycle by representing the DOM-side
> > reference as a JS-side reference (i.e., a hidden value of a V8 object). In
> this
> > particular case, we can do something like:
> > 
> > 1) Make sure that ScrollCallback always has a wrapper.
> > 2) Create the Element's wrapper if it doesn't exist.
> > 3) Store the Element's wrapper into the hidden value of the ScrollCallback's
> > wrapper.
> > 
> > The hidden value guarantees that the Element's wrapper is kept alive while
the
> > ScrollCallback's wrapper is alive. This guarantees that the Element is kept
> > alive while the ScrollCallback is alive. Thus you don't need to add a RefPtr
> > from the ScrollCallback to the Element.
> > 
> > I agree that this is ugly. We have the ugliness in multiple places in the
code
> > base. Our plan is to fix them by shipping oilpan.
> 
> We want to keep the callback around while either:
> - it's associated element exists, or
> - javascript has a reference to it
> 
> We aren't trying to keep the element alive for the lifetime of the callback,
> we're trying to keep the callback alive for the lifetime of the element.

Why can't you realize the lifetime management by:

PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
Member<ScrollStateCallback>> s_map;

class ScrollStateCallback : public GarbageCollected<ScrollStateCallback> {
};

class Element : public RefCountedWillBeGarbageCollected<Element> {
  ~Element() {
#if !ENABLE(OILPAN)
    s_map.remove(this);
#endif
  }
};

?

[tdresser, 2015-07-29 17:08:02]

On 2015/07/29 16:41:55, haraken wrote:
> On 2015/07/29 15:27:47, tdresser wrote:
> > On 2015/07/29 14:58:27, haraken wrote:
> > > On 2015/07/29 14:45:50, Rick Byers wrote:
> > > > On 2015/07/29 13:40:30, tdresser wrote:
> > > > > +jochen, who might have some thoughts on the right way to solve this
> > > problem.
> > > > 
> > > > High-level summary: we're trying to add a field to Element without
taking
> up
> > > > space in every single Element (since it's rarely used).  Conceptually in
> > > oil-pan
> > > > this shouldn't be too hard: we just need Element::trace to be the thing
> that
> > > > traces the callback objects (as opposed to the map itself).  But this
may
> > set
> > > > off alarms in the tooling.  Plus we need to support non-oilpan Element
> too,
> > so
> > > I
> > > > guess Element would have to hold the ref for the callback object.
> > > > 
> > > > Doing this manually for either oilpan or ref counting doesn't seem too
> hard
> > to
> > > > get right (if we can convince the tooling it's not a leak).  But it'll
be
> > > uglier
> > > > to do it cleanly for both.  Ideally there's a pattern for this already
in
> > > blink.
> > > >  If not, maybe we need to add a ConditionalWeakTable (or ExpandoObject)
> > > facility
> > > > to blink.
> > > > 
> > > > Unlike the previous problems with scroll customization and bindings,
this
> > one
> > > is
> > > > fundamental to any API design we choose I think.  So definitely worth
> > > investing
> > > > in to do right.
> > > 
> > > First of all, oilpan will solve that problem because a reference cycle is
> > > allowed in oilpan.
> > > 
> > > Without oilpan, we normally breaks the cycle by representing the DOM-side
> > > reference as a JS-side reference (i.e., a hidden value of a V8 object). In
> > this
> > > particular case, we can do something like:
> > > 
> > > 1) Make sure that ScrollCallback always has a wrapper.
> > > 2) Create the Element's wrapper if it doesn't exist.
> > > 3) Store the Element's wrapper into the hidden value of the
ScrollCallback's
> > > wrapper.
> > > 
> > > The hidden value guarantees that the Element's wrapper is kept alive while
> the
> > > ScrollCallback's wrapper is alive. This guarantees that the Element is
kept
> > > alive while the ScrollCallback is alive. Thus you don't need to add a
RefPtr
> > > from the ScrollCallback to the Element.
> > > 
> > > I agree that this is ugly. We have the ugliness in multiple places in the
> code
> > > base. Our plan is to fix them by shipping oilpan.
> > 
> > We want to keep the callback around while either:
> > - it's associated element exists, or
> > - javascript has a reference to it
> > 
> > We aren't trying to keep the element alive for the lifetime of the callback,
> > we're trying to keep the callback alive for the lifetime of the element.
> 
> Why can't you realize the lifetime management by:
> 
> PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> Member<ScrollStateCallback>> s_map;
> 
> class ScrollStateCallback : public GarbageCollected<ScrollStateCallback> {
> };
> 
> class Element : public RefCountedWillBeGarbageCollected<Element> {
>   ~Element() {
> #if !ENABLE(OILPAN)
>     s_map.remove(this);
> #endif
>   }
> };
> 
> ?

That's precisely what the patch above does.

The problem is that the persistent map keeps the ScrollStateCallback alive, and
the ScrollStateCallback may have a strong reference to the Element, which
prevents the Element from being collected.

[haraken, 2015-07-29 17:43:31]

On 2015/07/29 17:08:02, tdresser wrote:
> On 2015/07/29 16:41:55, haraken wrote:
> > On 2015/07/29 15:27:47, tdresser wrote:
> > > On 2015/07/29 14:58:27, haraken wrote:
> > > > On 2015/07/29 14:45:50, Rick Byers wrote:
> > > > > On 2015/07/29 13:40:30, tdresser wrote:
> > > > > > +jochen, who might have some thoughts on the right way to solve this
> > > > problem.
> > > > > 
> > > > > High-level summary: we're trying to add a field to Element without
> taking
> > up
> > > > > space in every single Element (since it's rarely used).  Conceptually
in
> > > > oil-pan
> > > > > this shouldn't be too hard: we just need Element::trace to be the
thing
> > that
> > > > > traces the callback objects (as opposed to the map itself).  But this
> may
> > > set
> > > > > off alarms in the tooling.  Plus we need to support non-oilpan Element
> > too,
> > > so
> > > > I
> > > > > guess Element would have to hold the ref for the callback object.
> > > > > 
> > > > > Doing this manually for either oilpan or ref counting doesn't seem too
> > hard
> > > to
> > > > > get right (if we can convince the tooling it's not a leak).  But it'll
> be
> > > > uglier
> > > > > to do it cleanly for both.  Ideally there's a pattern for this already
> in
> > > > blink.
> > > > >  If not, maybe we need to add a ConditionalWeakTable (or
ExpandoObject)
> > > > facility
> > > > > to blink.
> > > > > 
> > > > > Unlike the previous problems with scroll customization and bindings,
> this
> > > one
> > > > is
> > > > > fundamental to any API design we choose I think.  So definitely worth
> > > > investing
> > > > > in to do right.
> > > > 
> > > > First of all, oilpan will solve that problem because a reference cycle
is
> > > > allowed in oilpan.
> > > > 
> > > > Without oilpan, we normally breaks the cycle by representing the
DOM-side
> > > > reference as a JS-side reference (i.e., a hidden value of a V8 object).
In
> > > this
> > > > particular case, we can do something like:
> > > > 
> > > > 1) Make sure that ScrollCallback always has a wrapper.
> > > > 2) Create the Element's wrapper if it doesn't exist.
> > > > 3) Store the Element's wrapper into the hidden value of the
> ScrollCallback's
> > > > wrapper.
> > > > 
> > > > The hidden value guarantees that the Element's wrapper is kept alive
while
> > the
> > > > ScrollCallback's wrapper is alive. This guarantees that the Element is
> kept
> > > > alive while the ScrollCallback is alive. Thus you don't need to add a
> RefPtr
> > > > from the ScrollCallback to the Element.
> > > > 
> > > > I agree that this is ugly. We have the ugliness in multiple places in
the
> > code
> > > > base. Our plan is to fix them by shipping oilpan.
> > > 
> > > We want to keep the callback around while either:
> > > - it's associated element exists, or
> > > - javascript has a reference to it
> > > 
> > > We aren't trying to keep the element alive for the lifetime of the
callback,
> > > we're trying to keep the callback alive for the lifetime of the element.
> > 
> > Why can't you realize the lifetime management by:
> > 
> > PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> > Member<ScrollStateCallback>> s_map;
> > 
> > class ScrollStateCallback : public GarbageCollected<ScrollStateCallback> {
> > };
> > 
> > class Element : public RefCountedWillBeGarbageCollected<Element> {
> >   ~Element() {
> > #if !ENABLE(OILPAN)
> >     s_map.remove(this);
> > #endif
> >   }
> > };
> > 
> > ?
> 
> That's precisely what the patch above does.
> 
> The problem is that the persistent map keeps the ScrollStateCallback alive,
and
> the ScrollStateCallback may have a strong reference to the Element, which
> prevents the Element from being collected.

Ah, now I totally understand the problem.

In oilpan, this problem is being resolved by the mechanism called "ephemeron"
(https://en.wikipedia.org/wiki/Ephemeron). If you have:

  PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
Member<ScrollStateCallback>> s_map;

then oilpan does:

- if the Element is reachable (in the other way than the persistent hash map),
keep the map entry
- otherwise, remove the map entry.

Non-oilpan doesn't have a mechanism to accomplish such an advanced weak
processing, so you're hitting the problem :-/

Let me think about it a bit more.

[haraken, 2015-07-29 17:46:48]

On 2015/07/29 17:43:31, haraken wrote:
> On 2015/07/29 17:08:02, tdresser wrote:
> > On 2015/07/29 16:41:55, haraken wrote:
> > > On 2015/07/29 15:27:47, tdresser wrote:
> > > > On 2015/07/29 14:58:27, haraken wrote:
> > > > > On 2015/07/29 14:45:50, Rick Byers wrote:
> > > > > > On 2015/07/29 13:40:30, tdresser wrote:
> > > > > > > +jochen, who might have some thoughts on the right way to solve
this
> > > > > problem.
> > > > > > 
> > > > > > High-level summary: we're trying to add a field to Element without
> > taking
> > > up
> > > > > > space in every single Element (since it's rarely used). 
Conceptually
> in
> > > > > oil-pan
> > > > > > this shouldn't be too hard: we just need Element::trace to be the
> thing
> > > that
> > > > > > traces the callback objects (as opposed to the map itself).  But
this
> > may
> > > > set
> > > > > > off alarms in the tooling.  Plus we need to support non-oilpan
Element
> > > too,
> > > > so
> > > > > I
> > > > > > guess Element would have to hold the ref for the callback object.
> > > > > > 
> > > > > > Doing this manually for either oilpan or ref counting doesn't seem
too
> > > hard
> > > > to
> > > > > > get right (if we can convince the tooling it's not a leak).  But
it'll
> > be
> > > > > uglier
> > > > > > to do it cleanly for both.  Ideally there's a pattern for this
already
> > in
> > > > > blink.
> > > > > >  If not, maybe we need to add a ConditionalWeakTable (or
> ExpandoObject)
> > > > > facility
> > > > > > to blink.
> > > > > > 
> > > > > > Unlike the previous problems with scroll customization and bindings,
> > this
> > > > one
> > > > > is
> > > > > > fundamental to any API design we choose I think.  So definitely
worth
> > > > > investing
> > > > > > in to do right.
> > > > > 
> > > > > First of all, oilpan will solve that problem because a reference cycle
> is
> > > > > allowed in oilpan.
> > > > > 
> > > > > Without oilpan, we normally breaks the cycle by representing the
> DOM-side
> > > > > reference as a JS-side reference (i.e., a hidden value of a V8
object).
> In
> > > > this
> > > > > particular case, we can do something like:
> > > > > 
> > > > > 1) Make sure that ScrollCallback always has a wrapper.
> > > > > 2) Create the Element's wrapper if it doesn't exist.
> > > > > 3) Store the Element's wrapper into the hidden value of the
> > ScrollCallback's
> > > > > wrapper.
> > > > > 
> > > > > The hidden value guarantees that the Element's wrapper is kept alive
> while
> > > the
> > > > > ScrollCallback's wrapper is alive. This guarantees that the Element is
> > kept
> > > > > alive while the ScrollCallback is alive. Thus you don't need to add a
> > RefPtr
> > > > > from the ScrollCallback to the Element.
> > > > > 
> > > > > I agree that this is ugly. We have the ugliness in multiple places in
> the
> > > code
> > > > > base. Our plan is to fix them by shipping oilpan.
> > > > 
> > > > We want to keep the callback around while either:
> > > > - it's associated element exists, or
> > > > - javascript has a reference to it
> > > > 
> > > > We aren't trying to keep the element alive for the lifetime of the
> callback,
> > > > we're trying to keep the callback alive for the lifetime of the element.
> > > 
> > > Why can't you realize the lifetime management by:
> > > 
> > > PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> > > Member<ScrollStateCallback>> s_map;
> > > 
> > > class ScrollStateCallback : public GarbageCollected<ScrollStateCallback> {
> > > };
> > > 
> > > class Element : public RefCountedWillBeGarbageCollected<Element> {
> > >   ~Element() {
> > > #if !ENABLE(OILPAN)
> > >     s_map.remove(this);
> > > #endif
> > >   }
> > > };
> > > 
> > > ?
> > 
> > That's precisely what the patch above does.
> > 
> > The problem is that the persistent map keeps the ScrollStateCallback alive,
> and
> > the ScrollStateCallback may have a strong reference to the Element, which
> > prevents the Element from being collected.
> 
> Ah, now I totally understand the problem.
> 
> In oilpan, this problem is being resolved by the mechanism called "ephemeron"
> (https://en.wikipedia.org/wiki/Ephemeron). If you have:
> 
>   PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> Member<ScrollStateCallback>> s_map;
> 
> then oilpan does:
> 
> - if the Element is reachable (in the other way than the persistent hash map),
> keep the map entry
> - otherwise, remove the map entry.
> 
> Non-oilpan doesn't have a mechanism to accomplish such an advanced weak
> processing, so you're hitting the problem :-/
> 
> Let me think about it a bit more.

So the problem is that you want to have a bi-directional strong reference
between the Element and ScrollCallback. Then can we just implement what I
mentioned in #5? In other words, we can represetn the ScrollCallback => Element
reference as a JS-side reference using a hidden value on the ScriptCallback
wrapper. (You can do the other way (i.e., use a hidden value of the Element
wrapper), but it has a risk of regressing performance since the Element wrapper
is performance-sensitive.)

[haraken, 2015-07-29 17:48:09]

On 2015/07/29 17:46:48, haraken wrote:
> On 2015/07/29 17:43:31, haraken wrote:
> > On 2015/07/29 17:08:02, tdresser wrote:
> > > On 2015/07/29 16:41:55, haraken wrote:
> > > > On 2015/07/29 15:27:47, tdresser wrote:
> > > > > On 2015/07/29 14:58:27, haraken wrote:
> > > > > > On 2015/07/29 14:45:50, Rick Byers wrote:
> > > > > > > On 2015/07/29 13:40:30, tdresser wrote:
> > > > > > > > +jochen, who might have some thoughts on the right way to solve
> this
> > > > > > problem.
> > > > > > > 
> > > > > > > High-level summary: we're trying to add a field to Element without
> > > taking
> > > > up
> > > > > > > space in every single Element (since it's rarely used). 
> Conceptually
> > in
> > > > > > oil-pan
> > > > > > > this shouldn't be too hard: we just need Element::trace to be the
> > thing
> > > > that
> > > > > > > traces the callback objects (as opposed to the map itself).  But
> this
> > > may
> > > > > set
> > > > > > > off alarms in the tooling.  Plus we need to support non-oilpan
> Element
> > > > too,
> > > > > so
> > > > > > I
> > > > > > > guess Element would have to hold the ref for the callback object.
> > > > > > > 
> > > > > > > Doing this manually for either oilpan or ref counting doesn't seem
> too
> > > > hard
> > > > > to
> > > > > > > get right (if we can convince the tooling it's not a leak).  But
> it'll
> > > be
> > > > > > uglier
> > > > > > > to do it cleanly for both.  Ideally there's a pattern for this
> already
> > > in
> > > > > > blink.
> > > > > > >  If not, maybe we need to add a ConditionalWeakTable (or
> > ExpandoObject)
> > > > > > facility
> > > > > > > to blink.
> > > > > > > 
> > > > > > > Unlike the previous problems with scroll customization and
bindings,
> > > this
> > > > > one
> > > > > > is
> > > > > > > fundamental to any API design we choose I think.  So definitely
> worth
> > > > > > investing
> > > > > > > in to do right.
> > > > > > 
> > > > > > First of all, oilpan will solve that problem because a reference
cycle
> > is
> > > > > > allowed in oilpan.
> > > > > > 
> > > > > > Without oilpan, we normally breaks the cycle by representing the
> > DOM-side
> > > > > > reference as a JS-side reference (i.e., a hidden value of a V8
> object).
> > In
> > > > > this
> > > > > > particular case, we can do something like:
> > > > > > 
> > > > > > 1) Make sure that ScrollCallback always has a wrapper.
> > > > > > 2) Create the Element's wrapper if it doesn't exist.
> > > > > > 3) Store the Element's wrapper into the hidden value of the
> > > ScrollCallback's
> > > > > > wrapper.
> > > > > > 
> > > > > > The hidden value guarantees that the Element's wrapper is kept alive
> > while
> > > > the
> > > > > > ScrollCallback's wrapper is alive. This guarantees that the Element
is
> > > kept
> > > > > > alive while the ScrollCallback is alive. Thus you don't need to add
a
> > > RefPtr
> > > > > > from the ScrollCallback to the Element.
> > > > > > 
> > > > > > I agree that this is ugly. We have the ugliness in multiple places
in
> > the
> > > > code
> > > > > > base. Our plan is to fix them by shipping oilpan.
> > > > > 
> > > > > We want to keep the callback around while either:
> > > > > - it's associated element exists, or
> > > > > - javascript has a reference to it
> > > > > 
> > > > > We aren't trying to keep the element alive for the lifetime of the
> > callback,
> > > > > we're trying to keep the callback alive for the lifetime of the
element.
> > > > 
> > > > Why can't you realize the lifetime management by:
> > > > 
> > > > PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> > > > Member<ScrollStateCallback>> s_map;
> > > > 
> > > > class ScrollStateCallback : public GarbageCollected<ScrollStateCallback>
{
> > > > };
> > > > 
> > > > class Element : public RefCountedWillBeGarbageCollected<Element> {
> > > >   ~Element() {
> > > > #if !ENABLE(OILPAN)
> > > >     s_map.remove(this);
> > > > #endif
> > > >   }
> > > > };
> > > > 
> > > > ?
> > > 
> > > That's precisely what the patch above does.
> > > 
> > > The problem is that the persistent map keeps the ScrollStateCallback
alive,
> > and
> > > the ScrollStateCallback may have a strong reference to the Element, which
> > > prevents the Element from being collected.
> > 
> > Ah, now I totally understand the problem.
> > 
> > In oilpan, this problem is being resolved by the mechanism called
"ephemeron"
> > (https://en.wikipedia.org/wiki/Ephemeron). If you have:
> > 
> >   PersistentHeapHashMap<RawPtrWillBeWeakMember<Element>,
> > Member<ScrollStateCallback>> s_map;
> > 
> > then oilpan does:
> > 
> > - if the Element is reachable (in the other way than the persistent hash
map),
> > keep the map entry
> > - otherwise, remove the map entry.
> > 
> > Non-oilpan doesn't have a mechanism to accomplish such an advanced weak
> > processing, so you're hitting the problem :-/
> > 
> > Let me think about it a bit more.
> 
> So the problem is that you want to have a bi-directional strong reference
> between the Element and ScrollCallback. Then can we just implement what I
> mentioned in #5? In other words, we can represetn the ScrollCallback =>
Element
> reference as a JS-side reference using a hidden value on the ScriptCallback
> wrapper. (You can do the other way (i.e., use a hidden value of the Element
> wrapper), but it has a risk of regressing performance since the Element
wrapper
> is performance-sensitive.)

Either way, the long-term solution should be to ship oilpan (hopefully will
happen in Q3). I'm OK with taking any short-term solution that works out for
this particular problem.

[tdresser, 2015-07-29 19:32:08]

Thanks for your help here!

> So the problem is that you want to have a bi-directional strong reference
> between the Element and ScrollCallback. Then can we just implement what I
> mentioned in #5? In other words, we can represetn the ScrollCallback =>
Element
> reference as a JS-side reference using a hidden value on the ScriptCallback
> wrapper. (You can do the other way (i.e., use a hidden value of the Element
> wrapper), but it has a risk of regressing performance since the Element
wrapper
> is performance-sensitive.)

If we only use a hidden value on the ScriptCallback to store a reference to the
Element,
what keeps the ScriptCallback alive if there's no JS reference to it anymore?

We need something to keep the ScriptCallback alive.


For example, if a developer executes:

element.setApplyScroll(function() {...});
And then the user scrolls, we need to ensure that we don't collect the callback
until |element| is destroyed.

[haraken, 2015-07-29 19:40:22]

On 2015/07/29 19:32:08, tdresser wrote:
> Thanks for your help here!
> 
> > So the problem is that you want to have a bi-directional strong reference
> > between the Element and ScrollCallback. Then can we just implement what I
> > mentioned in #5? In other words, we can represetn the ScrollCallback =>
> Element
> > reference as a JS-side reference using a hidden value on the ScriptCallback
> > wrapper. (You can do the other way (i.e., use a hidden value of the Element
> > wrapper), but it has a risk of regressing performance since the Element
> wrapper
> > is performance-sensitive.)
> 
> If we only use a hidden value on the ScriptCallback to store a reference to
the
> Element,
> what keeps the ScriptCallback alive if there's no JS reference to it anymore?
> 
> We need something to keep the ScriptCallback alive.
> 
> 
> For example, if a developer executes:
> 
> element.setApplyScroll(function() {...});
> And then the user scrolls, we need to ensure that we don't collect the
callback
> until |element| is destroyed.

How about this idea?

- Remove the persistent hash map entirely.
- Forget about the hidden value.
- Use a RefPtrWillBeMember<Element> from the ScrollCallback to the Element.
- Add [Custom=visitDOMWrapper] on Element.idl. In the visitDOMWrapper callback
(which is called when a GC visits DOM wrappers), you can visit the
ScriptCallback's wrapper from the Element's wrapper. (See
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
for example.)

This approach will work as long as the Element has a wrapper. If the Element's
wrapper is gone for some reason, then the visitDOMWrapper won't be called and
thus the ScrollCallback wrapper will get collected. But will it happen? (i.e.,
can we assume that the Element has a wrapper while we want to keep the
ScrollCallback alive?)

[Rick Byers, 2015-07-29 23:45:39]

On 2015/07/29 19:40:22, haraken wrote:
> On 2015/07/29 19:32:08, tdresser wrote:
> > Thanks for your help here!
> > 
> > > So the problem is that you want to have a bi-directional strong reference
> > > between the Element and ScrollCallback. Then can we just implement what I
> > > mentioned in #5? In other words, we can represetn the ScrollCallback =>
> > Element
> > > reference as a JS-side reference using a hidden value on the
ScriptCallback
> > > wrapper. (You can do the other way (i.e., use a hidden value of the
Element
> > > wrapper), but it has a risk of regressing performance since the Element
> > wrapper
> > > is performance-sensitive.)
> > 
> > If we only use a hidden value on the ScriptCallback to store a reference to
> the
> > Element,
> > what keeps the ScriptCallback alive if there's no JS reference to it
anymore?
> > 
> > We need something to keep the ScriptCallback alive.
> > 
> > 
> > For example, if a developer executes:
> > 
> > element.setApplyScroll(function() {...});
> > And then the user scrolls, we need to ensure that we don't collect the
> callback
> > until |element| is destroyed.
> 
> How about this idea?
> 
> - Remove the persistent hash map entirely.
> - Forget about the hidden value.
> - Use a RefPtrWillBeMember<Element> from the ScrollCallback to the Element.
> - Add [Custom=visitDOMWrapper] on Element.idl. In the visitDOMWrapper callback
> (which is called when a GC visits DOM wrappers), you can visit the
> ScriptCallback's wrapper from the Element's wrapper. (See
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> for example.)
> 
> This approach will work as long as the Element has a wrapper. If the Element's
> wrapper is gone for some reason, then the visitDOMWrapper won't be called and
> thus the ScrollCallback wrapper will get collected. But will it happen? (i.e.,
> can we assume that the Element has a wrapper while we want to keep the
> ScrollCallback alive?)

I haven't had a chance to read this whole thread (sorry, just boarded a plane,
about to leave the gate).  But I had a chance to chat with Ojan about this
problem today to see if he was aware of a similar pattern in blink.  He
suggested that we shouldn't be using a map at all.  Instead introduce an
ElementRareData class and find some existing pointer in Element that we can
union to be rare data (and some bit indicating when that's used). 
StyleRare*Data follows a pattern like this.  It used to use a map, but they
found switching to a RareData object was a big perf improvement (no hash lookup)
while being about neutral on memory.  Sorry I didn't think of this pattern
earlier.  It's certainly simpler than using a map if we can pull it off. 
Perhaps there are other 'rare' members of Element you'd want to move to RareData
at the same time.  Feel free to chat with Ojan while he's in WAT for further
advice here.

[haraken, 2015-07-30 08:42:23]

On 2015/07/29 23:45:39, Rick Byers (Out until Aug 4th) wrote:
> On 2015/07/29 19:40:22, haraken wrote:
> > On 2015/07/29 19:32:08, tdresser wrote:
> > > Thanks for your help here!
> > > 
> > > > So the problem is that you want to have a bi-directional strong
reference
> > > > between the Element and ScrollCallback. Then can we just implement what
I
> > > > mentioned in #5? In other words, we can represetn the ScrollCallback =>
> > > Element
> > > > reference as a JS-side reference using a hidden value on the
> ScriptCallback
> > > > wrapper. (You can do the other way (i.e., use a hidden value of the
> Element
> > > > wrapper), but it has a risk of regressing performance since the Element
> > > wrapper
> > > > is performance-sensitive.)
> > > 
> > > If we only use a hidden value on the ScriptCallback to store a reference
to
> > the
> > > Element,
> > > what keeps the ScriptCallback alive if there's no JS reference to it
> anymore?
> > > 
> > > We need something to keep the ScriptCallback alive.
> > > 
> > > 
> > > For example, if a developer executes:
> > > 
> > > element.setApplyScroll(function() {...});
> > > And then the user scrolls, we need to ensure that we don't collect the
> > callback
> > > until |element| is destroyed.
> > 
> > How about this idea?
> > 
> > - Remove the persistent hash map entirely.
> > - Forget about the hidden value.
> > - Use a RefPtrWillBeMember<Element> from the ScrollCallback to the Element.
> > - Add [Custom=visitDOMWrapper] on Element.idl. In the visitDOMWrapper
callback
> > (which is called when a GC visits DOM wrappers), you can visit the
> > ScriptCallback's wrapper from the Element's wrapper. (See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> > for example.)
> > 
> > This approach will work as long as the Element has a wrapper. If the
Element's
> > wrapper is gone for some reason, then the visitDOMWrapper won't be called
and
> > thus the ScrollCallback wrapper will get collected. But will it happen?
(i.e.,
> > can we assume that the Element has a wrapper while we want to keep the
> > ScrollCallback alive?)
> 
> I haven't had a chance to read this whole thread (sorry, just boarded a plane,
> about to leave the gate).  But I had a chance to chat with Ojan about this
> problem today to see if he was aware of a similar pattern in blink.  He
> suggested that we shouldn't be using a map at all.  Instead introduce an
> ElementRareData class and find some existing pointer in Element that we can
> union to be rare data (and some bit indicating when that's used). 
> StyleRare*Data follows a pattern like this.  It used to use a map, but they
> found switching to a RareData object was a big perf improvement (no hash
lookup)
> while being about neutral on memory.  Sorry I didn't think of this pattern
> earlier.  It's certainly simpler than using a map if we can pull it off. 
> Perhaps there are other 'rare' members of Element you'd want to move to
RareData
> at the same time.  Feel free to chat with Ojan while he's in WAT for further
> advice here.

I don't think RareData is helpful to solve the problem here. Even if we use
RareData, the following cycle is produced:

Element =(RefPtr)=> SomeRareData =(Persistent)=> ScrollCallback =(RefPtr)=>
Element

The problem is not whether we should use RareData or a map, but how to break the
reference cycle.

[tdresser, 2015-07-30 14:04:45]

On 2015/07/30 08:42:23, haraken wrote:
> On 2015/07/29 23:45:39, Rick Byers (Out until Aug 4th) wrote:
> > On 2015/07/29 19:40:22, haraken wrote:
> > > On 2015/07/29 19:32:08, tdresser wrote:
> > > > Thanks for your help here!
> > > > 
> > > > > So the problem is that you want to have a bi-directional strong
> reference
> > > > > between the Element and ScrollCallback. Then can we just implement
what
> I
> > > > > mentioned in #5? In other words, we can represetn the ScrollCallback
=>
> > > > Element
> > > > > reference as a JS-side reference using a hidden value on the
> > ScriptCallback
> > > > > wrapper. (You can do the other way (i.e., use a hidden value of the
> > Element
> > > > > wrapper), but it has a risk of regressing performance since the
Element
> > > > wrapper
> > > > > is performance-sensitive.)
> > > > 
> > > > If we only use a hidden value on the ScriptCallback to store a reference
> to
> > > the
> > > > Element,
> > > > what keeps the ScriptCallback alive if there's no JS reference to it
> > anymore?
> > > > 
> > > > We need something to keep the ScriptCallback alive.
> > > > 
> > > > 
> > > > For example, if a developer executes:
> > > > 
> > > > element.setApplyScroll(function() {...});
> > > > And then the user scrolls, we need to ensure that we don't collect the
> > > callback
> > > > until |element| is destroyed.
> > > 
> > > How about this idea?
> > > 
> > > - Remove the persistent hash map entirely.
> > > - Forget about the hidden value.
> > > - Use a RefPtrWillBeMember<Element> from the ScrollCallback to the
Element.
> > > - Add [Custom=visitDOMWrapper] on Element.idl. In the visitDOMWrapper
> callback
> > > (which is called when a GC visits DOM wrappers), you can visit the
> > > ScriptCallback's wrapper from the Element's wrapper. (See
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> > > for example.)
> > > 
> > > This approach will work as long as the Element has a wrapper. If the
> Element's
> > > wrapper is gone for some reason, then the visitDOMWrapper won't be called
> and
> > > thus the ScrollCallback wrapper will get collected. But will it happen?
> (i.e.,
> > > can we assume that the Element has a wrapper while we want to keep the
> > > ScrollCallback alive?)
> > 
> > I haven't had a chance to read this whole thread (sorry, just boarded a
plane,
> > about to leave the gate).  But I had a chance to chat with Ojan about this
> > problem today to see if he was aware of a similar pattern in blink.  He
> > suggested that we shouldn't be using a map at all.  Instead introduce an
> > ElementRareData class and find some existing pointer in Element that we can
> > union to be rare data (and some bit indicating when that's used). 
> > StyleRare*Data follows a pattern like this.  It used to use a map, but they
> > found switching to a RareData object was a big perf improvement (no hash
> lookup)
> > while being about neutral on memory.  Sorry I didn't think of this pattern
> > earlier.  It's certainly simpler than using a map if we can pull it off. 
> > Perhaps there are other 'rare' members of Element you'd want to move to
> RareData
> > at the same time.  Feel free to chat with Ojan while he's in WAT for further
> > advice here.
> 
> I don't think RareData is helpful to solve the problem here. Even if we use
> RareData, the following cycle is produced:
> 
> Element =(RefPtr)=> SomeRareData =(Persistent)=> ScrollCallback =(RefPtr)=>
> Element
> 
> The problem is not whether we should use RareData or a map, but how to break
the
> reference cycle.

I don't think tying the callback's lifetime to the Element wrapper's lifetime is
adequate, as the wrapper will be cleaned up if no script holds onto the element,
but we could still need to trigger the callback due to user scrolling.

That prevents using the hidden value approach and the [Custom=visitDOMWrapper]
approach, if I understand correctly.

[haraken, 2015-07-30 14:17:20]

On 2015/07/30 14:04:45, tdresser wrote:
> On 2015/07/30 08:42:23, haraken wrote:
> > On 2015/07/29 23:45:39, Rick Byers (Out until Aug 4th) wrote:
> > > On 2015/07/29 19:40:22, haraken wrote:
> > > > On 2015/07/29 19:32:08, tdresser wrote:
> > > > > Thanks for your help here!
> > > > > 
> > > > > > So the problem is that you want to have a bi-directional strong
> > reference
> > > > > > between the Element and ScrollCallback. Then can we just implement
> what
> > I
> > > > > > mentioned in #5? In other words, we can represetn the ScrollCallback
> =>
> > > > > Element
> > > > > > reference as a JS-side reference using a hidden value on the
> > > ScriptCallback
> > > > > > wrapper. (You can do the other way (i.e., use a hidden value of the
> > > Element
> > > > > > wrapper), but it has a risk of regressing performance since the
> Element
> > > > > wrapper
> > > > > > is performance-sensitive.)
> > > > > 
> > > > > If we only use a hidden value on the ScriptCallback to store a
reference
> > to
> > > > the
> > > > > Element,
> > > > > what keeps the ScriptCallback alive if there's no JS reference to it
> > > anymore?
> > > > > 
> > > > > We need something to keep the ScriptCallback alive.
> > > > > 
> > > > > 
> > > > > For example, if a developer executes:
> > > > > 
> > > > > element.setApplyScroll(function() {...});
> > > > > And then the user scrolls, we need to ensure that we don't collect the
> > > > callback
> > > > > until |element| is destroyed.
> > > > 
> > > > How about this idea?
> > > > 
> > > > - Remove the persistent hash map entirely.
> > > > - Forget about the hidden value.
> > > > - Use a RefPtrWillBeMember<Element> from the ScrollCallback to the
> Element.
> > > > - Add [Custom=visitDOMWrapper] on Element.idl. In the visitDOMWrapper
> > callback
> > > > (which is called when a GC visits DOM wrappers), you can visit the
> > > > ScriptCallback's wrapper from the Element's wrapper. (See
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> > > > for example.)
> > > > 
> > > > This approach will work as long as the Element has a wrapper. If the
> > Element's
> > > > wrapper is gone for some reason, then the visitDOMWrapper won't be
called
> > and
> > > > thus the ScrollCallback wrapper will get collected. But will it happen?
> > (i.e.,
> > > > can we assume that the Element has a wrapper while we want to keep the
> > > > ScrollCallback alive?)
> > > 
> > > I haven't had a chance to read this whole thread (sorry, just boarded a
> plane,
> > > about to leave the gate).  But I had a chance to chat with Ojan about this
> > > problem today to see if he was aware of a similar pattern in blink.  He
> > > suggested that we shouldn't be using a map at all.  Instead introduce an
> > > ElementRareData class and find some existing pointer in Element that we
can
> > > union to be rare data (and some bit indicating when that's used). 
> > > StyleRare*Data follows a pattern like this.  It used to use a map, but
they
> > > found switching to a RareData object was a big perf improvement (no hash
> > lookup)
> > > while being about neutral on memory.  Sorry I didn't think of this pattern
> > > earlier.  It's certainly simpler than using a map if we can pull it off. 
> > > Perhaps there are other 'rare' members of Element you'd want to move to
> > RareData
> > > at the same time.  Feel free to chat with Ojan while he's in WAT for
further
> > > advice here.
> > 
> > I don't think RareData is helpful to solve the problem here. Even if we use
> > RareData, the following cycle is produced:
> > 
> > Element =(RefPtr)=> SomeRareData =(Persistent)=> ScrollCallback =(RefPtr)=>
> > Element
> > 
> > The problem is not whether we should use RareData or a map, but how to break
> the
> > reference cycle.
> 
> I don't think tying the callback's lifetime to the Element wrapper's lifetime
is
> adequate, as the wrapper will be cleaned up if no script holds onto the
element,
> but we could still need to trigger the callback due to user scrolling.
> 
> That prevents using the hidden value approach and the [Custom=visitDOMWrapper]
> approach, if I understand correctly.

Just want to confirm, but our problem boils down to the following problem,
right?

- The Element has a strong reference to the ScrollCallback.
- The ScrollCallback has a strong reference to the Element.
- How can we handle the bi-directional strong references?
- Replacing the map with ElementRareData is not helpful at all.

[tdresser, 2015-07-30 14:25:16]

> Just want to confirm, but our problem boils down to the following problem,
> right?
> 
> - The Element has a strong reference to the ScrollCallback.
The Element does not necessarily have a reference to the ScrollCallback. We need
some way to get the ScrollCallback for a given Element, and we need to keep the
ScrollCallback alive for as long as the Element is alive. The easiest way to
achieve this would be to give the Element a strong reference to the
ScrollCallback.

> - The ScrollCallback has a strong reference to the Element.
I think this is only true sometimes. If the script author writes a callback that
doesn't store a reference to the Element, then I don't think we get this
reference. This will be true the vast majority of the time though.

> - How can we handle the bi-directional strong references?
Although bi-directional strong references may not be the only way to solve this
problem, it may be the most straight forward. If we could handle the
bi-directional strong references, I think we could find a relatively straight
forward solution.

> - Replacing the map with ElementRareData is not helpful at all.
If my understanding is correct, with Oilpan on, using the map results in leaks,
whereas using ElementRareData doesn't. With Oilpan off, both will leak.

[haraken, 2015-07-30 14:32:36]

On 2015/07/30 14:25:16, tdresser wrote:
> > Just want to confirm, but our problem boils down to the following problem,
> > right?
> > 
> > - The Element has a strong reference to the ScrollCallback.
> The Element does not necessarily have a reference to the ScrollCallback. We
need
> some way to get the ScrollCallback for a given Element, and we need to keep
the
> ScrollCallback alive for as long as the Element is alive. The easiest way to
> achieve this would be to give the Element a strong reference to the
> ScrollCallback.
> 
> > - The ScrollCallback has a strong reference to the Element.
> I think this is only true sometimes. If the script author writes a callback
that
> doesn't store a reference to the Element, then I don't think we get this
> reference. This will be true the vast majority of the time though.
> 
> > - How can we handle the bi-directional strong references?
> Although bi-directional strong references may not be the only way to solve
this
> problem, it may be the most straight forward. If we could handle the
> bi-directional strong references, I think we could find a relatively straight
> forward solution.

So, conceptually the Element and the ScrollCallback can have bi-directional
strong references. The problem is how to implement the bi-directional strong
references in a non-leaky way.

> > - Replacing the map with ElementRareData is not helpful at all.
> If my understanding is correct, with Oilpan on, using the map results in
leaks,
> whereas using ElementRareData doesn't. With Oilpan off, both will leak.

With oilpan enabled, the map won't leak if you use
PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.

Actually, this problem is hard to fix without having oilpan. Given that this is
just an experimental feature and that it's rare the ScrollCalback has a strong
reference to the Element, would it be an option to accept the leak? We'll be
able to ship Oilpan by the time you ship the ScrollCallback.

[tdresser, 2015-07-30 14:44:50]

On 2015/07/30 14:32:36, haraken wrote:
> On 2015/07/30 14:25:16, tdresser wrote:
> > > Just want to confirm, but our problem boils down to the following problem,
> > > right?
> > > 
> > > - The Element has a strong reference to the ScrollCallback.
> > The Element does not necessarily have a reference to the ScrollCallback. We
> need
> > some way to get the ScrollCallback for a given Element, and we need to keep
> the
> > ScrollCallback alive for as long as the Element is alive. The easiest way to
> > achieve this would be to give the Element a strong reference to the
> > ScrollCallback.
> > 
> > > - The ScrollCallback has a strong reference to the Element.
> > I think this is only true sometimes. If the script author writes a callback
> that
> > doesn't store a reference to the Element, then I don't think we get this
> > reference. This will be true the vast majority of the time though.
> > 
> > > - How can we handle the bi-directional strong references?
> > Although bi-directional strong references may not be the only way to solve
> this
> > problem, it may be the most straight forward. If we could handle the
> > bi-directional strong references, I think we could find a relatively
straight
> > forward solution.
> 
> So, conceptually the Element and the ScrollCallback can have bi-directional
> strong references. The problem is how to implement the bi-directional strong
> references in a non-leaky way.
> 
> > > - Replacing the map with ElementRareData is not helpful at all.
> > If my understanding is correct, with Oilpan on, using the map results in
> leaks,
> > whereas using ElementRareData doesn't. With Oilpan off, both will leak.
> 
> With oilpan enabled, the map won't leak if you use
> PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> 
> Actually, this problem is hard to fix without having oilpan. Given that this
is
> just an experimental feature and that it's rare the ScrollCalback has a strong
> reference to the Element, would it be an option to accept the leak? We'll be
> able to ship Oilpan by the time you ship the ScrollCallback.

I think we can accept temporarily leaking in the non-oilpan case when behind a
flag. In the event that we do want to ship scroll customization before oilpan
ships, we can revisit this.

Doesn't that map still leak?

The cycle is disguised a bit, but we still have 
PersistentMap => ScrollCallback => Element,
and the only way that the ScrollCallback will be collected is if the Element
goes away, so we effectively also have:
Element => ScrollCallback.

[haraken, 2015-07-30 14:58:08]

On 2015/07/30 14:44:50, tdresser wrote:
> On 2015/07/30 14:32:36, haraken wrote:
> > On 2015/07/30 14:25:16, tdresser wrote:
> > > > Just want to confirm, but our problem boils down to the following
problem,
> > > > right?
> > > > 
> > > > - The Element has a strong reference to the ScrollCallback.
> > > The Element does not necessarily have a reference to the ScrollCallback.
We
> > need
> > > some way to get the ScrollCallback for a given Element, and we need to
keep
> > the
> > > ScrollCallback alive for as long as the Element is alive. The easiest way
to
> > > achieve this would be to give the Element a strong reference to the
> > > ScrollCallback.
> > > 
> > > > - The ScrollCallback has a strong reference to the Element.
> > > I think this is only true sometimes. If the script author writes a
callback
> > that
> > > doesn't store a reference to the Element, then I don't think we get this
> > > reference. This will be true the vast majority of the time though.
> > > 
> > > > - How can we handle the bi-directional strong references?
> > > Although bi-directional strong references may not be the only way to solve
> > this
> > > problem, it may be the most straight forward. If we could handle the
> > > bi-directional strong references, I think we could find a relatively
> straight
> > > forward solution.
> > 
> > So, conceptually the Element and the ScrollCallback can have bi-directional
> > strong references. The problem is how to implement the bi-directional strong
> > references in a non-leaky way.
> > 
> > > > - Replacing the map with ElementRareData is not helpful at all.
> > > If my understanding is correct, with Oilpan on, using the map results in
> > leaks,
> > > whereas using ElementRareData doesn't. With Oilpan off, both will leak.
> > 
> > With oilpan enabled, the map won't leak if you use
> > PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> > 
> > Actually, this problem is hard to fix without having oilpan. Given that this
> is
> > just an experimental feature and that it's rare the ScrollCalback has a
strong
> > reference to the Element, would it be an option to accept the leak? We'll be
> > able to ship Oilpan by the time you ship the ScrollCallback.
> 
> I think we can accept temporarily leaking in the non-oilpan case when behind a
> flag. In the event that we do want to ship scroll customization before oilpan
> ships, we can revisit this.

Sounds reasonable to me. (Actually we have the same problem in other (already
shipped) places as well :)

> Doesn't that map still leak?
> 
> The cycle is disguised a bit, but we still have 
> PersistentMap => ScrollCallback => Element,
> and the only way that the ScrollCallback will be collected is if the Element
> goes away, so we effectively also have:
> Element => ScrollCallback.

PersistentMap<Member<Element>, Member<ScrollCallback>> will cause the problem
you described, but PersistentMap<WeakMember<Element>, Member<ScrollCallback>>
won't cause the problem. The PersistentMap doesn't visit the Element and the
ScrollCallback unless the Element is reachable in another way. (This is the
mechanism called "ephemeron".)

[tdresser, 2015-07-31 14:47:11]

On 2015/07/30 14:58:08, haraken wrote:
> On 2015/07/30 14:44:50, tdresser wrote:
> > On 2015/07/30 14:32:36, haraken wrote:
> > > On 2015/07/30 14:25:16, tdresser wrote:
> > > > > Just want to confirm, but our problem boils down to the following
> problem,
> > > > > right?
> > > > > 
> > > > > - The Element has a strong reference to the ScrollCallback.
> > > > The Element does not necessarily have a reference to the ScrollCallback.
> We
> > > need
> > > > some way to get the ScrollCallback for a given Element, and we need to
> keep
> > > the
> > > > ScrollCallback alive for as long as the Element is alive. The easiest
way
> to
> > > > achieve this would be to give the Element a strong reference to the
> > > > ScrollCallback.
> > > > 
> > > > > - The ScrollCallback has a strong reference to the Element.
> > > > I think this is only true sometimes. If the script author writes a
> callback
> > > that
> > > > doesn't store a reference to the Element, then I don't think we get this
> > > > reference. This will be true the vast majority of the time though.
> > > > 
> > > > > - How can we handle the bi-directional strong references?
> > > > Although bi-directional strong references may not be the only way to
solve
> > > this
> > > > problem, it may be the most straight forward. If we could handle the
> > > > bi-directional strong references, I think we could find a relatively
> > straight
> > > > forward solution.
> > > 
> > > So, conceptually the Element and the ScrollCallback can have
bi-directional
> > > strong references. The problem is how to implement the bi-directional
strong
> > > references in a non-leaky way.
> > > 
> > > > > - Replacing the map with ElementRareData is not helpful at all.
> > > > If my understanding is correct, with Oilpan on, using the map results in
> > > leaks,
> > > > whereas using ElementRareData doesn't. With Oilpan off, both will leak.
> > > 
> > > With oilpan enabled, the map won't leak if you use
> > > PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> > > 
> > > Actually, this problem is hard to fix without having oilpan. Given that
this
> > is
> > > just an experimental feature and that it's rare the ScrollCalback has a
> strong
> > > reference to the Element, would it be an option to accept the leak? We'll
be
> > > able to ship Oilpan by the time you ship the ScrollCallback.
> > 
> > I think we can accept temporarily leaking in the non-oilpan case when behind
a
> > flag. In the event that we do want to ship scroll customization before
oilpan
> > ships, we can revisit this.
> 
> Sounds reasonable to me. (Actually we have the same problem in other (already
> shipped) places as well :)
> 
> > Doesn't that map still leak?
> > 
> > The cycle is disguised a bit, but we still have 
> > PersistentMap => ScrollCallback => Element,
> > and the only way that the ScrollCallback will be collected is if the Element
> > goes away, so we effectively also have:
> > Element => ScrollCallback.
> 
> PersistentMap<Member<Element>, Member<ScrollCallback>> will cause the problem
> you described, but PersistentMap<WeakMember<Element>, Member<ScrollCallback>>
> won't cause the problem. The PersistentMap doesn't visit the Element and the
> ScrollCallback unless the Element is reachable in another way. (This is the
> mechanism called "ephemeron".)

Interesting.

As far as I can tell, that's exactly what I did in the patch above, but it still
leaks even with oilpan on.
The callbacks are stored in a HeapHashMap<RawPtrWillBeWeakMember<Element>,
Member<ScrollStateCallback>>.

I wrote a quick test of ephemeron to verify that my understanding is correct:
TEST(HeapTest, EphemeronVerification)
{
    typedef HeapHashSet<Member<IntWrapper>> Set;
    typedef PersistentHeapHashMap<WeakMember<IntWrapper>, Member<Set>> Map;

    Map map;

    IntWrapper* i = IntWrapper::create(1);
    Set* set = new Set();
    set->add(i);
    map.add(i, set);

    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
ThreadState::GCWithSweep, Heap::ForcedGC);

    EXPECT_EQ(0u, map.size());
}

and it works fine.


Presumably this means that I'm wrong about where the cycle is.
However, if I switch from storing the real callback to storing a fake callback
which doesn't contain a reference to anything, then the leak goes away.

Are there any tools available that I could use to try to figure out where the
cycle is coming from?
If my understanding is correct, no matter what the ScrollStateCallback has a
reference to, as long as it's only referenced by the map in which it has a
WeakMember key, it shouldn't be able to cause leaks, right?

[haraken, 2015-07-31 15:01:25]

On 2015/07/31 14:47:11, tdresser wrote:
> On 2015/07/30 14:58:08, haraken wrote:
> > On 2015/07/30 14:44:50, tdresser wrote:
> > > On 2015/07/30 14:32:36, haraken wrote:
> > > > On 2015/07/30 14:25:16, tdresser wrote:
> > > > > > Just want to confirm, but our problem boils down to the following
> > problem,
> > > > > > right?
> > > > > > 
> > > > > > - The Element has a strong reference to the ScrollCallback.
> > > > > The Element does not necessarily have a reference to the
ScrollCallback.
> > We
> > > > need
> > > > > some way to get the ScrollCallback for a given Element, and we need to
> > keep
> > > > the
> > > > > ScrollCallback alive for as long as the Element is alive. The easiest
> way
> > to
> > > > > achieve this would be to give the Element a strong reference to the
> > > > > ScrollCallback.
> > > > > 
> > > > > > - The ScrollCallback has a strong reference to the Element.
> > > > > I think this is only true sometimes. If the script author writes a
> > callback
> > > > that
> > > > > doesn't store a reference to the Element, then I don't think we get
this
> > > > > reference. This will be true the vast majority of the time though.
> > > > > 
> > > > > > - How can we handle the bi-directional strong references?
> > > > > Although bi-directional strong references may not be the only way to
> solve
> > > > this
> > > > > problem, it may be the most straight forward. If we could handle the
> > > > > bi-directional strong references, I think we could find a relatively
> > > straight
> > > > > forward solution.
> > > > 
> > > > So, conceptually the Element and the ScrollCallback can have
> bi-directional
> > > > strong references. The problem is how to implement the bi-directional
> strong
> > > > references in a non-leaky way.
> > > > 
> > > > > > - Replacing the map with ElementRareData is not helpful at all.
> > > > > If my understanding is correct, with Oilpan on, using the map results
in
> > > > leaks,
> > > > > whereas using ElementRareData doesn't. With Oilpan off, both will
leak.
> > > > 
> > > > With oilpan enabled, the map won't leak if you use
> > > > PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> > > > 
> > > > Actually, this problem is hard to fix without having oilpan. Given that
> this
> > > is
> > > > just an experimental feature and that it's rare the ScrollCalback has a
> > strong
> > > > reference to the Element, would it be an option to accept the leak?
We'll
> be
> > > > able to ship Oilpan by the time you ship the ScrollCallback.
> > > 
> > > I think we can accept temporarily leaking in the non-oilpan case when
behind
> a
> > > flag. In the event that we do want to ship scroll customization before
> oilpan
> > > ships, we can revisit this.
> > 
> > Sounds reasonable to me. (Actually we have the same problem in other
(already
> > shipped) places as well :)
> > 
> > > Doesn't that map still leak?
> > > 
> > > The cycle is disguised a bit, but we still have 
> > > PersistentMap => ScrollCallback => Element,
> > > and the only way that the ScrollCallback will be collected is if the
Element
> > > goes away, so we effectively also have:
> > > Element => ScrollCallback.
> > 
> > PersistentMap<Member<Element>, Member<ScrollCallback>> will cause the
problem
> > you described, but PersistentMap<WeakMember<Element>,
Member<ScrollCallback>>
> > won't cause the problem. The PersistentMap doesn't visit the Element and the
> > ScrollCallback unless the Element is reachable in another way. (This is the
> > mechanism called "ephemeron".)
> 
> Interesting.
> 
> As far as I can tell, that's exactly what I did in the patch above, but it
still
> leaks even with oilpan on.
> The callbacks are stored in a HeapHashMap<RawPtrWillBeWeakMember<Element>,
> Member<ScrollStateCallback>>.
> 
> I wrote a quick test of ephemeron to verify that my understanding is correct:
> TEST(HeapTest, EphemeronVerification)
> {
>     typedef HeapHashSet<Member<IntWrapper>> Set;
>     typedef PersistentHeapHashMap<WeakMember<IntWrapper>, Member<Set>> Map;
> 
>     Map map;
> 
>     IntWrapper* i = IntWrapper::create(1);
>     Set* set = new Set();
>     set->add(i);
>     map.add(i, set);
> 
>     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> ThreadState::GCWithSweep, Heap::ForcedGC);
> 
>     EXPECT_EQ(0u, map.size());
> }
> 
> and it works fine.
> 
> 
> Presumably this means that I'm wrong about where the cycle is.
> However, if I switch from storing the real callback to storing a fake callback
> which doesn't contain a reference to anything, then the leak goes away.
> 
> Are there any tools available that I could use to try to figure out where the
> cycle is coming from?

I hope we have a nice tool for it :)

> If my understanding is correct, no matter what the ScrollStateCallback has a
> reference to, as long as it's only referenced by the map in which it has a
> WeakMember key, it shouldn't be able to cause leaks, right?

Correct. The ephemeron will realize the lifetime you want to realize.

[tdresser, 2015-07-31 15:27:17]

On 2015/07/31 15:01:25, haraken wrote:
> On 2015/07/31 14:47:11, tdresser wrote:
> > On 2015/07/30 14:58:08, haraken wrote:
> > > On 2015/07/30 14:44:50, tdresser wrote:
> > > > On 2015/07/30 14:32:36, haraken wrote:
> > > > > On 2015/07/30 14:25:16, tdresser wrote:
> > > > > > > Just want to confirm, but our problem boils down to the following
> > > problem,
> > > > > > > right?
> > > > > > > 
> > > > > > > - The Element has a strong reference to the ScrollCallback.
> > > > > > The Element does not necessarily have a reference to the
> ScrollCallback.
> > > We
> > > > > need
> > > > > > some way to get the ScrollCallback for a given Element, and we need
to
> > > keep
> > > > > the
> > > > > > ScrollCallback alive for as long as the Element is alive. The
easiest
> > way
> > > to
> > > > > > achieve this would be to give the Element a strong reference to the
> > > > > > ScrollCallback.
> > > > > > 
> > > > > > > - The ScrollCallback has a strong reference to the Element.
> > > > > > I think this is only true sometimes. If the script author writes a
> > > callback
> > > > > that
> > > > > > doesn't store a reference to the Element, then I don't think we get
> this
> > > > > > reference. This will be true the vast majority of the time though.
> > > > > > 
> > > > > > > - How can we handle the bi-directional strong references?
> > > > > > Although bi-directional strong references may not be the only way to
> > solve
> > > > > this
> > > > > > problem, it may be the most straight forward. If we could handle the
> > > > > > bi-directional strong references, I think we could find a relatively
> > > > straight
> > > > > > forward solution.
> > > > > 
> > > > > So, conceptually the Element and the ScrollCallback can have
> > bi-directional
> > > > > strong references. The problem is how to implement the bi-directional
> > strong
> > > > > references in a non-leaky way.
> > > > > 
> > > > > > > - Replacing the map with ElementRareData is not helpful at all.
> > > > > > If my understanding is correct, with Oilpan on, using the map
results
> in
> > > > > leaks,
> > > > > > whereas using ElementRareData doesn't. With Oilpan off, both will
> leak.
> > > > > 
> > > > > With oilpan enabled, the map won't leak if you use
> > > > > PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> > > > > 
> > > > > Actually, this problem is hard to fix without having oilpan. Given
that
> > this
> > > > is
> > > > > just an experimental feature and that it's rare the ScrollCalback has
a
> > > strong
> > > > > reference to the Element, would it be an option to accept the leak?
> We'll
> > be
> > > > > able to ship Oilpan by the time you ship the ScrollCallback.
> > > > 
> > > > I think we can accept temporarily leaking in the non-oilpan case when
> behind
> > a
> > > > flag. In the event that we do want to ship scroll customization before
> > oilpan
> > > > ships, we can revisit this.
> > > 
> > > Sounds reasonable to me. (Actually we have the same problem in other
> (already
> > > shipped) places as well :)
> > > 
> > > > Doesn't that map still leak?
> > > > 
> > > > The cycle is disguised a bit, but we still have 
> > > > PersistentMap => ScrollCallback => Element,
> > > > and the only way that the ScrollCallback will be collected is if the
> Element
> > > > goes away, so we effectively also have:
> > > > Element => ScrollCallback.
> > > 
> > > PersistentMap<Member<Element>, Member<ScrollCallback>> will cause the
> problem
> > > you described, but PersistentMap<WeakMember<Element>,
> Member<ScrollCallback>>
> > > won't cause the problem. The PersistentMap doesn't visit the Element and
the
> > > ScrollCallback unless the Element is reachable in another way. (This is
the
> > > mechanism called "ephemeron".)
> > 
> > Interesting.
> > 
> > As far as I can tell, that's exactly what I did in the patch above, but it
> still
> > leaks even with oilpan on.
> > The callbacks are stored in a HeapHashMap<RawPtrWillBeWeakMember<Element>,
> > Member<ScrollStateCallback>>.
> > 
> > I wrote a quick test of ephemeron to verify that my understanding is
correct:
> > TEST(HeapTest, EphemeronVerification)
> > {
> >     typedef HeapHashSet<Member<IntWrapper>> Set;
> >     typedef PersistentHeapHashMap<WeakMember<IntWrapper>, Member<Set>> Map;
> > 
> >     Map map;
> > 
> >     IntWrapper* i = IntWrapper::create(1);
> >     Set* set = new Set();
> >     set->add(i);
> >     map.add(i, set);
> > 
> >     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> > ThreadState::GCWithSweep, Heap::ForcedGC);
> > 
> >     EXPECT_EQ(0u, map.size());
> > }
> > 
> > and it works fine.
> > 
> > 
> > Presumably this means that I'm wrong about where the cycle is.
> > However, if I switch from storing the real callback to storing a fake
callback
> > which doesn't contain a reference to anything, then the leak goes away.
> > 
> > Are there any tools available that I could use to try to figure out where
the
> > cycle is coming from?
> 
> I hope we have a nice tool for it :)
> 
> > If my understanding is correct, no matter what the ScrollStateCallback has a
> > reference to, as long as it's only referenced by the map in which it has a
> > WeakMember key, it shouldn't be able to cause leaks, right?
> 
> Correct. The ephemeron will realize the lifetime you want to realize.

Could the problem be that we've got a cycle which is partially in blink gc, and
partially in v8 gc?

Roughly, 
ScrollStateCallback (is implemented by) V8ScrollStateCallback => V8Element =>
Element.
Element keeps ScrollStateCallback alive due to the map.
Will this cycle be gc'ed even though it passes between the blink and v8 gc's?

[haraken, 2015-07-31 15:49:17]

On 2015/07/31 15:27:17, tdresser wrote:
> On 2015/07/31 15:01:25, haraken wrote:
> > On 2015/07/31 14:47:11, tdresser wrote:
> > > On 2015/07/30 14:58:08, haraken wrote:
> > > > On 2015/07/30 14:44:50, tdresser wrote:
> > > > > On 2015/07/30 14:32:36, haraken wrote:
> > > > > > On 2015/07/30 14:25:16, tdresser wrote:
> > > > > > > > Just want to confirm, but our problem boils down to the
following
> > > > problem,
> > > > > > > > right?
> > > > > > > > 
> > > > > > > > - The Element has a strong reference to the ScrollCallback.
> > > > > > > The Element does not necessarily have a reference to the
> > ScrollCallback.
> > > > We
> > > > > > need
> > > > > > > some way to get the ScrollCallback for a given Element, and we
need
> to
> > > > keep
> > > > > > the
> > > > > > > ScrollCallback alive for as long as the Element is alive. The
> easiest
> > > way
> > > > to
> > > > > > > achieve this would be to give the Element a strong reference to
the
> > > > > > > ScrollCallback.
> > > > > > > 
> > > > > > > > - The ScrollCallback has a strong reference to the Element.
> > > > > > > I think this is only true sometimes. If the script author writes a
> > > > callback
> > > > > > that
> > > > > > > doesn't store a reference to the Element, then I don't think we
get
> > this
> > > > > > > reference. This will be true the vast majority of the time though.
> > > > > > > 
> > > > > > > > - How can we handle the bi-directional strong references?
> > > > > > > Although bi-directional strong references may not be the only way
to
> > > solve
> > > > > > this
> > > > > > > problem, it may be the most straight forward. If we could handle
the
> > > > > > > bi-directional strong references, I think we could find a
relatively
> > > > > straight
> > > > > > > forward solution.
> > > > > > 
> > > > > > So, conceptually the Element and the ScrollCallback can have
> > > bi-directional
> > > > > > strong references. The problem is how to implement the
bi-directional
> > > strong
> > > > > > references in a non-leaky way.
> > > > > > 
> > > > > > > > - Replacing the map with ElementRareData is not helpful at all.
> > > > > > > If my understanding is correct, with Oilpan on, using the map
> results
> > in
> > > > > > leaks,
> > > > > > > whereas using ElementRareData doesn't. With Oilpan off, both will
> > leak.
> > > > > > 
> > > > > > With oilpan enabled, the map won't leak if you use
> > > > > > PersistentHeapMap<WeakMember<Element>, Member<ScrollCallback>>.
> > > > > > 
> > > > > > Actually, this problem is hard to fix without having oilpan. Given
> that
> > > this
> > > > > is
> > > > > > just an experimental feature and that it's rare the ScrollCalback
has
> a
> > > > strong
> > > > > > reference to the Element, would it be an option to accept the leak?
> > We'll
> > > be
> > > > > > able to ship Oilpan by the time you ship the ScrollCallback.
> > > > > 
> > > > > I think we can accept temporarily leaking in the non-oilpan case when
> > behind
> > > a
> > > > > flag. In the event that we do want to ship scroll customization before
> > > oilpan
> > > > > ships, we can revisit this.
> > > > 
> > > > Sounds reasonable to me. (Actually we have the same problem in other
> > (already
> > > > shipped) places as well :)
> > > > 
> > > > > Doesn't that map still leak?
> > > > > 
> > > > > The cycle is disguised a bit, but we still have 
> > > > > PersistentMap => ScrollCallback => Element,
> > > > > and the only way that the ScrollCallback will be collected is if the
> > Element
> > > > > goes away, so we effectively also have:
> > > > > Element => ScrollCallback.
> > > > 
> > > > PersistentMap<Member<Element>, Member<ScrollCallback>> will cause the
> > problem
> > > > you described, but PersistentMap<WeakMember<Element>,
> > Member<ScrollCallback>>
> > > > won't cause the problem. The PersistentMap doesn't visit the Element and
> the
> > > > ScrollCallback unless the Element is reachable in another way. (This is
> the
> > > > mechanism called "ephemeron".)
> > > 
> > > Interesting.
> > > 
> > > As far as I can tell, that's exactly what I did in the patch above, but it
> > still
> > > leaks even with oilpan on.
> > > The callbacks are stored in a HeapHashMap<RawPtrWillBeWeakMember<Element>,
> > > Member<ScrollStateCallback>>.
> > > 
> > > I wrote a quick test of ephemeron to verify that my understanding is
> correct:
> > > TEST(HeapTest, EphemeronVerification)
> > > {
> > >     typedef HeapHashSet<Member<IntWrapper>> Set;
> > >     typedef PersistentHeapHashMap<WeakMember<IntWrapper>, Member<Set>>
Map;
> > > 
> > >     Map map;
> > > 
> > >     IntWrapper* i = IntWrapper::create(1);
> > >     Set* set = new Set();
> > >     set->add(i);
> > >     map.add(i, set);
> > > 
> > >     Heap::collectGarbage(ThreadState::NoHeapPointersOnStack,
> > > ThreadState::GCWithSweep, Heap::ForcedGC);
> > > 
> > >     EXPECT_EQ(0u, map.size());
> > > }
> > > 
> > > and it works fine.
> > > 
> > > 
> > > Presumably this means that I'm wrong about where the cycle is.
> > > However, if I switch from storing the real callback to storing a fake
> callback
> > > which doesn't contain a reference to anything, then the leak goes away.
> > > 
> > > Are there any tools available that I could use to try to figure out where
> the
> > > cycle is coming from?
> > 
> > I hope we have a nice tool for it :)
> > 
> > > If my understanding is correct, no matter what the ScrollStateCallback has
a
> > > reference to, as long as it's only referenced by the map in which it has a
> > > WeakMember key, it shouldn't be able to cause leaks, right?
> > 
> > Correct. The ephemeron will realize the lifetime you want to realize.
> 
> Could the problem be that we've got a cycle which is partially in blink gc,
and
> partially in v8 gc?
> 
> Roughly, 
> ScrollStateCallback (is implemented by) V8ScrollStateCallback => V8Element =>
> Element.
> Element keeps ScrollStateCallback alive due to the map.

The map doesn't keep the ScrollStateCallback alive as long as you use
WeakMember<Element>.

[tdresser, 2015-08-13 13:43:34]

Patchset #2 (id:40001) has been deleted

[tdresser, 2015-08-13 15:40:10]

Patchset #3 (id:80001) has been deleted

[tdresser, 2015-08-13 17:20:23]

tdresser@chromium.org changed reviewers:
+ haraken@chromium.org

[tdresser, 2015-08-13 17:20:23]

What do you think of this approach?

Event listeners require being explicitly removed as well. Seeing as we were
previously only leaking when we navigate, an explicit cleanup step seems
reasonable.

[haraken, 2015-08-13 23:42:05]

The approach looks good to me.

https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
Source/core/dom/Document.cpp:2696:
Element::removeScrollCustomizationCallbacksForDocument(this);

Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't it be
enough to let each Element's destructor remove the callback with
removeCallbacksForElement?

[tdresser, 2015-08-14 13:07:18]

On 2015/08/13 23:42:05, haraken wrote:
> The approach looks good to me.
> 
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> Source/core/dom/Document.cpp:2696:
> Element::removeScrollCustomizationCallbacksForDocument(this);
> 
> Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't it be
> enough to let each Element's destructor remove the callback with
> removeCallbacksForElement?

There's a cycle (which is partially in the V8 garbage collector, and partially
in the oilpan garbage collector) between the Element and the Callback. We need
to break the cycle to enable the Elements to be collected.

We can't break the cycle in the Element's destructor, because the Element's
destructor won't be called until the cycle is broken.

[tdresser, 2015-08-19 12:35:16]

On 2015/08/14 13:07:18, tdresser wrote:
> On 2015/08/13 23:42:05, haraken wrote:
> > The approach looks good to me.
> > 
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > File Source/core/dom/Document.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > Source/core/dom/Document.cpp:2696:
> > Element::removeScrollCustomizationCallbacksForDocument(this);
> > 
> > Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't it be
> > enough to let each Element's destructor remove the callback with
> > removeCallbacksForElement?
> 
> There's a cycle (which is partially in the V8 garbage collector, and partially
> in the oilpan garbage collector) between the Element and the Callback. We need
> to break the cycle to enable the Elements to be collected.
> 
> We can't break the cycle in the Element's destructor, because the Element's
> destructor won't be called until the cycle is broken.

haraken@, just confirming, you're fine with landing this approach?

[haraken, 2015-08-19 14:13:20]

On 2015/08/19 12:35:16, tdresser wrote:
> On 2015/08/14 13:07:18, tdresser wrote:
> > On 2015/08/13 23:42:05, haraken wrote:
> > > The approach looks good to me.
> > > 
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > File Source/core/dom/Document.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > Source/core/dom/Document.cpp:2696:
> > > Element::removeScrollCustomizationCallbacksForDocument(this);
> > > 
> > > Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't it
be
> > > enough to let each Element's destructor remove the callback with
> > > removeCallbacksForElement?
> > 
> > There's a cycle (which is partially in the V8 garbage collector, and
partially
> > in the oilpan garbage collector) between the Element and the Callback. We
need
> > to break the cycle to enable the Elements to be collected.
> > 
> > We can't break the cycle in the Element's destructor, because the Element's
> > destructor won't be called until the cycle is broken.
> 
> haraken@, just confirming, you're fine with landing this approach?

If you want to explicitly remove callbacks, you should do it in
Document::detach(), not in dispatchUnloadEvent().

That said, I'm not yet convinced why we need to explicitly remove callbacks. Per
the discussion thread with jochen and me, haven't we reached a conclusion that
there is no memory leak (i.e., the reason you observed a leaked document is just
due to GC timings)?

Would you elaborate on where a cycle exists if you don't remove the callbacks in
dispatchUnloadEvent()?

[tdresser, 2015-08-19 17:22:19]

On 2015/08/19 14:13:20, haraken wrote:
> On 2015/08/19 12:35:16, tdresser wrote:
> > On 2015/08/14 13:07:18, tdresser wrote:
> > > On 2015/08/13 23:42:05, haraken wrote:
> > > > The approach looks good to me.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > > File Source/core/dom/Document.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > > Source/core/dom/Document.cpp:2696:
> > > > Element::removeScrollCustomizationCallbacksForDocument(this);
> > > > 
> > > > Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't
it
> be
> > > > enough to let each Element's destructor remove the callback with
> > > > removeCallbacksForElement?
> > > 
> > > There's a cycle (which is partially in the V8 garbage collector, and
> partially
> > > in the oilpan garbage collector) between the Element and the Callback. We
> need
> > > to break the cycle to enable the Elements to be collected.
> > > 
> > > We can't break the cycle in the Element's destructor, because the
Element's
> > > destructor won't be called until the cycle is broken.
> > 
> > haraken@, just confirming, you're fine with landing this approach?
> 
> If you want to explicitly remove callbacks, you should do it in
> Document::detach(), not in dispatchUnloadEvent().
> 
> That said, I'm not yet convinced why we need to explicitly remove callbacks.
Per
> the discussion thread with jochen and me, haven't we reached a conclusion that
> there is no memory leak (i.e., the reason you observed a leaked document is
just
> due to GC timings)?
> 
> Would you elaborate on where a cycle exists if you don't remove the callbacks
in
> dispatchUnloadEvent()?

Removing the callbacks in Document::detach sounds good to me - I debated back
and
forth between the two.

I verified the leak isn't due to GC timings, however, the leak only occurs when
window has a reference to either the callback or the element.

The leak as I understand it is:
window -(v8)> elementWrapper -(oilpan?)> element -(oilpan)> callback -(v8)>
v8::Function -(v8)> window

[haraken, 2015-08-20 00:47:31]

On 2015/08/19 17:22:19, tdresser wrote:
> On 2015/08/19 14:13:20, haraken wrote:
> > On 2015/08/19 12:35:16, tdresser wrote:
> > > On 2015/08/14 13:07:18, tdresser wrote:
> > > > On 2015/08/13 23:42:05, haraken wrote:
> > > > > The approach looks good to me.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > > > File Source/core/dom/Document.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1236913004/diff/100001/Source/core/dom/Docume...
> > > > > Source/core/dom/Document.cpp:2696:
> > > > > Element::removeScrollCustomizationCallbacksForDocument(this);
> > > > > 
> > > > > Why do we need to remove callbacks in dispatchUnloadEvents()? Wouldn't
> it
> > be
> > > > > enough to let each Element's destructor remove the callback with
> > > > > removeCallbacksForElement?
> > > > 
> > > > There's a cycle (which is partially in the V8 garbage collector, and
> > partially
> > > > in the oilpan garbage collector) between the Element and the Callback.
We
> > need
> > > > to break the cycle to enable the Elements to be collected.
> > > > 
> > > > We can't break the cycle in the Element's destructor, because the
> Element's
> > > > destructor won't be called until the cycle is broken.
> > > 
> > > haraken@, just confirming, you're fine with landing this approach?
> > 
> > If you want to explicitly remove callbacks, you should do it in
> > Document::detach(), not in dispatchUnloadEvent().
> > 
> > That said, I'm not yet convinced why we need to explicitly remove callbacks.
> Per
> > the discussion thread with jochen and me, haven't we reached a conclusion
that
> > there is no memory leak (i.e., the reason you observed a leaked document is
> just
> > due to GC timings)?
> > 
> > Would you elaborate on where a cycle exists if you don't remove the
callbacks
> in
> > dispatchUnloadEvent()?
> 
> Removing the callbacks in Document::detach sounds good to me - I debated back
> and
> forth between the two.
> 
> I verified the leak isn't due to GC timings, however, the leak only occurs
when
> window has a reference to either the callback or the element.
> 
> The leak as I understand it is:
> window -(v8)> elementWrapper -(oilpan?)> element -(oilpan)> callback -(v8)>
> v8::Function -(v8)> window

The "element -(oilpan)-> callback" reference is weak in oilpan and a raw pointer
in non-oilpan. Either way it won't cause leaks, will it?

[tdresser, 2015-08-26 15:21:40]

Thanks so much for sticking with this patch.

I was misunderstanding the behavior of HeapHashMap<WeakMember<T>, U>, and hadn't
made the map value a WeakMember. Sorry for the trouble.

This passes the tests without leaking, with and without oilpan.

Does this look correct?

[haraken, 2015-08-27 00:49:41]

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:498: void
Element::removeScrollCustomizationCallbacksForDocument(Document* document)

Can we remove this?

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:547: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformAfterNativeScroll)

Shouldn't this be "Before"?

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:550: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformBeforeNativeScroll)

Shouldn't this be "After"?

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:612: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformAfterNativeScroll)

Ditto.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:615: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformBeforeNativeScroll)

Ditto.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.h:382: static void
removeScrollCustomizationCallbacksForDocument(Document*);

Can we remove this now?

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:39: void
ScrollCustomizationCallbacks::removeAllCallbacksForDocument(Document* document)

Can we remove this?

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollCustomizationCallbacks.h (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.h:34: void
removeAllCallbacksForDocument(Document*);

Can we remove this?

[tdresser, 2015-08-27 20:46:25]

Patchset #5 (id:140001) has been deleted

[tdresser, 2015-08-27 20:48:40]

Thanks for catching that cruft.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:498: void
Element::removeScrollCustomizationCallbacksForDocument(Document* document)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Can we remove this?

Done.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:547: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformAfterNativeScroll)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Shouldn't this be "Before"?

This is correct, but confusing.
I rearranged it to be more clear.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:550: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformBeforeNativeScroll)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Shouldn't this be "After"?

Same as above.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:612: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformAfterNativeScroll)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Ditto.

Same as above.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:615: if (callback->nativeScrollBehavior() ==
NativeScrollBehavior::PerformBeforeNativeScroll)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Ditto.

Same as above.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/dom/Elemen...
Source/core/dom/Element.h:382: static void
removeScrollCustomizationCallbacksForDocument(Document*);
On 2015/08/27 00:49:40, haraken wrote:
> 
> Can we remove this now?

Done.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:39: void
ScrollCustomizationCallbacks::removeAllCallbacksForDocument(Document* document)
On 2015/08/27 00:49:40, haraken wrote:
> 
> Can we remove this?

Done.

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollCustomizationCallbacks.h (right):

https://codereview.chromium.org/1236913004/diff/120001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.h:34: void
removeAllCallbacksForDocument(Document*);
On 2015/08/27 00:49:40, haraken wrote:
> 
> Can we remove this?

Done.

[haraken, 2015-08-28 00:14:13]

LGTM!

I want to have Rick take a final look from the perspective of the design of
scroll customization.

https://codereview.chromium.org/1236913004/diff/160001/Source/core/page/scrol...
File Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp (right):

https://codereview.chromium.org/1236913004/diff/160001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:15:
m_distributeScrollCallbacks.set(element, scrollStateCallback);

If multiple callbacks are installed on an element, the old callback will be
overwritten. Is it expected?

https://codereview.chromium.org/1236913004/diff/160001/Source/core/page/scrol...
Source/core/page/scrolling/ScrollCustomizationCallbacks.cpp:28:
m_applyScrollCallbacks.set(element, scrollStateCallback);

Ditto.

[tdresser, 2015-08-28 12:18:01]

tdresser@chromium.org changed reviewers:
+ rbyers@chromium.org

[tdresser, 2015-08-28 12:18:02]

We're fine landing (behind the flag) with this behavior for now, but we
definitely will want more flexibility in the future.

Thanks!

+rbyers for scroll customization logic.

[Rick Byers, 2015-08-30 20:57:50]

Nice, LGTM!

Thanks tdresser@ and haraken@ for all the patience in coming up with something
that'll work.  This is really important ground work for the compositor-worker
side that's required for snap points.

https://codereview.chromium.org/1236913004/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1236913004/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:181:
scrollCustomizationCallbacks().removeCallbacksForElement(this);
So in the Oilpan case I assume the weak collections automatically remove entries
that get collected, right?  Cool!

[haraken, 2015-08-30 22:21:38]

https://codereview.chromium.org/1236913004/diff/160001/Source/core/dom/Elemen...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/1236913004/diff/160001/Source/core/dom/Elemen...
Source/core/dom/Element.cpp:181:
scrollCustomizationCallbacks().removeCallbacksForElement(this);
On 2015/08/30 20:57:50, Rick Byers (Out until 9-4) wrote:
> So in the Oilpan case I assume the weak collections automatically remove
entries
> that get collected, right?  Cool!

Right :)

[tdresser, 2015-08-31 14:01:56]

Sorry, this solution doesn't work, as nothing has a strong reference to the
callback. I've added a gc to the layout tests to ensure the solution we go with
doesn't have this issue.

> The "element -(oilpan)-> callback" reference is weak in > oilpan and a raw
pointer
> in non-oilpan. Either way it won't cause leaks, will it?

The reference from the element to the callback has to be strong, because we need
to keep the callback alive for as long as the element is alive.

Thus the cycle (a bit overly simplified) is:
window -> element -> callback -> window

The first reference comes from user JS, and thus must be strong.
The second reference must be strong since it's the only thing keeping the
callback alive.
The third reference is strong due to the nature of how a v8::Function keeps a
reference to it's context.

Breaking the cycle in Document::detach() works fine though.

haraken@, are you happy with this approach?

[haraken, 2015-08-31 23:49:10]

On 2015/08/31 14:01:56, tdresser wrote:
> Sorry, this solution doesn't work, as nothing has a strong reference to the
> callback. I've added a gc to the layout tests to ensure the solution we go
with
> doesn't have this issue.
> 
> > The "element -(oilpan)-> callback" reference is weak in > oilpan and a raw
> pointer
> > in non-oilpan. Either way it won't cause leaks, will it?
> 
> The reference from the element to the callback has to be strong, because we
need
> to keep the callback alive for as long as the element is alive.
> 
> Thus the cycle (a bit overly simplified) is:
> window -> element -> callback -> window
> 
> The first reference comes from user JS, and thus must be strong.
> The second reference must be strong since it's the only thing keeping the
> callback alive.
> The third reference is strong due to the nature of how a v8::Function keeps a
> reference to it's context.
> 
> Breaking the cycle in Document::detach() works fine though.
> 
> haraken@, are you happy with this approach?

No. It is wrong to do this kind of lifetime management in Document::detach().

What you need is to keep the ScrollCallback alive while the Element's wrapper is
alive (or more accurately, while any wrapper in the object group which the
Element belongs to is alive). You'll need to use a hidden value or
[SetWrapperReferenceFrom] to represent the wrapper reachability.

I recommend you choose either of the following options:

a) Represent the wrapper reachability with a hidden value or [SetReferenceFrom].
I'm afraid that this will require a bunch of custom bindings.

b) Wait until we implement the traceWrapper
(https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP...).
Then you can easily represent the wrapper reachability. However, it won't happen
until Q4.

c) Just use a strong reference from the Element to the ScrollCallback and accept
the memory leak.

I'd prefer c). Given that the ScrollCallback is not yet speced and thus the
design can change in the future, I don't think it's worth spending a lot of time
or introducing complexity for it.

[haraken, 2015-08-31 23:49:58]

On 2015/08/31 23:49:10, haraken wrote:
> On 2015/08/31 14:01:56, tdresser wrote:
> > Sorry, this solution doesn't work, as nothing has a strong reference to the
> > callback. I've added a gc to the layout tests to ensure the solution we go
> with
> > doesn't have this issue.
> > 
> > > The "element -(oilpan)-> callback" reference is weak in > oilpan and a raw
> > pointer
> > > in non-oilpan. Either way it won't cause leaks, will it?
> > 
> > The reference from the element to the callback has to be strong, because we
> need
> > to keep the callback alive for as long as the element is alive.
> > 
> > Thus the cycle (a bit overly simplified) is:
> > window -> element -> callback -> window
> > 
> > The first reference comes from user JS, and thus must be strong.
> > The second reference must be strong since it's the only thing keeping the
> > callback alive.
> > The third reference is strong due to the nature of how a v8::Function keeps
a
> > reference to it's context.
> > 
> > Breaking the cycle in Document::detach() works fine though.
> > 
> > haraken@, are you happy with this approach?
> 
> No. It is wrong to do this kind of lifetime management in Document::detach().
> 
> What you need is to keep the ScrollCallback alive while the Element's wrapper
is
> alive (or more accurately, while any wrapper in the object group which the
> Element belongs to is alive). You'll need to use a hidden value or
> [SetWrapperReferenceFrom] to represent the wrapper reachability.
> 
> I recommend you choose either of the following options:
> 
> a) Represent the wrapper reachability with a hidden value or
[SetReferenceFrom].
> I'm afraid that this will require a bunch of custom bindings.
> 
> b) Wait until we implement the traceWrapper
>
(https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP...).
> Then you can easily represent the wrapper reachability. However, it won't
happen
> until Q4.
> 
> c) Just use a strong reference from the Element to the ScrollCallback and
accept
> the memory leak.
> 
> I'd prefer c). Given that the ScrollCallback is not yet speced and thus the
> design can change in the future, I don't think it's worth spending a lot of
time
> or introducing complexity for it.

If you want to go with a), look at how MutationCallback is implemented.

[tdresser, 2015-09-01 13:23:54]

> > It is wrong to do this kind of lifetime management in Document::detach().

Some of the lifetime management of EventHandler is performed in
Document::dispatchUnloadEvents().
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

You stated above:
"If you want to explicitly remove callbacks, you should do it in
Document::detach(), not in dispatchUnloadEvent()."

Is this just done for EventHandler for historical reasons, or is there some
reason it's okay for EventHandler to manage memory this way, but not for scroll
customization handlers?


Although we aren't planning to expose scroll customization to the web anytime
soon, we are hoping to ship snap points on top of scroll customization
(https://docs.google.com/document/d/1xbc9W_Ac-ve0gE_ppT9y8aydVJJ7NXUL-9MVq0yPh...).
We may end up shipping snap points on top of scroll customization with this API,
so we may need a solution here that doesn't leak.

[haraken, 2015-09-01 13:41:37]

On 2015/09/01 13:23:54, tdresser wrote:
> > > It is wrong to do this kind of lifetime management in Document::detach().
> 
> Some of the lifetime management of EventHandler is performed in
> Document::dispatchUnloadEvents().
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

EventHandlers are a totally different thing. What you have to solve here is how
to manage wrapper reachability.


> You stated above:
> "If you want to explicitly remove callbacks, you should do it in
> Document::detach(), not in dispatchUnloadEvent()."
> 
> Is this just done for EventHandler for historical reasons, or is there some
> reason it's okay for EventHandler to manage memory this way, but not for
scroll
> customization handlers?

I said that in a sense that "if you really have to remove the callbacks when the
document is going to shut down, that should be at Document::detach not in
Document::dispatchUnloadEvent". As I said just after the comment, I'm never
proposing to do that.

As mentioned above, what you have to make sure is that "keep the ScrollCallback
alive as long as the Element wrapper is alive". Document::detach is not related
to the concept at all. Breaking a cycle in Document::detach may work as expected
in most cases, but it is just a hack to make things look working.

For example, what happens for an Element that has been removed from a Document
but has a ScrollCallback? Who removes the ScrollCallback from the Element? Note
that element->document() can change over time (can be null).


> Although we aren't planning to expose scroll customization to the web anytime
> soon, we are hoping to ship snap points on top of scroll customization
>
(https://docs.google.com/document/d/1xbc9W_Ac-ve0gE_ppT9y8aydVJJ7NXUL-9MVq0yPh...).
> We may end up shipping snap points on top of scroll customization with this
API,
> so we may need a solution here that doesn't leak.

[tdresser, 2015-09-01 14:17:28]

> I said that in a sense that "if you really have to remove the callbacks when
the
> document is going to shut down, that should be at Document::detach not in
> Document::dispatchUnloadEvent". As I said just after the comment, I'm never
> proposing to do that.

Thanks for clarifying.

> For example, what happens for an Element that has been removed from a Document
> but has a ScrollCallback? Who removes the ScrollCallback from the Element?
Note
> that element->document() can change over time (can be null).

Understood, that's a good example.


So if I understand correctly, the options are:
1. Land this with the leak, and block shipping snap points on traceWrapper.
2. Uncouple snap points from scroll customization's javascript interface.
3. Write some custom bindings logic.

I think the best path forward for now is to land this with the leak, and in the
case that the only thing holding back snap points is this memory leak, we can
investigate the custom bindings solution.

[tdresser, 2015-09-01 14:19:16]

kouhei@, is there any way to add a leak suppression for a testharness.js test?

The only tests I've seen that leak have been written with doLeakTest, which
requires being a js-test.

[haraken, 2015-09-01 14:34:36]

On 2015/09/01 14:17:28, tdresser wrote:
> > I said that in a sense that "if you really have to remove the callbacks when
> the
> > document is going to shut down, that should be at Document::detach not in
> > Document::dispatchUnloadEvent". As I said just after the comment, I'm never
> > proposing to do that.
> 
> Thanks for clarifying.
> 
> > For example, what happens for an Element that has been removed from a
Document
> > but has a ScrollCallback? Who removes the ScrollCallback from the Element?
> Note
> > that element->document() can change over time (can be null).
> 
> Understood, that's a good example.
> 
> 
> So if I understand correctly, the options are:
> 1. Land this with the leak, and block shipping snap points on traceWrapper.
> 2. Uncouple snap points from scroll customization's javascript interface.
> 3. Write some custom bindings logic.
> 
> I think the best path forward for now is to land this with the leak, and in
the
> case that the only thing holding back snap points is this memory leak, we can
> investigate the custom bindings solution.

Just a question: How likely will we hit the leak? The leak will happen only when
the contents of the ScrollCallback has a reference back to the Element. How
likely will it happen? Only in artificial cases?

[tdresser, 2015-09-01 14:40:09]

On 2015/09/01 14:34:36, haraken wrote:
> On 2015/09/01 14:17:28, tdresser wrote:
> > > I said that in a sense that "if you really have to remove the callbacks
when
> > the
> > > document is going to shut down, that should be at Document::detach not in
> > > Document::dispatchUnloadEvent". As I said just after the comment, I'm
never
> > > proposing to do that.
> > 
> > Thanks for clarifying.
> > 
> > > For example, what happens for an Element that has been removed from a
> Document
> > > but has a ScrollCallback? Who removes the ScrollCallback from the Element?
> > Note
> > > that element->document() can change over time (can be null).
> > 
> > Understood, that's a good example.
> > 
> > 
> > So if I understand correctly, the options are:
> > 1. Land this with the leak, and block shipping snap points on traceWrapper.
> > 2. Uncouple snap points from scroll customization's javascript interface.
> > 3. Write some custom bindings logic.
> > 
> > I think the best path forward for now is to land this with the leak, and in
> the
> > case that the only thing holding back snap points is this memory leak, we
can
> > investigate the custom bindings solution.
> 
> Just a question: How likely will we hit the leak? The leak will happen only
when
> the contents of the ScrollCallback has a reference back to the Element. How
> likely will it happen? Only in artificial cases?

My understanding is that the ScrollCallback has a v8::Function, which keeps
track of some v8 context which includes the window object. If the window object
references the element, then we'll leak.

I think this is pretty likely - if a developer gets a reference to the element
outside of any function, then the window will end up with a reference to the
element.

[haraken, 2015-09-01 14:43:57]

On 2015/09/01 14:40:09, tdresser wrote:
> On 2015/09/01 14:34:36, haraken wrote:
> > On 2015/09/01 14:17:28, tdresser wrote:
> > > > I said that in a sense that "if you really have to remove the callbacks
> when
> > > the
> > > > document is going to shut down, that should be at Document::detach not
in
> > > > Document::dispatchUnloadEvent". As I said just after the comment, I'm
> never
> > > > proposing to do that.
> > > 
> > > Thanks for clarifying.
> > > 
> > > > For example, what happens for an Element that has been removed from a
> > Document
> > > > but has a ScrollCallback? Who removes the ScrollCallback from the
Element?
> > > Note
> > > > that element->document() can change over time (can be null).
> > > 
> > > Understood, that's a good example.
> > > 
> > > 
> > > So if I understand correctly, the options are:
> > > 1. Land this with the leak, and block shipping snap points on
traceWrapper.
> > > 2. Uncouple snap points from scroll customization's javascript interface.
> > > 3. Write some custom bindings logic.
> > > 
> > > I think the best path forward for now is to land this with the leak, and
in
> > the
> > > case that the only thing holding back snap points is this memory leak, we
> can
> > > investigate the custom bindings solution.
> > 
> > Just a question: How likely will we hit the leak? The leak will happen only
> when
> > the contents of the ScrollCallback has a reference back to the Element. How
> > likely will it happen? Only in artificial cases?
> 
> My understanding is that the ScrollCallback has a v8::Function, which keeps
> track of some v8 context which includes the window object. If the window
object
> references the element, then we'll leak.
> 
> I think this is pretty likely - if a developer gets a reference to the element
> outside of any function, then the window will end up with a reference to the
> element.

Yeah, it's likely...

MutationCallback and PerformanceObserver are hitting exactly the same problem.
They're solving the leak with custom bindings.

[kouhei (in TOK), 2015-09-02 22:05:16]

> kouhei@, is there any way to add a leak suppression for a testharness.js test?
If the leak appears with "run-webkit-tests --enable-leak-detection", you can add
an entry to LeakExpectations file:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[tdresser, 2015-09-03 19:14:06]

The CQ bit was checked by tdresser@chromium.org

[tdresser, 2015-09-03 19:14:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1236913004/#ps220001
(title: "Add to leak expectations.")

[commit-bot: I haz the power, 2015-09-03 19:14:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1236913004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1236913004/220001

[commit-bot: I haz the power, 2015-09-03 19:19:33]

Committed patchset #8 (id:220001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201736

[haraken, 2015-09-04 00:17:49]

On 2015/09/03 19:19:33, commit-bot: I haz the power wrote:
> Committed patchset #8 (id:220001) as
> https://src.chromium.org/viewvc/blink?view=rev&revision=201736

This caused leaks:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak

[tdresser1, 2015-09-04 00:42:43]

On 2015/09/04 00:17:49, haraken wrote:
> On 2015/09/03 19:19:33, commit-bot: I haz the power wrote:
> > Committed patchset #8 (id:220001) as
> > https://src.chromium.org/viewvc/blink?view=rev&revision=201736
> 
> This caused leaks:
> http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak

I'll add the suppression first thing tomorrow morning.

[tdresser1, 2015-09-04 01:47:47]

On 2015/09/04 00:42:43, tdresser-g wrote:
> On 2015/09/04 00:17:49, haraken wrote:
> > On 2015/09/03 19:19:33, commit-bot: I haz the power wrote:
> > > Committed patchset #8 (id:220001) as
> > > https://src.chromium.org/viewvc/blink?view=rev&revision=201736
> > 
> > This caused leaks:
> > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak
> 
> I'll add the suppression first thing tomorrow morning.

Assuming this is the same leak as the one I've already suppressed.

[tdresser, 2015-09-04 12:51:52]

On 2015/09/04 01:47:47, tdresser-g wrote:
> On 2015/09/04 00:42:43, tdresser-g wrote:
> > On 2015/09/04 00:17:49, haraken wrote:
> > > On 2015/09/03 19:19:33, commit-bot: I haz the power wrote:
> > > > Committed patchset #8 (id:220001) as
> > > > https://src.chromium.org/viewvc/blink?view=rev&revision=201736
> > > 
> > > This caused leaks:
> > > http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak
> > 
> > I'll add the suppression first thing tomorrow morning.
> 
> Assuming this is the same leak as the one I've already suppressed.

Suppression added here:
https://codereview.chromium.org/1326083003/

I've verified this leak is expected.