Bug 78149 - [V8] Bindings for list interfaces are all broken
Summary: [V8] Bindings for list interfaces are all broken
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Erik Arvidsson
URL:
Keywords:
Depends on:
Blocks: 73865
  Show dependency treegraph
 
Reported: 2012-02-08 13:50 PST by Erik Arvidsson
Modified: 2013-04-05 17:25 PDT (History)
15 users (show)

See Also:


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 Details | Formatted Diff | Diff
WIP (59.76 KB, patch)
2012-02-29 10:18 PST, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (21.55 KB, patch)
2012-06-22 16:14 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff
Patch (30.72 KB, patch)
2012-07-09 14:01 PDT, Erik Arvidsson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Arvidsson 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.
Comment 1 Erik Arvidsson 2012-02-14 14:06:02 PST
Created attachment 127040 [details]
WIP
Comment 2 Erik Arvidsson 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.
Comment 3 WebKit Review Bot 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
Comment 4 Erik Arvidsson 2012-02-29 10:18:18 PST
Created attachment 129470 [details]
WIP
Comment 5 Erik Arvidsson 2012-06-22 16:14:15 PDT
Created attachment 149130 [details]
Patch
Comment 6 Erik Arvidsson 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.
Comment 7 Erik Arvidsson 2012-06-22 16:19:59 PDT
This is the same approach as in bug 808880 which landed yesterday.
Comment 8 Erik Arvidsson 2012-07-09 14:01:44 PDT
Created attachment 151312 [details]
Patch
Comment 9 Kentaro Hara 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.
Comment 10 Erik Arvidsson 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
Comment 11 Kentaro Hara 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.
Comment 12 Erik Arvidsson 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');
Comment 13 Kentaro Hara 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.
Comment 14 Erik Arvidsson 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
Comment 15 anton muhin 2012-07-12 00:22:16 PDT
Comment on attachment 151312 [details]
Patch

neat
Comment 16 Sam Weinig 2013-04-05 17:24:54 PDT
V8 is no longer supported in WebKit.