Bug 104203 - [V8] Remove addImplicitReferencesForNodeWithEventListeners()
Summary: [V8] Remove addImplicitReferencesForNodeWithEventListeners()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-05 18:18 PST by Kentaro Hara
Modified: 2012-12-17 01:30 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.63 KB, patch)
2012-12-05 18:20 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-12-05 18:18:36 PST
We can use opaqueRootForGC() instead. By this change, we can remove all AddImplicitReferences() from V8 bindings.
Comment 1 Kentaro Hara 2012-12-05 18:20:32 PST
Created attachment 177899 [details]
Patch
Comment 2 Adam Barth 2012-12-05 18:40:43 PST
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.
Comment 3 Adam Barth 2012-12-05 18:42:26 PST
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.
Comment 4 Kentaro Hara 2012-12-05 19:41:53 PST
(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?
Comment 5 Adam Barth 2012-12-05 20:44:58 PST
> Maybe we shouldn't make this change?

I don't think we should worry about it.
Comment 6 Kentaro Hara 2012-12-05 20:45:27 PST
Comment on attachment 177899 [details]
Patch

ok
Comment 7 WebKit Review Bot 2012-12-05 20:55:05 PST
Comment on attachment 177899 [details]
Patch

Clearing flags on attachment: 177899

Committed r136794: <http://trac.webkit.org/changeset/136794>
Comment 8 WebKit Review Bot 2012-12-05 20:55:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Yury Semikhatsky 2012-12-17 00:48:36 PST
(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.
Comment 10 Kentaro Hara 2012-12-17 00:54:14 PST
(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.
Comment 11 Kentaro Hara 2012-12-17 01:27:20 PST
Rolled out in r136794.
Comment 12 Yury Semikhatsky 2012-12-17 01:30:00 PST
(In reply to comment #11)
> Rolled out in r136794.

You probably meant r137885