RESOLVED WONTFIX 78149
[V8] Bindings for list interfaces are all broken 
https://bugs.webkit.org/show_bug.cgi?id=78149
Summary [V8] Bindings for list interfaces are all broken
Erik Arvidsson
Reported 2012-02-08 13:50:14 PST
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.
Attachments
LayoutTest showing that FileList is broken (1.00 KB, text/html)
2012-02-08 13:50 PST, Erik Arvidsson 		no flags
Details
WIP (25.04 KB, patch)
2012-02-14 14:06 PST, Erik Arvidsson 		no flags
DetailsFormatted DiffDiff
WIP (59.76 KB, patch)
2012-02-29 10:18 PST, Erik Arvidsson 		no flags
DetailsFormatted DiffDiff
Patch (21.55 KB, patch)
2012-06-22 16:14 PDT, Erik Arvidsson 		no flags
DetailsFormatted DiffDiff
Patch (30.72 KB, patch)
2012-07-09 14:01 PDT, Erik Arvidsson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Erik Arvidsson
Comment 1 2012-02-14 14:06:02 PST
Created attachment 127040 [details] WIP
Erik Arvidsson
Comment 2 2012-02-14 14:09:16 PST
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.
WebKit Review Bot
Comment 3 2012-02-14 15:28:32 PST
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
Erik Arvidsson
Comment 4 2012-02-29 10:18:18 PST
Created attachment 129470 [details] WIP
Erik Arvidsson
Comment 5 2012-06-22 16:14:15 PDT
Created attachment 149130 [details] Patch
Erik Arvidsson
Comment 6 2012-06-22 16:18:35 PDT
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.
Erik Arvidsson
Comment 7 2012-06-22 16:19:59 PDT
This is the same approach as in bug 808880 which landed yesterday.
Erik Arvidsson
Comment 8 2012-07-09 14:01:44 PDT
Created attachment 151312 [details] Patch
Kentaro Hara
Comment 9 2012-07-09 23:34:28 PDT
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.
Erik Arvidsson
Comment 10 2012-07-11 09:26:38 PDT
(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
Kentaro Hara
Comment 11 2012-07-11 09:35:48 PDT
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.
Erik Arvidsson
Comment 12 2012-07-11 10:15:39 PDT
(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');
Kentaro Hara
Comment 13 2012-07-11 10:20:57 PDT
(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.
Erik Arvidsson
Comment 14 2012-07-11 14:15:06 PDT
I filed a new bug for the static NodeList bug. https://bugs.webkit.org/show_bug.cgi?id=91014
anton muhin
Comment 15 2012-07-12 00:22:16 PDT
Comment on attachment 151312 [details] Patch neat
Sam Weinig
Comment 16 2013-04-05 17:24:54 PDT
V8 is no longer supported in WebKit.
Note You need to log in before you can comment on or make changes to this bug.