Bug 104203 – [V8] Remove addImplicitReferencesForNodeWithEventListeners()

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Kentaro Hara]

We can use opaqueRootForGC() instead. By this change, we can remove all AddImplicitReferences() from V8 bindings.

[Kentaro Hara]

Created attachment 177899 [details]
Patch

[Adam Barth]

Comment on attachment 177899 [details]
Patch

I tried this before but ran into trouble (some tests failed).  It's likely I screwed it up though.

[Adam Barth]

Comment on attachment 177899 [details]
Patch

There is a subtle difference here: Previously when someone was hold an event listener function, they wouldn't hold the node it was attached to (because AddImplicitReferences are one-way connections).  Now we'll hold the node as well (because addToGroup is a two-way connection).  I can't see how that would matter in practice.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 177899 [details])
> There is a subtle difference here: Previously when someone was hold an event listener function, they wouldn't hold the node it was attached to (because AddImplicitReferences are one-way connections).  Now we'll hold the node as well (because addToGroup is a two-way connection).  I can't see how that would matter in practice.

Good point. In theory, the following situation would be possible:

- There is a tree T.
- A node P is in T. P has an event listener L.
- A JS object X has a reference to L. X is reachable from JS roots.
- All nodes in T become unreachable.
- T is expected to be collected. However, due to the reference from X to L, T is not collected.

Maybe we shouldn't make this change?

[Adam Barth]

> Maybe we shouldn't make this change?

I don't think we should worry about it.

[Kentaro Hara]

Comment on attachment 177899 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 177899 [details]
Patch

Clearing flags on attachment: 177899

Committed r136794: <http://trac.webkit.org/changeset/136794>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

(In reply to comment #5)
> > Maybe we shouldn't make this change?
> 
> I don't think we should worry about it.

Could you elaborate on this? Now adding a listener to a node will keep the tree alive forever unless the listener is also collected. I can easily imagine a case when the same function is reused as an event listener for different DOM trees and in this case all the trees will never be GCed.

What is the benefit of this patch given that it introduces this regression? I think it should be rolled out.

[Kentaro Hara]

(In reply to comment #9)
> (In reply to comment #5)
> > > Maybe we shouldn't make this change?
> > 
> > I don't think we should worry about it.
> 
> Could you elaborate on this? Now adding a listener to a node will keep the tree alive forever unless the listener is also collected. I can easily imagine a case when the same function is reused as an event listener for different DOM trees and in this case all the trees will never be GCed.
> 
> What is the benefit of this patch given that it introduces this regression? I think it should be rolled out.

Adam is on vacation. I'll roll out the patch for now.

[Kentaro Hara]

Rolled out in r136794.

[Yury Semikhatsky]

(In reply to comment #11)
> Rolled out in r136794.

You probably meant r137885