Chromium bug: http://code.google.com/p/chromium/issues/detail?id=164882
Created attachment 178908 [details] Patch
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.
Created attachment 178916 [details] patch for landing
(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.
Created attachment 178937 [details] Patch
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
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.
(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.
Comment on attachment 178937 [details] Patch Clearing flags on attachment: 178937 Committed r137415: <http://trac.webkit.org/changeset/137415>
All reviewed patches have been landed. Closing bug.