Bug 108658 – [v8] Binding Integrity doesn't handle multiple expected subclasses wrapped through same interface.

• Component: WebKit Misc.
• Status: RESOLVED WONTFIX

[Thomas Sepez]

This bug tracks the changes needed to make V8 binding integrity not throw errors when there are multiple valid native sub-types passed through the same .IDL wrapper.

[Thomas Sepez]

Created attachment 186091 [details]
Patch.

The .cpp files are new baseline output for the bindings test.  May have spurious style warnings which can't be resolved.

[WebKit Review Bot]

Attachment 186091 [details] did not pass style-queue:

Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp:39:  _ZTVN7WebCore20TestEventConstructorE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp:40:  _ZTVN7WebCore26TestMediaQueryListListenerE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp:42:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp:44:  _ZTVN7WebCore26TestOverloadedConstructorsE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp:39:  _ZTVN7WebCore21TestCustomNamedGetterE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:43:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp:45:  _ZTVN7WebCore34TestSerializedScriptValueInterfaceE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp:36:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp:38:  _ZTVN7WebCore13TestExceptionE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:79:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:81:  _ZTVN7WebCore7TestObjE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp:39:  _ZTVN7WebCore8TestNodeE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp:38:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp:40:  _ZTVN7WebCore19TestActiveDOMObjectE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp:41:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp:43:  _ZTVN7WebCore15TestEventTargetE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp:47:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp:49:  _ZTVN7WebCore12Float64ArrayE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:45:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:47:  _ZTVN7WebCore13TestInterfaceE is incorrectly named. Don't use underscores in your identifiFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/Modules/indexeddb/IDBAny.idl', u'Source/WebCore/Modules/webaudio/AudioDestinationNode.idl', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/IDLAttributes.txt', u'Source/WebCore/bindings/scripts/test/V8/V8Float64Array.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestActiveDOMObject.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestCustomNamedGetter.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestEventTarget.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestException.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestMediaQueryListListener.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestNode.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestOverloadedConstructors.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestSerializedScriptValueInterface.cpp', u'Source/WebCore/css/CSSStyleDeclaration.idl', u'Source/WebCore/dom/Clipboard.idl', u'Source/WebCore/dom/DOMStringMap.idl', u'Source/WebCore/html/DOMTokenList.idl', u'Source/WebCore/html/HTMLDocument.idl', u'Source/WebCore/html/HTMLImageElement.idl', u'Source/WebCore/html/HTMLSelectElement.idl', u'Source/WebCore/html/track/TextTrack.idl', u'Source/WebCore/svg/SVGPathSeg.idl', u'Source/WebCore/xml/XPathNSResolver.idl']" exit_code: 1
er names.  [readability/naming/underscores] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp:37:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/bindings/scripts/test/V8/V8TestNamedConstructor.cpp:39:  _ZTVN7WebCore20TestNamedConstructorE is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 26 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Sepez]

Comment on attachment 186091 [details]
Patch.

Ah.  Have found way to avoid style noise.  stay tuned.

[Thomas Sepez]

Created attachment 186107 [details]
Patch, fix style.

[Adam Barth]

Comment on attachment 186107 [details]
Patch, fix style.

View in context: https://bugs.webkit.org/attachment.cgi?id=186107&action=review

> Source/WebCore/Modules/webaudio/AudioDestinationNode.idl:31
> +    V8ValidNativeTypes=
> +        AudioDestinationNode
> +        DefaultAudioDestinationNode
> +        OfflineAudioDestinationNode,

Is this valid WebIDL syntax?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3256
> +    my @validNativeTypes = split(' ', GetValidNativeTypesForInterface($interface));

Why not use | as a separator?  That's what we do in other situations.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3259
> +#if defined(OS_WIN)

Do you mean OS(WINDOWS) ?

> Source/WebCore/html/HTMLDocument.idl:32
> +        FTPDirectoryDocument
> +        HTMLDocument
> +        HTMLViewSourceDocument
> +        ImageDocument
> +        MediaDocument
> +        PluginDocument
> +        SinkDocument
> +        TextDocument,

It's really lame to need to need to list all of these in the IDL file, especially without a compile-time check that we did it right.  For example, we could add a TextViewSourceDocument and not realize that we're causing trouble here.

[Thomas Sepez]

(In reply to comment #5)
> (From update of attachment 186107 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186107&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioDestinationNode.idl:31
> > +    V8ValidNativeTypes=
> > +        AudioDestinationNode
> > +        DefaultAudioDestinationNode
> > +        OfflineAudioDestinationNode,
> 
> Is this valid WebIDL syntax?

From WebIDL section 3.11: The ExtendedAttribute grammar symbol matches nearly any sequence of tokens, however the extended attributes defined in this document only accept a more restricted syntax. 

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3256
> > +    my @validNativeTypes = split(' ', GetValidNativeTypesForInterface($interface));
> 
> Why not use | as a separator?  That's what we do in other situations.

I wasn't able to make this work with your existing parsers.  For example, 
   V8ValidNativeTypes=AudioDestinationNode|DefaultAudioDestinationNode|OfflineAudioDestinationNode,

gives:
Next token should be ], but | at 4:     V8ValidNativeTypes=AudioDestinationNode|DefaultAudioDestinationNode|OfflineAudioDestinationNode, IDLParser.pm:1438 at ../bindings/scripts/IDLParser.pm line 129.

changing the definition of the attribute in the IDLAttributes.txt file didn't help, 
V8SkipVTableValidation=*|*
gives the same error.  The other cases I see of this in that .txt file all have fixed keywords between the |s, not wildcards.



> 
> > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3259
> > +#if defined(OS_WIN)
> 
> Do you mean OS(WINDOWS) ?

I might.  But I've been using OS_WIN successfully so far.

> 
> > Source/WebCore/html/HTMLDocument.idl:32
> > +        FTPDirectoryDocument
> > +        HTMLDocument
> > +        HTMLViewSourceDocument
> > +        ImageDocument
> > +        MediaDocument
> > +        PluginDocument
> > +        SinkDocument
> > +        TextDocument,
> 
> It's really lame to need to need to list all of these in the IDL file, especially without a compile-time check that we did it right.  For example, we could add a TextViewSourceDocument and not realize that we're causing trouble here.

Yep.  This is one of those lame things where we can't get the information from other sources, like the IDL (because there aren't IDL types for many of these) or like the c++ compiler (because this whole binding integrity thing works around its limited world view).  The alternative would be to have a 1-1 mapping between classes and IDL wrappers, but that seems painful now.

[Thomas Sepez]

Created attachment 187658 [details]
Patch, put implementation-specific checks in custom cpp files, not IDL.

[Adam Barth]

Comment on attachment 187658 [details]
Patch, put implementation-specific checks in custom cpp files, not IDL.

View in context: https://bugs.webkit.org/attachment.cgi?id=187658&action=review

I like the approach of having custom vtable validation.  I'm just worried about the maintainability of this approach.  Maybe we should have an "in" file like we use for HTMLNames that generates a header that declares these symbols?  We could potentially move the name mangling perl out of CodeGeneratorV8 into a separate model and share with whatever script processes that in file.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3241
> +#if defined(OS_WIN)

OS_WIN -> OS(WINDOWS)

I wonder if this should be COMPILER(MSVC) ?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:3265
> +#if defined(OS_WIN)

ditto

> Source/WebCore/bindings/v8/custom/V8AudioDestinationNodeCustom.cpp:34
> +#if defined(OS_WIN)

ditto

> Source/WebCore/bindings/v8/custom/V8AudioDestinationNodeCustom.cpp:42
> +extern "C" { extern void(*const __identifier("??_7AudioDestinationNode@WebCore@@6B@")[])(); }
> +extern "C" { extern void(*const __identifier("??_7DefaultAudioDestinationNode@WebCore@@6B@")[])(); }
> +extern "C" { extern void(*const __identifier("??_7OfflineAudioDestinationNode@WebCore@@6B@")[])(); }
> +#else
> +extern "C" { extern void* _ZTVN7WebCore20AudioDestinationNodeE[]; }
> +extern "C" { extern void* _ZTVN7WebCore27DefaultAudioDestinationNodeE[]; }
> +extern "C" { extern void* _ZTVN7WebCore27OfflineAudioDestinationNodeE[]; }

This is going to be hard to maintain.  How is a developer on Linux supposed to figure out the MSVC mangled symbol names?  Can we generate these names somehow?

[Thomas Sepez]

> > +#else
> > +extern "C" { extern void* _ZTVN7WebCore20AudioDestinationNodeE[]; }
> > +extern "C" { extern void* _ZTVN7WebCore27DefaultAudioDestinationNodeE[]; }
> > +extern "C" { extern void* _ZTVN7WebCore27OfflineAudioDestinationNodeE[]; }
> 
> This is going to be hard to maintain.  How is a developer on Linux supposed to figure out the MSVC mangled symbol names?  Can we generate these names somehow?

I was thinking about creating macros that would do the right thing on each platform, but the arguments get ugly -- since there is no way to take the length of a token in the preprocessor, one must pass a correct length to generate the unix symbol.

MSVC has no lengths, but it turns out the naming of symbols is complicated by having to qualify the symbol with the particular baseclass name when there is multiple inheritance from multiple classes containing vtables. So this would imply 2 forms for MSVC -- how the developer knows which one to pick is hard.

[Adam Barth]

What do you think about using an "in" file?

[Sam Weinig]

V8 is no longer supported in WebKit.