[webkit-reviews] review granted: [Bug 104734] [V8] Reachable event listeners on image elements can be collected in a minor DOM GC : [Attachment 178908] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has granted Kentaro Hara
<haraken at chromium.org>'s request for review:
Bug 104734: [V8] Reachable event listeners on image elements can be collected
in a minor DOM GC
https://bugs.webkit.org/show_bug.cgi?id=104734

Attachment 178908: Patch
https://bugs.webkit.org/attachment.cgi?id=178908&action=review

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=178908&action=review


The code change looks OK as an immediate patch. Please make the comments more
precise. I am concerned that this is only one example of a pattern where a DOM
related object has a JavaScript callback, and that the new minor DOM GC may be
collecting the JavaScript wrappers for those objects too eagerly, causing
callbacks to randomly not be called. I'm concerned about XMLHttpRequest in
particular. I think the V8 team and all reviewers of the minor DOM GC algorithm
should be engaged urgently to discuss what is going on here. r=me

> Source/WebCore/ChangeLog:16
> +	   However, as far as I experimented, it looks like this patch fixes
the bug.

Is the test case tf-test.zip attached to
https://code.google.com/p/chromium/issues/detail?id=164882 sufficient?

> Source/WebCore/bindings/v8/V8GCController.cpp:226
> +	       // FIXME: I'm not sure why node->hasEventListeners() is needed.

It seems to me that the JavaScript wrapper for the Image element is being
garbage collected too early. I think it is worth mentioning this in the
comment.

> Source/WebCore/bindings/v8/V8GCController.cpp:227
> +	       // But without this check, image loading can crash.

It isn't that image loading can crash; it is that the image's onload handler is
never called.