Created attachment 126146 [details] LayoutTest showing that FileList is broken This cropped up when I was looking at bug 73865. The V8 bindings for collection like list interfaces that contains non Node wrappers are all broken because we do not keep an implicit reference to the items. The following fails for example: inputElement.files[0].custom = 42; forceGC(); shouldBe('inputElement.files[0].custom', '42'); The code snippet above will fail for all lists where the item method returns a non Node, non value type. By searching for item() methods in the IDL files I believe all of the following are broken: ClientRectList CSSRuleList CSSValueList DataTransferItemList DOMMimeTypeArray DOMPlugin DOMPluginArray EntryArray EntryArraySync FileList GamepadList MediaStreamList MediaStreamTrackList SpeechInputResultList SQLResultSetRowList? StyleSheetList TextTrackCueList TextTrackList TouchList WebKitAnimationList Since this is a common pattern we should generate code for these in the code generator.
Created attachment 127040 [details] WIP
Comment on attachment 127040 [details] WIP This depends on bug 78052 for the tests to pass. This is pretty rough but the general idea is to let the code generator generate visitor functions for the list interface and add that to the WrapperTypeInfo. Feedback wanted.
Comment on attachment 127040 [details] WIP Attachment 127040 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11525091 New failing tests: media/track/text-track-is-reachable.html media/track/tracklist-is-reachable.html fast/dom/StyleSheet/gc-styleheet-wrapper.xhtml media/track/text-track-cue-is-reachable.html
Created attachment 129470 [details] WIP
Created attachment 149130 [details] Patch
Comment on attachment 149130 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149130&action=review > Source/WebCore/Modules/gamepad/Gamepad.idl:30 > + V8DependentLifetime The code generator should do this in these cases. > Source/WebCore/bindings/v8/custom/V8NodeListCustom.cpp:66 > + if (impl->isDynamicNodeList()) { I need to double check which NodeLists may have an ownerNode.
This is the same approach as in bug 808880 which landed yesterday.
Created attachment 151312 [details] Patch
Comment on attachment 151312 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151312&action=review The change looks good to me. > Source/WebCore/WebCore.gyp/WebCore.gyp:1058 > + '--include', '../Modules/battery', > '--include', '../Modules/filesystem', > + '--include', '../Modules/gamepad', > + '--include', '../Modules/geolocation', > '--include', '../Modules/indexeddb', > '--include', '../Modules/intents', > '--include', '../Modules/mediastream', > '--include', '../Modules/notifications', > + '--include', '../Modules/quota', > + '--include', '../Modules/speech', > '--include', '../Modules/webaudio', > '--include', '../Modules/webdatabase', > + '--include', '../Modules/websockets', Nit: How is the current WebCore.gyp working without these lines? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:208 > + if (!$indexer) { > + return 0; > + } Is there any possibility that [IndexedGetter] is defined but item() is not found? (item() might be defined in custom bindings?) > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:212 > + if (IsWrapperType($indexerType) && !IsDOMNodeType($indexerType)) { Would you elaborate on this line? What does this condition mean? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:287 > + v8::Handle<v8::Object> itemWrapper = store->$mapName().get(item.get()); Nit: ${mapName} would be better.
(In reply to comment #9) > (From update of attachment 151312 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151312&action=review > > The change looks good to me. > > > Source/WebCore/WebCore.gyp/WebCore.gyp:1058 > > + '--include', '../Modules/battery', > > '--include', '../Modules/filesystem', > > + '--include', '../Modules/gamepad', > > + '--include', '../Modules/geolocation', > > '--include', '../Modules/indexeddb', > > '--include', '../Modules/intents', > > '--include', '../Modules/mediastream', > > '--include', '../Modules/notifications', > > + '--include', '../Modules/quota', > > + '--include', '../Modules/speech', > > '--include', '../Modules/webaudio', > > '--include', '../Modules/webdatabase', > > + '--include', '../Modules/websockets', > > Nit: How is the current WebCore.gyp working without these lines? This was confusing me too. If I understand correctly there are two different ways that files are read by the script. One uses the include to build a map of known idl files and one way just does a recursive directory tree traversal. The reason this was triggered by this change was that with this change we need to go back and look at other idl files and then the include paths matter. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:208 > > + if (!$indexer) { > > + return 0; > > + } > > Is there any possibility that [IndexedGetter] is defined but item() is not found? (item() might be defined in custom bindings?) I don't believe so. There is no CustomIndexedGetter extended attribute and we generate code that always uses item. > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:212 > > + if (IsWrapperType($indexerType) && !IsDOMNodeType($indexerType)) { > > Would you elaborate on this line? What does this condition mean? If the type of the items is a non wrapper type (no wrapper is created, string, number etc) we don't need to go through this. If the type of the items is a Node then the Node is already kept alive though other means. I'm not convinced this is the case though. For example if we have a static NodeList, from querySelectorAll for example, and a node is only reachable through the collection will we keep its wrapper? I'll add a test at least. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:287 > > + v8::Handle<v8::Object> itemWrapper = store->$mapName().get(item.get()); > > Nit: ${mapName} would be better. OK
Thanks for the clarification! (In reply to comment #10) > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:208 > > > + if (!$indexer) { > > > + return 0; > > > + } > > > > Is there any possibility that [IndexedGetter] is defined but item() is not found? (item() might be defined in custom bindings?) > > I don't believe so. There is no CustomIndexedGetter extended attribute and we generate code that always uses item. Then, the code could be 'die "Indexed getter must have item()." unless $indexer;'. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:212 > > > + if (IsWrapperType($indexerType) && !IsDOMNodeType($indexerType)) { > > > > Would you elaborate on this line? What does this condition mean? > > If the type of the items is a non wrapper type (no wrapper is created, string, number etc) we don't need to go through this. > > If the type of the items is a Node then the Node is already kept alive though other means. I'm not convinced this is the case though. For example if we have a static NodeList, from querySelectorAll for example, and a node is only reachable through the collection will we keep its wrapper? I'll add a test at least. IMPO, all reachable wrapper objects should be kept alive, but I'm not sure. I am sad we don't know what wrapper objects should be kept alive. Isn't there any spec about wrapper lifetime? I've searched it (when I was implementing the new lifetime management of DOM nodes) but couldn't find it.
(In reply to comment #11) > Then, the code could be 'die "Indexed getter must have item()." unless $indexer;'. Good idea. Will do. > IMPO, all reachable wrapper objects should be kept alive, but I'm not sure. I am sad we don't know what wrapper objects should be kept alive. Isn't there any spec about wrapper lifetime? I've searched it (when I was implementing the new lifetime management of DOM nodes) but couldn't find it. Wrappers are an implementation detail. Conceptually there is only one object, the javascript object. The case I am worried about is. document.body.innerHTML = '<b></b>'; var els = document.body.querySelectorAll('*'); els[0].custom = 42; document.body.textContent = ''; gc(); shouldBe('els[0].custom', '42');
(In reply to comment #12) > Conceptually there is only one object, the javascript object. This means that all reachable wrapper objects in the JavaScript side should be kept alive, right? If so, > For example if we have a static NodeList, from querySelectorAll for example, and a node is only reachable through the collection will we keep its wrapper? the answer of the above question is yes (i.e. shouldBe('els[0].custom', '42') in your example). The behavior sounds reasonable to me.
I filed a new bug for the static NodeList bug. https://bugs.webkit.org/show_bug.cgi?id=91014
Comment on attachment 151312 [details] Patch neat
V8 is no longer supported in WebKit.