Bug 78149 – [V8] Bindings for list interfaces are all broken

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

[Erik Arvidsson]

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.

[Erik Arvidsson]

Created attachment 127040 [details]
WIP

[Erik Arvidsson]

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

Created attachment 129470 [details]
WIP

[Erik Arvidsson]

Created attachment 149130 [details]
Patch

[Erik Arvidsson]

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]

This is the same approach as in bug 808880 which landed yesterday.

[Erik Arvidsson]

Created attachment 151312 [details]
Patch

[Kentaro Hara]

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]

(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]

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]

(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]

(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]

I filed a new bug for the static NodeList bug. 

https://bugs.webkit.org/show_bug.cgi?id=91014

[anton muhin]

Comment on attachment 151312 [details]
Patch

neat

[Sam Weinig]

V8 is no longer supported in WebKit.