Bug 104734 – [V8] Reachable event listeners on image elements can be collected in a minor DOM GC

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=164882

[Kentaro Hara]

Created attachment 178908 [details]
Patch

[Kenneth Russell]

Comment on attachment 178908 [details]
Patch

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.

[Kentaro Hara]

Created attachment 178916 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 178908 [details])
> 
> > 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.

Improved the comment. Thanks!

> > 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.

Fixed.

> 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

Will investigate. Although XMLHttpRequest is not a problem because it is not a DOM node (XMLHttpRequest is a DOM object and thus the minor GC does nothing), the problem can happen for audio elements and video elements that have some pending activities.

[Kentaro Hara]

Created attachment 178937 [details]
Patch

[Kenneth Russell]

Comment on attachment 178937 [details]
Patch

Looks good -- but is it not possible to add a test for this? Something along the lines of loading a bunch of images and ensuring that their onload handlers are all eventually called. r=me

[Kentaro Hara]

Comment on attachment 178937 [details]
Patch

For now let me land the patch, since this is an urgent fix and this change is anyway harmless. Let me seek a good way to test the change.

[Kenneth Russell]

(In reply to comment #7)
> (From update of attachment 178937 [details])
> For now let me land the patch, since this is an urgent fix and this change is anyway harmless. Let me seek a good way to test the change.

Agree.

[WebKit Review Bot]

Comment on attachment 178937 [details]
Patch

Clearing flags on attachment: 178937

Committed r137415: <http://trac.webkit.org/changeset/137415>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.