Bug 112191 – [v8] [meta] Binding Integrity should move off of vtable checks.

• Component: WebCore Misc.
• Status: RESOLVED INVALID

[Thomas Sepez]

Tracking bug for forthcoming effort.

[Thomas Sepez]

Created attachment 192811 [details]
WIP.

Patch for discussion of direction.

[Adam Barth]

Comment on attachment 192811 [details]
WIP.

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

> WebCore/page/History.idl:36
> +    IsScriptWrappable

We tried to avoid having an IDL attribute for script wrappable.  Instead we used C++ tricks to automatically detect a ScriptWrappable base class.

[Thomas Sepez]

(In reply to comment #2)
> (From update of attachment 192811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192811&action=review
> 
> > WebCore/page/History.idl:36
> > +    IsScriptWrappable
> 
> We tried to avoid having an IDL attribute for script wrappable.  Instead we used C++ tricks to automatically detect a ScriptWrappable base class.

Yeah.  It's hard, though, because the info is used to change the initialization of a const POD struct.  The bright side is only the classes that directly depend on scriptwrappable need to mention this, the rest is inferred via inherited IDL attributes.

[Adam Barth]

Comment on attachment 192811 [details]
WIP.

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

> WebCore/bindings/v8/ScriptWrappable.h:133
> +template <typename T>void initWrappable(T* object);

I thought we were going to do this in adoptPtr so that we wouldn't have to write this boilerplate code in every class...

[Adam Barth]

> Yeah.  It's hard, though, because the info is used to change the initialization of a const POD struct.  The bright side is only the classes that directly depend on scriptwrappable need to mention this, the rest is inferred via inherited IDL attributes.

That should be doable with template specialization, which is the trick we use elsewhere to autodetect ScriptWrappable.

[Thomas Sepez]

> I thought we were going to do this in adoptPtr so that we wouldn't have to write this boilerplate code in every class...

Apart from being counter-intuitive that adoptPtr would do this, it creates the need to have a single centralized file generated from the .idl that lists all of the possible entry adoptPtr varieties in one place.  This then gets included everywhere, such that a small perturbation in any idl file triggers a large rebuild.  Seemed opposite to the decentralized approach we seem to favor.

[Thomas Sepez]

Created attachment 192977 [details]
WIP.

Patch resolves the need for IsScriptWrappable IDL attribute.  We always generate the initWrappable() code; some of these will compile into nothingness.  Thing to note is that I moved some of the static methods from DOMDataStore.h to scriptwrapable and minor name changes.

[Adam Barth]

Comment on attachment 192977 [details]
WIP.

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

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:2020
> -    V8DOMWrapper::associateObjectWithWrapper(impl.release(), &V8${interfaceName}::info, wrapper, args.GetIsolate(), WrapperConfiguration::Dependent);
> +    V8DOMWrapper::associateObjectWithWrapper(impl.release().get(), &V8${interfaceName}::info, wrapper, args.GetIsolate(), WrapperConfiguration::Dependent);

impl.release().get()   <---  This is the same as impl.get()

> Source/WebCore/bindings/v8/NPV8Object.cpp:54
> -    static WrapperTypeInfo typeInfo = { 0, 0, 0, 0, 0, 0, 0, WrapperTypeObjectPrototype };
> +  static WrapperTypeInfo typeInfo = { 0, 0, 0, 0, 0, 0, 0, WrapperTypeObjectPrototype, 0 };

bad indent?

> Source/WebCore/bindings/v8/ScriptWrappable.h:71
> +   void setTypeInfo(const WrapperTypeInfo* info)

bad indent

[Adam Barth]

Ok, this looks workable.  Can you break this patch into smaller, reviewable pieces?  Maybe...

1) Move functions to ScriptWrappable.h
2) Generate any empty initScriptWrappable function and add callers
3) The rest

?

[Thomas Sepez]

(In reply to comment #9)
> Ok, this looks workable.  Can you break this patch into smaller, reviewable pieces?  Maybe...
> 
> 1) Move functions to ScriptWrappable.h
> 2) Generate any empty initScriptWrappable function and add callers
Strikes me this should be templated static method ScriptWrappable::init().

> 3) The rest
> 
> ?

[Thomas Sepez]

Created attachment 193207 [details]
WIP.

ScriptWrappable: note use of function template to call external function in place of function template with no body (c++ standard violation corrected).  Also note retrieval of typeinfo from wrapper when wrapper replaces typeinfo in slot.

[Adam Barth]

> Strikes me this should be templated static method ScriptWrappable::init().

Sure.

[Thomas Sepez]

Making this a meta bug.  Will be adding one last WIP set, then break this into smaller pieces.

[Thomas Sepez]

Created attachment 193663 [details]
WIP.

[Thomas Sepez]

Latest hangup: wrapping custom elements.  There is no HTMLCustomElement class from which to initialize a wrapper, which makes it hard to cover the special case for it that formerly existed in HTMLElementWrapperFactory.

[Adam Barth]

Maybe morrita has ideas for how to address the custom element issue?

[Hajime Morrita]

> Latest hangup: wrapping custom elements.  There is no HTMLCustomElement class from which to initialize a wrapper, which makes it hard to cover the special case for it that formerly existed in HTMLElementWrapperFactory.

In my understanding, you're telling WrapperTypeInfo to each C++ wrapable objects, right?
In that case, I don't think you need any extra work for custom elements.

As you mentioned, there is no C++ HTMLCustomElement class, 
There is no WrapperTypeInfo for custom element either. 
Each custom element is backed by one of either HTMLElement class or its subclasses,
both of which you already instrumented in the latest WIP patch.
So any custom element should already have a WrapperTypeInfo installed with the patch.

Does this make sense?

[Thomas Sepez]

Created attachment 194314 [details]
WIP.

Fix custom elements by upgrading cached wrapper type info when is=xxx attribute is parsed.

[Hajime Morrita]

Comment on attachment 194314 [details]
WIP.

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

> third_party/WebKit/Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:56
> +    object->setTypeInfo(&V8CustomElement::info);

It looks you are working on an old version. There is no V8CustomElement::info anymore.

[Hajime Morrita]

Comment on attachment 194314 [details]
WIP.

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

> third_party/WebKit/Source/WebCore/bindings/v8/V8CustomElement.cpp:47
> +WrapperTypeInfo V8CustomElement::info = { 0, 0, 0, 0, 0, 0, 0, WrapperTypeObjectPrototype, reinterpret_cast<WrapObjectFunction>(V8CustomElement::createWrapper) };

This is confusing. We have no CustomElement C++ class to be wrapped.
Custom elements are rather mechanism, not a single class.

I admit that name V8CustomElement was misleading.
This should've been named CustomElementConstructorInjector or something like that.

[Thomas Sepez]

(In reply to comment #19)
> (From update of attachment 194314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194314&action=review
> 
> > third_party/WebKit/Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:56
> > +    object->setTypeInfo(&V8CustomElement::info);
> 
> It looks you are working on an old version. There is no V8CustomElement::info anymore.
Actually, I re-introduced it.  I need a WrapperTypeInfo to hold a pointer to the createWrapper function that will invoke your constructor mechanism.  You're right that it should be named something else.

In the new world, custom elements, being elements and hence scriptwrappables, need to get a pointer to a wrappertypeinfo assigned at creation time.  It needs to be different based upon whether it's an ordinary or custom element (the latter invoking you constructor mechanism).

[Thomas Sepez]

Hajime, would it make sense to rename class V8CustomElement to V8CustomElementConstructorCaller (which seems closest to me to the actual intention)?

[Hajime Morrita]

(In reply to comment #21)
> 
> In the new world, custom elements, being elements and hence scriptwrappables, need to get a pointer to a wrappertypeinfo assigned at creation time.  It needs to be different based upon whether it's an ordinary or custom element (the latter invoking you constructor mechanism).

I'm sorry but I don't quite understand.
Currently we don't use WrapperTypeInfo to determine whether the element is custom element or not.
Why this change makes it required?

(In reply to comment #22)
> Hajime, would it make sense to rename class V8CustomElement to V8CustomElementConstructorCaller (which seems closest to me to the actual intention)?

Or we can just merge it CustomElementHelper.
File bug 113165 for this.

[Thomas Sepez]

> I'm sorry but I don't quite understand.
> Currently we don't use WrapperTypeInfo to determine whether the element is custom element or not.
> Why this change makes it required?
> 
Ah.  We want to harden the binding system against certain types of deliberate tampering, with the result that the wrapper factories, with their introspection, need to go away.  As such, a scriptwrappable will always get wrapped in the new world via indirection off of its pointer to a WrapperTypeInfo as set at object creation time (the wrapper type info is then amended to include the wrapper creation function pointer).

So, we need to have a stubb WrapperTypeInfo just to allow us to transfer control to the wrapper function  in the exact same manner as any other scriptwrappable without invoking additional logic.

[Thomas Sepez]

Created attachment 195765 [details]
WIP.

Keeping pace with changes to custom element helpers.

[Adam Barth]

Comment on attachment 195765 [details]
WIP.

This patch is too large.  I thought we talked about splitting this patch into smaller pieces.

[Brian Burg]

Closing some V8-related work items.