RESOLVED FIXED 104203
[V8] Remove addImplicitReferencesForNodeWithEventListeners() 
https://bugs.webkit.org/show_bug.cgi?id=104203
Summary [V8] Remove addImplicitReferencesForNodeWithEventListeners()
Kentaro Hara
Reported 2012-12-05 18:18:36 PST
We can use opaqueRootForGC() instead. By this change, we can remove all AddImplicitReferences() from V8 bindings.
Attachments
Patch (3.63 KB, patch)
2012-12-05 18:20 PST, Kentaro Hara 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Kentaro Hara
Comment 1 2012-12-05 18:20:32 PST
Created attachment 177899 [details] Patch
Adam Barth
Comment 2 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.
Adam Barth
Comment 3 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.
Kentaro Hara
Comment 4 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?
Adam Barth
Comment 5 2012-12-05 20:44:58 PST
> Maybe we shouldn't make this change? I don't think we should worry about it.
Kentaro Hara
Comment 6 2012-12-05 20:45:27 PST
Comment on attachment 177899 [details] Patch ok
WebKit Review Bot
Comment 7 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>
WebKit Review Bot
Comment 8 2012-12-05 20:55:09 PST
All reviewed patches have been landed. Closing bug.
Yury Semikhatsky
Comment 9 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.
Kentaro Hara
Comment 10 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.
Kentaro Hara
Comment 11 2012-12-17 01:27:20 PST
Rolled out in r136794.
Yury Semikhatsky
Comment 12 2012-12-17 01:30:00 PST
(In reply to comment #11) > Rolled out in r136794. You probably meant r137885
Note You need to log in before you can comment on or make changes to this bug.