[wrapper-tracing] Fix EventTarget with LazyEventListeners

[wrapper-tracing] Fix EventTarget with LazyEventListeners

LazyEventListener_s have their listener object set at some later point in time.
We need to trace the EventListener objects in any case and add a write barrier
for the ScopedPersistent.

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org

[Michael Lippautz, 2016-10-29 14:14:46]

PTAL, needed to get this out of my head.

What I think currently happens (which is wrong):
- We create a V8LazyEventListener without listener object
- The write barrier will not fire
- At some point we will set the listener in V8LazyEventListener
- The verifier triggers because now we should've traced it.

Does that make sense?

[haraken, 2016-10-29 18:58:26]

Maybe should we introduce TraceWrapperV8Persistent?

I'm wondering why it's enough to add write barriers to V8AbstractEventListener's
ScopedPersistent. What about other ScopedPersistents traced by traceWrapper()?

[Michael Lippautz, 2016-10-30 20:55:03]

On 2016/10/29 18:58:26, haraken wrote:
> Maybe should we introduce TraceWrapperV8Persistent?
> 

Possibly, see below.

> I'm wondering why it's enough to add write barriers to
V8AbstractEventListener's
> ScopedPersistent. What about other ScopedPersistents traced by traceWrapper()?

As far as I know, in the usual case ScopedPersistent_s on ScriptWrappable_s do
not change and this might be the only case that lazily sets the reference. Not
sure it's worth to introduce the whole infrastructure (+ back pointers overhead)
to cover this case. I would only introduce the whole machinery if it's really
needed, i.e., we see that many cases actually require it.

Will check what the other uses of ScopedPersistent do on Wednesday.

[haraken, 2016-10-31 10:23:36]

On 2016/10/30 20:55:03, Michael Lippautz wrote:
> On 2016/10/29 18:58:26, haraken wrote:
> > Maybe should we introduce TraceWrapperV8Persistent?
> > 
> 
> Possibly, see below.
> 
> > I'm wondering why it's enough to add write barriers to
> V8AbstractEventListener's
> > ScopedPersistent. What about other ScopedPersistents traced by
traceWrapper()?
> 
> As far as I know, in the usual case ScopedPersistent_s on ScriptWrappable_s do
> not change and this might be the only case that lazily sets the reference. Not
> sure it's worth to introduce the whole infrastructure (+ back pointers
overhead)
> to cover this case. I would only introduce the whole machinery if it's really
> needed, i.e., we see that many cases actually require it.
> 
> Will check what the other uses of ScopedPersistent do on Wednesday.

We will anyway need a new pointer type for ScopedPersistent, because one of the
goals of traceWrapper() is to remove most of the ScopedPersistents from the code
base and thus fix memory leaks that cross the Blink <=> V8 boundary. So I think
it's worth introducing TraceWrapperV8Persistent.

But I'm confused. How are we handling ScopedPersistents when the wrapper tracing
is enabled? Is it traced by traceWrapper() but still using ScopedPersistent?
Then it doesn't make sense because with the wrapper tracing enabled, it doesn't
need to be a v8::Persistent handle at all (and that's the whole point of the
wrapper tracing).

[Michael Lippautz, 2016-10-31 10:51:00]

On 2016/10/31 10:23:36, haraken wrote:
> On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > On 2016/10/29 18:58:26, haraken wrote:
> > > Maybe should we introduce TraceWrapperV8Persistent?
> > > 
> > 
> > Possibly, see below.
> > 
> > > I'm wondering why it's enough to add write barriers to
> > V8AbstractEventListener's
> > > ScopedPersistent. What about other ScopedPersistents traced by
> traceWrapper()?
> > 
> > As far as I know, in the usual case ScopedPersistent_s on ScriptWrappable_s
do
> > not change and this might be the only case that lazily sets the reference.
Not
> > sure it's worth to introduce the whole infrastructure (+ back pointers
> overhead)
> > to cover this case. I would only introduce the whole machinery if it's
really
> > needed, i.e., we see that many cases actually require it.
> > 
> > Will check what the other uses of ScopedPersistent do on Wednesday.
> 
> We will anyway need a new pointer type for ScopedPersistent, because one of
the
> goals of traceWrapper() is to remove most of the ScopedPersistents from the
code
> base and thus fix memory leaks that cross the Blink <=> V8 boundary. So I
think
> it's worth introducing TraceWrapperV8Persistent.
> 

Thanks for the explanation, I see that now.

> But I'm confused. How are we handling ScopedPersistents when the wrapper
tracing
> is enabled? Is it traced by traceWrapper() but still using ScopedPersistent?
> Then it doesn't make sense because with the wrapper tracing enabled, it
doesn't
> need to be a v8::Persistent handle at all (and that's the whole point of the
> wrapper tracing).

- ScopedPersistent is traced, i.e., the reference is registered with v8. 
- ScriptWrappable still contains a v8::Persistent which is also traced by
registering it with v8

I guess what we really want is a new type of handle on the V8 side that is
usually weak, but can be used by wrapper tracing to indicate liveness and thus
make it strong.

[Marcel Hlopko, 2016-10-31 11:06:30]

On 2016/10/31 at 10:51:00, mlippautz wrote:
> On 2016/10/31 10:23:36, haraken wrote:
> > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > On 2016/10/29 18:58:26, haraken wrote:
> > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > 
> > > 
> > > Possibly, see below.
> > > 
> > > > I'm wondering why it's enough to add write barriers to
> > > V8AbstractEventListener's
> > > > ScopedPersistent. What about other ScopedPersistents traced by
> > traceWrapper()?
> > > 
> > > As far as I know, in the usual case ScopedPersistent_s on
ScriptWrappable_s do
> > > not change and this might be the only case that lazily sets the reference.
Not
> > > sure it's worth to introduce the whole infrastructure (+ back pointers
> > overhead)
> > > to cover this case. I would only introduce the whole machinery if it's
really
> > > needed, i.e., we see that many cases actually require it.
> > > 
> > > Will check what the other uses of ScopedPersistent do on Wednesday.
> > 
> > We will anyway need a new pointer type for ScopedPersistent, because one of
the
> > goals of traceWrapper() is to remove most of the ScopedPersistents from the
code
> > base and thus fix memory leaks that cross the Blink <=> V8 boundary. So I
think
> > it's worth introducing TraceWrapperV8Persistent.
> > 
> 
> Thanks for the explanation, I see that now.
> 
> > But I'm confused. How are we handling ScopedPersistents when the wrapper
tracing
> > is enabled? Is it traced by traceWrapper() but still using ScopedPersistent?
> > Then it doesn't make sense because with the wrapper tracing enabled, it
doesn't
> > need to be a v8::Persistent handle at all (and that's the whole point of the
> > wrapper tracing).
> 
> - ScopedPersistent is traced, i.e., the reference is registered with v8. 
> - ScriptWrappable still contains a v8::Persistent which is also traced by
registering it with v8
> 
> I guess what we really want is a new type of handle on the V8 side that is
usually weak, but can be used by wrapper tracing to indicate liveness and thus
make it strong.

LGTM for the logic in this cl.

There is no simple solution (that doesn't regress memory or performance) to
making progress in the persistent cycles front while object grouping is still in
use. Our effort is to ship wrapper tracing, and then solve the cycles issue,
once object grouping is gone. That should be "easy", just like Michael said, by
introducing new v8 wrapper-tracing handle type, that normally would be weak, but
wrapper tracing "will turn it into strong" upon reaching. Does it make sense?

[haraken, 2016-10-31 12:29:26]

On 2016/10/31 11:06:30, Marcel Hlopko wrote:
> On 2016/10/31 at 10:51:00, mlippautz wrote:
> > On 2016/10/31 10:23:36, haraken wrote:
> > > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > > On 2016/10/29 18:58:26, haraken wrote:
> > > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > > 
> > > > 
> > > > Possibly, see below.
> > > > 
> > > > > I'm wondering why it's enough to add write barriers to
> > > > V8AbstractEventListener's
> > > > > ScopedPersistent. What about other ScopedPersistents traced by
> > > traceWrapper()?
> > > > 
> > > > As far as I know, in the usual case ScopedPersistent_s on
> ScriptWrappable_s do
> > > > not change and this might be the only case that lazily sets the
reference.
> Not
> > > > sure it's worth to introduce the whole infrastructure (+ back pointers
> > > overhead)
> > > > to cover this case. I would only introduce the whole machinery if it's
> really
> > > > needed, i.e., we see that many cases actually require it.
> > > > 
> > > > Will check what the other uses of ScopedPersistent do on Wednesday.
> > > 
> > > We will anyway need a new pointer type for ScopedPersistent, because one
of
> the
> > > goals of traceWrapper() is to remove most of the ScopedPersistents from
the
> code
> > > base and thus fix memory leaks that cross the Blink <=> V8 boundary. So I
> think
> > > it's worth introducing TraceWrapperV8Persistent.
> > > 
> > 
> > Thanks for the explanation, I see that now.
> > 
> > > But I'm confused. How are we handling ScopedPersistents when the wrapper
> tracing
> > > is enabled? Is it traced by traceWrapper() but still using
ScopedPersistent?
> > > Then it doesn't make sense because with the wrapper tracing enabled, it
> doesn't
> > > need to be a v8::Persistent handle at all (and that's the whole point of
the
> > > wrapper tracing).
> > 
> > - ScopedPersistent is traced, i.e., the reference is registered with v8. 
> > - ScriptWrappable still contains a v8::Persistent which is also traced by
> registering it with v8
> > 
> > I guess what we really want is a new type of handle on the V8 side that is
> usually weak, but can be used by wrapper tracing to indicate liveness and thus
> make it strong.
> 
> LGTM for the logic in this cl.
> 
> There is no simple solution (that doesn't regress memory or performance) to
> making progress in the persistent cycles front while object grouping is still
in
> use. Our effort is to ship wrapper tracing, and then solve the cycles issue,
> once object grouping is gone. That should be "easy", just like Michael said,
by
> introducing new v8 wrapper-tracing handle type, that normally would be weak,
but
> wrapper tracing "will turn it into strong" upon reaching. Does it make sense?

The shipping plan makes sense.

But then why do we need to add a write barrier to ScopedPersistent? It is
already treated as a root set, so we shouldn't need to worry about the write
barrier, right?

[Michael Lippautz, 2016-10-31 13:04:14]

On 2016/10/31 12:29:26, haraken wrote:
> On 2016/10/31 11:06:30, Marcel Hlopko wrote:
> > On 2016/10/31 at 10:51:00, mlippautz wrote:
> > > On 2016/10/31 10:23:36, haraken wrote:
> > > > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > > > On 2016/10/29 18:58:26, haraken wrote:
> > > > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > > > 
> > > > > 
> > > > > Possibly, see below.
> > > > > 
> > > > > > I'm wondering why it's enough to add write barriers to
> > > > > V8AbstractEventListener's
> > > > > > ScopedPersistent. What about other ScopedPersistents traced by
> > > > traceWrapper()?
> > > > > 
> > > > > As far as I know, in the usual case ScopedPersistent_s on
> > ScriptWrappable_s do
> > > > > not change and this might be the only case that lazily sets the
> reference.
> > Not
> > > > > sure it's worth to introduce the whole infrastructure (+ back pointers
> > > > overhead)
> > > > > to cover this case. I would only introduce the whole machinery if it's
> > really
> > > > > needed, i.e., we see that many cases actually require it.
> > > > > 
> > > > > Will check what the other uses of ScopedPersistent do on Wednesday.
> > > > 
> > > > We will anyway need a new pointer type for ScopedPersistent, because one
> of
> > the
> > > > goals of traceWrapper() is to remove most of the ScopedPersistents from
> the
> > code
> > > > base and thus fix memory leaks that cross the Blink <=> V8 boundary. So
I
> > think
> > > > it's worth introducing TraceWrapperV8Persistent.
> > > > 
> > > 
> > > Thanks for the explanation, I see that now.
> > > 
> > > > But I'm confused. How are we handling ScopedPersistents when the wrapper
> > tracing
> > > > is enabled? Is it traced by traceWrapper() but still using
> ScopedPersistent?
> > > > Then it doesn't make sense because with the wrapper tracing enabled, it
> > doesn't
> > > > need to be a v8::Persistent handle at all (and that's the whole point of
> the
> > > > wrapper tracing).
> > > 
> > > - ScopedPersistent is traced, i.e., the reference is registered with v8. 
> > > - ScriptWrappable still contains a v8::Persistent which is also traced by
> > registering it with v8
> > > 
> > > I guess what we really want is a new type of handle on the V8 side that is
> > usually weak, but can be used by wrapper tracing to indicate liveness and
thus
> > make it strong.
> > 
> > LGTM for the logic in this cl.
> > 
> > There is no simple solution (that doesn't regress memory or performance) to
> > making progress in the persistent cycles front while object grouping is
still
> in
> > use. Our effort is to ship wrapper tracing, and then solve the cycles issue,
> > once object grouping is gone. That should be "easy", just like Michael said,
> by
> > introducing new v8 wrapper-tracing handle type, that normally would be weak,
> but
> > wrapper tracing "will turn it into strong" upon reaching. Does it make
sense?
> 
> The shipping plan makes sense.
> 
> But then why do we need to add a write barrier to ScopedPersistent? It is
> already treated as a root set, so we shouldn't need to worry about the write
> barrier, right?

It's in the root set and will be held alive, yes. 

The barrier is needed since we already have the verifier in place that checks
non-inc vs inc wrapper tracing assuming that the references are weak (and only
strong by tracing).

[Michael Lippautz, 2016-10-31 13:04:16]

On 2016/10/31 12:29:26, haraken wrote:
> On 2016/10/31 11:06:30, Marcel Hlopko wrote:
> > On 2016/10/31 at 10:51:00, mlippautz wrote:
> > > On 2016/10/31 10:23:36, haraken wrote:
> > > > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > > > On 2016/10/29 18:58:26, haraken wrote:
> > > > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > > > 
> > > > > 
> > > > > Possibly, see below.
> > > > > 
> > > > > > I'm wondering why it's enough to add write barriers to
> > > > > V8AbstractEventListener's
> > > > > > ScopedPersistent. What about other ScopedPersistents traced by
> > > > traceWrapper()?
> > > > > 
> > > > > As far as I know, in the usual case ScopedPersistent_s on
> > ScriptWrappable_s do
> > > > > not change and this might be the only case that lazily sets the
> reference.
> > Not
> > > > > sure it's worth to introduce the whole infrastructure (+ back pointers
> > > > overhead)
> > > > > to cover this case. I would only introduce the whole machinery if it's
> > really
> > > > > needed, i.e., we see that many cases actually require it.
> > > > > 
> > > > > Will check what the other uses of ScopedPersistent do on Wednesday.
> > > > 
> > > > We will anyway need a new pointer type for ScopedPersistent, because one
> of
> > the
> > > > goals of traceWrapper() is to remove most of the ScopedPersistents from
> the
> > code
> > > > base and thus fix memory leaks that cross the Blink <=> V8 boundary. So
I
> > think
> > > > it's worth introducing TraceWrapperV8Persistent.
> > > > 
> > > 
> > > Thanks for the explanation, I see that now.
> > > 
> > > > But I'm confused. How are we handling ScopedPersistents when the wrapper
> > tracing
> > > > is enabled? Is it traced by traceWrapper() but still using
> ScopedPersistent?
> > > > Then it doesn't make sense because with the wrapper tracing enabled, it
> > doesn't
> > > > need to be a v8::Persistent handle at all (and that's the whole point of
> the
> > > > wrapper tracing).
> > > 
> > > - ScopedPersistent is traced, i.e., the reference is registered with v8. 
> > > - ScriptWrappable still contains a v8::Persistent which is also traced by
> > registering it with v8
> > > 
> > > I guess what we really want is a new type of handle on the V8 side that is
> > usually weak, but can be used by wrapper tracing to indicate liveness and
thus
> > make it strong.
> > 
> > LGTM for the logic in this cl.
> > 
> > There is no simple solution (that doesn't regress memory or performance) to
> > making progress in the persistent cycles front while object grouping is
still
> in
> > use. Our effort is to ship wrapper tracing, and then solve the cycles issue,
> > once object grouping is gone. That should be "easy", just like Michael said,
> by
> > introducing new v8 wrapper-tracing handle type, that normally would be weak,
> but
> > wrapper tracing "will turn it into strong" upon reaching. Does it make
sense?
> 
> The shipping plan makes sense.
> 
> But then why do we need to add a write barrier to ScopedPersistent? It is
> already treated as a root set, so we shouldn't need to worry about the write
> barrier, right?

It's in the root set and will be held alive, yes. 

The barrier is needed since we already have the verifier in place that checks
non-inc vs inc wrapper tracing assuming that the references are weak (and only
strong by tracing).

[haraken, 2016-11-01 04:28:08]

On 2016/10/31 13:04:16, Michael Lippautz wrote:
> On 2016/10/31 12:29:26, haraken wrote:
> > On 2016/10/31 11:06:30, Marcel Hlopko wrote:
> > > On 2016/10/31 at 10:51:00, mlippautz wrote:
> > > > On 2016/10/31 10:23:36, haraken wrote:
> > > > > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > > > > On 2016/10/29 18:58:26, haraken wrote:
> > > > > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > > > > 
> > > > > > 
> > > > > > Possibly, see below.
> > > > > > 
> > > > > > > I'm wondering why it's enough to add write barriers to
> > > > > > V8AbstractEventListener's
> > > > > > > ScopedPersistent. What about other ScopedPersistents traced by
> > > > > traceWrapper()?
> > > > > > 
> > > > > > As far as I know, in the usual case ScopedPersistent_s on
> > > ScriptWrappable_s do
> > > > > > not change and this might be the only case that lazily sets the
> > reference.
> > > Not
> > > > > > sure it's worth to introduce the whole infrastructure (+ back
pointers
> > > > > overhead)
> > > > > > to cover this case. I would only introduce the whole machinery if
it's
> > > really
> > > > > > needed, i.e., we see that many cases actually require it.
> > > > > > 
> > > > > > Will check what the other uses of ScopedPersistent do on Wednesday.
> > > > > 
> > > > > We will anyway need a new pointer type for ScopedPersistent, because
one
> > of
> > > the
> > > > > goals of traceWrapper() is to remove most of the ScopedPersistents
from
> > the
> > > code
> > > > > base and thus fix memory leaks that cross the Blink <=> V8 boundary.
So
> I
> > > think
> > > > > it's worth introducing TraceWrapperV8Persistent.
> > > > > 
> > > > 
> > > > Thanks for the explanation, I see that now.
> > > > 
> > > > > But I'm confused. How are we handling ScopedPersistents when the
wrapper
> > > tracing
> > > > > is enabled? Is it traced by traceWrapper() but still using
> > ScopedPersistent?
> > > > > Then it doesn't make sense because with the wrapper tracing enabled,
it
> > > doesn't
> > > > > need to be a v8::Persistent handle at all (and that's the whole point
of
> > the
> > > > > wrapper tracing).
> > > > 
> > > > - ScopedPersistent is traced, i.e., the reference is registered with v8.

> > > > - ScriptWrappable still contains a v8::Persistent which is also traced
by
> > > registering it with v8
> > > > 
> > > > I guess what we really want is a new type of handle on the V8 side that
is
> > > usually weak, but can be used by wrapper tracing to indicate liveness and
> thus
> > > make it strong.
> > > 
> > > LGTM for the logic in this cl.
> > > 
> > > There is no simple solution (that doesn't regress memory or performance)
to
> > > making progress in the persistent cycles front while object grouping is
> still
> > in
> > > use. Our effort is to ship wrapper tracing, and then solve the cycles
issue,
> > > once object grouping is gone. That should be "easy", just like Michael
said,
> > by
> > > introducing new v8 wrapper-tracing handle type, that normally would be
weak,
> > but
> > > wrapper tracing "will turn it into strong" upon reaching. Does it make
> sense?
> > 
> > The shipping plan makes sense.
> > 
> > But then why do we need to add a write barrier to ScopedPersistent? It is
> > already treated as a root set, so we shouldn't need to worry about the write
> > barrier, right?
> 
> It's in the root set and will be held alive, yes. 
> 
> The barrier is needed since we already have the verifier in place that checks
> non-inc vs inc wrapper tracing assuming that the references are weak (and only
> strong by tracing).

Ah, got it. So the ScopedPersistent is NOT in the root set (because it's
weakened).

If this is really the only place where a ScopedPersistent is updated, this CL
LGTM.

However, in general I think that all weak ScopedPersistents should be replaced
with TraceWrapperV8Persistents. For example, if we have the following scenario,
it will miss a write barrier.

class DOMObject {
  DOMObject() {  // The ScopedPersistent is assigned in the constructor but...
    m_persistent.set(...);
    m_persistent.setPhantom();
    someV8API();  // if this V8 API triggers a GC...
    m_persistent2.set(...);  // we need to have a write barrier for
m_persistent2.
    m_persistent2.setPhantom();
  }
  ScopedPersistent<v8::Value> m_persistent;
  ScopedPersistent<v8::Value> m_persistent2;
};

[Michael Lippautz, 2016-11-01 11:24:29]

On 2016/11/01 04:28:08, haraken wrote:
> On 2016/10/31 13:04:16, Michael Lippautz wrote:
> > On 2016/10/31 12:29:26, haraken wrote:
> > > On 2016/10/31 11:06:30, Marcel Hlopko wrote:
> > > > On 2016/10/31 at 10:51:00, mlippautz wrote:
> > > > > On 2016/10/31 10:23:36, haraken wrote:
> > > > > > On 2016/10/30 20:55:03, Michael Lippautz wrote:
> > > > > > > On 2016/10/29 18:58:26, haraken wrote:
> > > > > > > > Maybe should we introduce TraceWrapperV8Persistent?
> > > > > > > > 
> > > > > > > 
> > > > > > > Possibly, see below.
> > > > > > > 
> > > > > > > > I'm wondering why it's enough to add write barriers to
> > > > > > > V8AbstractEventListener's
> > > > > > > > ScopedPersistent. What about other ScopedPersistents traced by
> > > > > > traceWrapper()?
> > > > > > > 
> > > > > > > As far as I know, in the usual case ScopedPersistent_s on
> > > > ScriptWrappable_s do
> > > > > > > not change and this might be the only case that lazily sets the
> > > reference.
> > > > Not
> > > > > > > sure it's worth to introduce the whole infrastructure (+ back
> pointers
> > > > > > overhead)
> > > > > > > to cover this case. I would only introduce the whole machinery if
> it's
> > > > really
> > > > > > > needed, i.e., we see that many cases actually require it.
> > > > > > > 
> > > > > > > Will check what the other uses of ScopedPersistent do on
Wednesday.
> > > > > > 
> > > > > > We will anyway need a new pointer type for ScopedPersistent, because
> one
> > > of
> > > > the
> > > > > > goals of traceWrapper() is to remove most of the ScopedPersistents
> from
> > > the
> > > > code
> > > > > > base and thus fix memory leaks that cross the Blink <=> V8 boundary.
> So
> > I
> > > > think
> > > > > > it's worth introducing TraceWrapperV8Persistent.
> > > > > > 
> > > > > 
> > > > > Thanks for the explanation, I see that now.
> > > > > 
> > > > > > But I'm confused. How are we handling ScopedPersistents when the
> wrapper
> > > > tracing
> > > > > > is enabled? Is it traced by traceWrapper() but still using
> > > ScopedPersistent?
> > > > > > Then it doesn't make sense because with the wrapper tracing enabled,
> it
> > > > doesn't
> > > > > > need to be a v8::Persistent handle at all (and that's the whole
point
> of
> > > the
> > > > > > wrapper tracing).
> > > > > 
> > > > > - ScopedPersistent is traced, i.e., the reference is registered with
v8.
> 
> > > > > - ScriptWrappable still contains a v8::Persistent which is also traced
> by
> > > > registering it with v8
> > > > > 
> > > > > I guess what we really want is a new type of handle on the V8 side
that
> is
> > > > usually weak, but can be used by wrapper tracing to indicate liveness
and
> > thus
> > > > make it strong.
> > > > 
> > > > LGTM for the logic in this cl.
> > > > 
> > > > There is no simple solution (that doesn't regress memory or performance)
> to
> > > > making progress in the persistent cycles front while object grouping is
> > still
> > > in
> > > > use. Our effort is to ship wrapper tracing, and then solve the cycles
> issue,
> > > > once object grouping is gone. That should be "easy", just like Michael
> said,
> > > by
> > > > introducing new v8 wrapper-tracing handle type, that normally would be
> weak,
> > > but
> > > > wrapper tracing "will turn it into strong" upon reaching. Does it make
> > sense?
> > > 
> > > The shipping plan makes sense.
> > > 
> > > But then why do we need to add a write barrier to ScopedPersistent? It is
> > > already treated as a root set, so we shouldn't need to worry about the
write
> > > barrier, right?
> > 
> > It's in the root set and will be held alive, yes. 
> > 
> > The barrier is needed since we already have the verifier in place that
checks
> > non-inc vs inc wrapper tracing assuming that the references are weak (and
only
> > strong by tracing).
> 
> Ah, got it. So the ScopedPersistent is NOT in the root set (because it's
> weakened).
> 
> If this is really the only place where a ScopedPersistent is updated, this CL
> LGTM.
> 
> However, in general I think that all weak ScopedPersistents should be replaced
> with TraceWrapperV8Persistents. For example, if we have the following
scenario,
> it will miss a write barrier.
> 
> class DOMObject {
>   DOMObject() {  // The ScopedPersistent is assigned in the constructor but...
>     m_persistent.set(...);
>     m_persistent.setPhantom();
>     someV8API();  // if this V8 API triggers a GC...
>     m_persistent2.set(...);  // we need to have a write barrier for
> m_persistent2.
>     m_persistent2.setPhantom();
>   }
>   ScopedPersistent<v8::Value> m_persistent;
>   ScopedPersistent<v8::Value> m_persistent2;
> };

Correct. I will check how many cases of tracing through ScopedPersistent we have
and whether we should add a type to better support this.

[Michael Lippautz, 2016-11-03 14:59:30]

Description was changed from

==========
[wrapper-tracing] Fix EventTarget with LazyEventListeners

LazyEventListener_s have their listener object set at some later point in time.
We need to trace the EventListener objects in any case and add a write barrier
for the ScopedPersistent.

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========

to

==========
[wrapper-tracing] Fix EventTarget with LazyEventListeners

LazyEventListener_s have their listener object set at some later point in time.
We need to trace the EventListener objects in any case and add a write barrier
for the ScopedPersistent.

BUG=chromium:468240
R=haraken@chromium.org,hlopko@chromium.org
==========