Bug 100229 – [Custom Elements] Implement bare-bone document.register()

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Implement document.register() with no option parameter support, which will come after this.

[Hajime Morrita]

Created attachment 170372 [details]
Patch

[Hajime Morrita]

If overall direction looks OK, I'll land some preparation parts as sub changes then
make this green.

[Dimitri Glazkov (Google)]

Comment on attachment 170372 [details]
Patch

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

I like the overall direction, I think it's very close to the spec. I would love for Haraken to look over the V8 code.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:349
> +    static v8::Persistent<v8::FunctionTemplate> GetTemplate(void* = 0);

Can you help me understand why this additional param is needed?

> Source/WebCore/bindings/v8/DOMFunction.h:49
> +class DOMFunction : public RefCounted<DOMFunction>

DOMFunction seems like a bad name. There aren't any DOM functions :)

Also, it seems that it's not really used in itself, only as DOMElementConstructor. Maybe we could elide it in to DOMElementConstructor altogether?

> Source/WebCore/dom/Document.cpp:723
> +        clearSupplements();

This seems wrong. Why didn't we need this before? Smells like an architectural flaw somewhere in this patch.

> Source/WebCore/dom/DocumentCustomElement.h:52
> +class DocumentCustomElement : public Supplement<ScriptExecutionContext> {

I wonder if this can be combined with CustomElementRegistry?

[Hajime Morrita]

Comment on attachment 170372 [details]
Patch

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

Hi Dimiri, thanks for the quick reqview. That helped!

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:349
>> +    static v8::Persistent<v8::FunctionTemplate> GetTemplate(void* = 0);
> 
> Can you help me understand why this additional param is needed?

We need to return different FunctionTemplate for each custom element definition.
But we cannot generate different C++ function for each of them.
So this change passes DOMElemenntConstructor as a parameter so that the function
can look the associated FunctionTemplate object.

For built-in classes, each of them needs only one FunctionTemplate.

>> Source/WebCore/bindings/v8/DOMFunction.h:49
>> +class DOMFunction : public RefCounted<DOMFunction>
> 
> DOMFunction seems like a bad name. There aren't any DOM functions :)
> 
> Also, it seems that it's not really used in itself, only as DOMElementConstructor. Maybe we could elide it in to DOMElementConstructor altogether?

Good point. I'll kill this.
I made this just because I'd like to have a counterpart of WebIDL "Function" interface.
But it now seems overkill.

>> Source/WebCore/dom/Document.cpp:723
>> +        clearSupplements();
> 
> This seems wrong. Why didn't we need this before? Smells like an architectural flaw somewhere in this patch.

While writing a response, I actually found a flaw and came up with an alternative idea.
So this will be removed. Thanks for reviewing ;-)

>> Source/WebCore/dom/DocumentCustomElement.h:52
>> +class DocumentCustomElement : public Supplement<ScriptExecutionContext> {
> 
> I wonder if this can be combined with CustomElementRegistry?

Good point. Will do. I'll merge CustomElementRegistry to this class since
I'm hesitating to have CustomElementRegistry.idl.

[Hajime Morrita]

Created attachment 170596 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Dimitri Glazkov (Google)]

Comment on attachment 170596 [details]
Patch

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

> Source/WebCore/bindings/v8/V8PerContextData.h:87
> +    void addDisposable(V8ContextDisposable* disposable) { m_disposables.append(disposable); }

Abarth needs to look a this.

> Source/WebCore/dom/DocumentCustomElement.h:51
> +class DocumentCustomElement : public Supplement<ScriptExecutionContext> {

This seems like a bad name... It's neither Document nor Custom element :) I liked CustomElementRegistry... can we bring it back?

[Adam Barth]

Comment on attachment 170596 [details]
Patch

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

Have you run performance tests on this patch?  It seems like it might introduce slowdowns to document.createElement and possibly to parsing.

> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:47
> +    return elementTemplate()->GetFunction();

It seems very strange that you're ignoring the creationContext and the isolate.

> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:122
> +    v8::Handle<v8::FunctionTemplate> desc = createRawTemplate();

desc -> please use complete words in variable names.

> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:124
> +    return v8::Persistent<v8::FunctionTemplate>::New(desc);

Can we use ScopedPersistent rather than manually calling Persistent::New ?

> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:130
> +    ref();

Manually ref()/deref() is bad times.

> Source/WebCore/bindings/v8/WrapperTypeInfo.h:48
> -    typedef v8::Persistent<v8::FunctionTemplate> (*GetTemplateFunction)();
> +    typedef v8::Persistent<v8::FunctionTemplate> (*GetTemplateFunction)(void*);
> +    typedef void* GetTemplateFunctionParameterType;

Why is this a void*?  Is there a more semantic type we can use?

> Source/WebCore/dom/CustomElementDefinition.cpp:57
> +    willDestroyDocument();

What does ~CustomElementDefinition have to do with destroying documents?

> Source/WebCore/dom/DocumentCustomElement.cpp:89
> +PassRefPtr<DOMElementConstructor> DocumentCustomElement::webkitRegister(Document* document, const AtomicString& name, ExceptionCode& ec)

webkitRegister -> please use unprefixed names in the implementation and use ImplementedAs in the IDL.

>> Source/WebCore/dom/DocumentCustomElement.h:51
>> +class DocumentCustomElement : public Supplement<ScriptExecutionContext> {
> 
> This seems like a bad name... It's neither Document nor Custom element :) I liked CustomElementRegistry... can we bring it back?

Is there a reason to use Supplement for this feature?  It seems like its a core part of DOM and it should be implemented directly on Document.

[Adam Barth]

I need to digest this patch a bit.  There's a lot of unusual stuff in there that I'd like to think about.

[Hajime Morrita]

Comment on attachment 170596 [details]
Patch

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

>> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:47
>> +    return elementTemplate()->GetFunction();
> 
> It seems very strange that you're ignoring the creationContext and the isolate.

I'll remove them. Since the GetFunction() doesn't take them so we have no place to use it anyway.

>> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:124
>> +    return v8::Persistent<v8::FunctionTemplate>::New(desc);
> 
> Can we use ScopedPersistent rather than manually calling Persistent::New ?

Oh, I wasn't aware of that. Will do.

>> Source/WebCore/bindings/v8/DOMElementConstructor.cpp:130
>> +    ref();
> 
> Manually ref()/deref() is bad times.

Yeah, this is a pain point. I have another idea so will try it.

>> Source/WebCore/bindings/v8/WrapperTypeInfo.h:48
>> +    typedef void* GetTemplateFunctionParameterType;
> 
> Why is this a void*?  Is there a more semantic type we can use?

Hmm. This actually points DOMElementConstructdor. But I don't want to propagate that fact to binding global.
Maybe providing a superclass is sufficient.

>> Source/WebCore/dom/CustomElementDefinition.cpp:57
>> +    willDestroyDocument();
> 
> What does ~CustomElementDefinition have to do with destroying documents?

DOMElementConstructor, which is owned by a document and a V8 object, has a pointer to the document.
We need to clear the pointer since DOMElementConstructor can survive after the host document is dead due to the nature of GC.

>> Source/WebCore/dom/DocumentCustomElement.cpp:89
>> +PassRefPtr<DOMElementConstructor> DocumentCustomElement::webkitRegister(Document* document, const AtomicString& name, ExceptionCode& ec)
> 
> webkitRegister -> please use unprefixed names in the implementation and use ImplementedAs in the IDL.

Sure, I wasn't aware of implementAs. "register" is a keyword of C++ so I'll pick some reasonable-looking name.

>>> Source/WebCore/dom/DocumentCustomElement.h:51
>>> +class DocumentCustomElement : public Supplement<ScriptExecutionContext> {
>> 
>> This seems like a bad name... It's neither Document nor Custom element :) I liked CustomElementRegistry... can we bring it back?
> 
> Is there a reason to use Supplement for this feature?  It seems like its a core part of DOM and it should be implemented directly on Document.

Hmm. I felt uncomfortable with messing up Document with #ifdefs anymore.
But in this case you are right.

[Hajime Morrita]

> I need to digest this patch a bit.  There's a lot of unusual stuff in there that I'd like to think about.
Thanks!

In my feeling, what notably different is that
- FunctionTemplate and WrapperTypeInfo are no longer static variables.
  They are allocated dynamically for custom elements and we need take 
  care of its lifetime.
- GetTemplate() is no longer stateless. It needs some associated context.

[Adam Barth]

Yeah, I need to think about the implications of those things, especially WrapperTypeInfo not being static.  In principle, we should be able to do this with only static WrapperTypeInfo objects.

[Adam Barth]

Comment on attachment 170596 [details]
Patch

I've now read the spec and gone over this patch a couple times.  I don't really understand how this works.  Maybe I need to study it more or perhaps a ChangeLog entry would help.

[Dimitri Glazkov (Google)]

(In reply to comment #13)
> (From update of attachment 170596 [details])
> I've now read the spec and gone over this patch a couple times.  I don't really understand how this works.  Maybe I need to study it more or perhaps a ChangeLog entry would help.

Sounds like we need a design doc or somesuch.

[Adam Barth]

> Sounds like we need a design doc or somesuch.

Yeah, I can also try implementing it myself to see how each of these issues come up.

[Adam Barth]

Ok, I'm starting to understand.  You can seem my work here:

https://github.com/abarth/webkit/compare/master...document-register

[Adam Barth]

I now understand why you went down the design path you did in your patch.  I think your patch has the same problems with memory leaks that I noted in my GitHub branch.

I still think we can avoid dynamically creating WrapperTypeInfo structs.  I need to work the code a bit more to figure it out.  Basically, we need to work at a lower layer (beneath WrapperTypeInfo, if you like), probably by calling V8ObjectConstructor functions directly.

We also need to solve the lifetime issue, which is going to be tricky.  Basically, whatever object we use to hold on to the options.prototype (whether that's the prototype object itself or the Function we return from document.register) is going to need to be a weak pointer that uses the Document as its opaqueRootForGC.

Question for dglazkov: What is supposed to happen in the following situation:

var a = {};
a.which = "a";
a.__proto__ = HTMLElement.prototype;

var b = {};
b.which = "b";
b.__proto__ = HTMLElement.prototype;

var func = document.register("x-foo", {
  prototype: a
});

var createdBefore = document.createElement("x-foo");
var constructedBefore = new func();

alert(createdBefore.which);  // I assume this alerts "a".
alert(constructedBefore.which);  // I assume this alerts "a" as well.

func.prototype = b;

var createdAfter = document.createElement("x-foo");
var constructedAfter = new func();

alert(createdAfter.which);  // ???
alert(constructedAfter.which);  // ???

document.body.appendChild(createdAfter);  // I assume this works.
document.body.appendChild(constructedAfter);  // Does this work?

var c = {};
c.which = "c";

func.prototype = c;

var constructedMuchLater = new func();

document.body.appendChild(constructedMuchLater);  // Does this work?

[Hajime Morrita]

Adam, thanks for your investigation! I looked into the prototype.

Note that it isn't enough to just re-set the prototype for each wrapper.
We also need to inherit v8::Accessor from the parent prototype's FunctionTemplate
so that we have IDL-defined properties (attributes) on the wrapper JS object.

On WrapperTypeInfo allocation, we possibly able to pass the caller context to GetTemplate()
so that either GetTemplate or WrapperTypeInfo doen't need to know its own context, 
which requires the dynamic allocation.
The context will be an implementation object to be wrapped.
The GetTemplate() could lookup the corresponding function template object based on the 
implementation object.

On lifetime of ScriptValue (in github version)/FunctionTemplate (in my version), 
Probably we could make the reference weak and let Document's visitation callback to traverse 
the weak reference. I need to learn how these cross-boundary lifecycle work though.

I'll revise the patch next week once I come back from TPAC.

[Hajime Morrita]

By the way, we should mark the prototype of the constructor function read-only. 
That is how constructors of built-in elements work.

[Adam Barth]

@dglazkov: How is document.register supposed to work for extension content scripts?

== main world ==

document.register("x-foo", ...)
document.body.appendChild(document.createElement("x-foo"));

== content script ==

var x = document.body.lastChild.__proto__;

Is "x" an object from the main world?  That would violate the security requirement that JavaScript objects do not cross from one world to another.

Similarly, what happens if the content script tries to register an element?  Will that leak an object from the content script to the mail world?

Are the lifetime callbacks made once for each element or once for each element in each world?  If the former, then the wrapper in the isolated world probably won't get whatever special sauce the lifetime callback sets up.  If the former, the lifetime callbacks are going to get very confused because they'll be running on a wrapper object from another world...

[Adam Barth]

> Note that it isn't enough to just re-set the prototype for each wrapper.
> We also need to inherit v8::Accessor from the parent prototype's FunctionTemplate
> so that we have IDL-defined properties (attributes) on the wrapper JS object.

I think that should work fine in my branch.  V8HTMLElement::wrap should construct the wrapper JS object with the right v8::Accessor object.  I haven't tested it, so maybe there's some subtly I don't understand.

> On WrapperTypeInfo allocation, we possibly able to pass the caller context to GetTemplate()
> so that either GetTemplate or WrapperTypeInfo doen't need to know its own context, 
> which requires the dynamic allocation.

I don't understand why we need to create a new template for these wrappers.  We should be able to use the HTMLElement template and then suitably modify the wrapper.

> On lifetime of ScriptValue (in github version)/FunctionTemplate (in my version), 
> Probably we could make the reference weak and let Document's visitation callback to traverse 
> the weak reference. I need to learn how these cross-boundary lifecycle work though.

Yes, the reference needs to be weak, and we need to integrate with V8GCController's GC prologue.

> I'll revise the patch next week once I come back from TPAC.

Sounds good.  This is going to be a very tricky patch.

[Hajime Morrita]

(In reply to comment #21)
> > Note that it isn't enough to just re-set the prototype for each wrapper.
> > We also need to inherit v8::Accessor from the parent prototype's FunctionTemplate
> > so that we have IDL-defined properties (attributes) on the wrapper JS object.
> 
> I think that should work fine in my branch.  V8HTMLElement::wrap should construct the wrapper JS object with the right v8::Accessor object.  I haven't tested it, so maybe there's some subtly I don't understand.

It seems you are right. I'm sorry for my confusing comment.

> 
> > On WrapperTypeInfo allocation, we possibly able to pass the caller context to GetTemplate()
> > so that either GetTemplate or WrapperTypeInfo doen't need to know its own context, 
> > which requires the dynamic allocation.
> 
> I don't understand why we need to create a new template for these wrappers.  We should be able to use the HTMLElement template and then suitably modify the wrapper.
> 

I thought it is the only way to give a (C++-implemented) constructor callback for
each returning constructor function. Other options?


> > On lifetime of ScriptValue (in github version)/FunctionTemplate (in my version), 
> > Probably we could make the reference weak and let Document's visitation callback to traverse 
> > the weak reference. I need to learn how these cross-boundary lifecycle work though.
> 
> Yes, the reference needs to be weak, and we need to integrate with V8GCController's GC prologue.
> 
> > I'll revise the patch next week once I come back from TPAC.
> 
> Sounds good.  This is going to be a very tricky patch.

[Adam Barth]

> I thought it is the only way to give a (C++-implemented) constructor callback for
> each returning constructor function. Other options?

I haven't looked at that closely yet.  I would try creating a C++ object that has a wrapper.  We'd make the object callable so that the web page could call it as a constructor.  I think it auto-magically gets typeof "function" when it becomes callable.

[Hajime Morrita]

(In reply to comment #23)
> > I thought it is the only way to give a (C++-implemented) constructor callback for
> > each returning constructor function. Other options?
> 
> I haven't looked at that closely yet.  I would try creating a C++ object that has a wrapper.  We'd make the object callable so that the web page could call it as a constructor.  I think it auto-magically gets typeof "function" when it becomes callable.

Wow, found that there is ObjectTemplate::SetCallAsFunctionHandler(). I can use it!
Thanks for your advice :-)

[Dimitri Glazkov (Google)]

(In reply to comment #17)
> I now understand why you went down the design path you did in your patch.  I think your patch has the same problems with memory leaks that I noted in my GitHub branch.
> 
> I still think we can avoid dynamically creating WrapperTypeInfo structs.  I need to work the code a bit more to figure it out.  Basically, we need to work at a lower layer (beneath WrapperTypeInfo, if you like), probably by calling V8ObjectConstructor functions directly.
> 
> We also need to solve the lifetime issue, which is going to be tricky.  Basically, whatever object we use to hold on to the options.prototype (whether that's the prototype object itself or the Function we return from document.register) is going to need to be a weak pointer that uses the Document as its opaqueRootForGC.
> 
> Question for dglazkov: What is supposed to happen in the following situation:
> 
> var a = {};
> a.which = "a";
> a.__proto__ = HTMLElement.prototype;
> 
> var b = {};
> b.which = "b";
> b.__proto__ = HTMLElement.prototype;
> 
> var func = document.register("x-foo", {
>   prototype: a
> });
> 
> var createdBefore = document.createElement("x-foo");
> var constructedBefore = new func();
> 
> alert(createdBefore.which);  // I assume this alerts "a".
> alert(constructedBefore.which);  // I assume this alerts "a" as well.

Yes in both cases. "func" is an element constructor.

> 
> func.prototype = b;
> 
> var createdAfter = document.createElement("x-foo");
> var constructedAfter = new func();
> 
> alert(createdAfter.which);  // ???
> alert(constructedAfter.which);  // ???

Per spec today, it will still be "a", because constructor closes over the original prototype and uses it to instantiate the element:

http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/custom/index.html#dfn-custom-element-constructor-generation

> 
> document.body.appendChild(createdAfter);  // I assume this works.
> document.body.appendChild(constructedAfter);  // Does this work?

Yup. both work.

> 
> var c = {};
> c.which = "c";
> 
> func.prototype = c;
> 
> var constructedMuchLater = new func();
> 
> document.body.appendChild(constructedMuchLater);  // Does this work?

Still uses a.

These are good tricky questions. In your opinion, does the specified behavior make sense?

[Adam Barth]

> These are good tricky questions. In your opinion, does the specified behavior make sense?

Yes!  They're the best answers from an implementation point of view.  What they tell me is that we need to hold m_prototype with a lifetime equal to that of document and that the function that we return from document.register is decoupled from the element creation / wrapping process.

[Hajime Morrita]

(In reply to comment #24)
> > I haven't looked at that closely yet.  I would try creating a C++ object that has a wrapper.  We'd make the object callable so that the web page could call it as a constructor.  I think it auto-magically gets typeof "function" when it becomes callable.
> 
> Wow, found that there is ObjectTemplate::SetCallAsFunctionHandler(). I can use it!
> Thanks for your advice :-)

Haraken pointed out that the object cannot be a function even if it is callable.
This is right. Functions need to have apply, bind and other built-in method but
function-like callable objects don't have these. I'll look into some other path.

[Kentaro Hara]

(In reply to comment #27)
> (In reply to comment #24)
> > > I haven't looked at that closely yet.  I would try creating a C++ object that has a wrapper.  We'd make the object callable so that the web page could call it as a constructor.  I think it auto-magically gets typeof "function" when it becomes callable.
> > 
> > Wow, found that there is ObjectTemplate::SetCallAsFunctionHandler(). I can use it!
> > Thanks for your advice :-)
> 
> Haraken pointed out that the object cannot be a function even if it is callable.
> This is right. Functions need to have apply, bind and other built-in method but
> function-like callable objects don't have these. I'll look into some other path.

In a long term, a correct way to fix the problem would be to have v8::Function inherit from v8::Object and have v8::FunctionTemplate inherit from v8::ObjectTemplate. I know you need a more short-term solution though.

[Adam Barth]

> Haraken pointed out that the object cannot be a function even if it is callable.
> This is right. Functions need to have apply, bind and other built-in method but
> function-like callable objects don't have these. I'll look into some other path.

You're going to need to set the __proto__ property anyway, so that shouldn't be a problem.

[Hajime Morrita]

(In reply to comment #29)
> > Haraken pointed out that the object cannot be a function even if it is callable.
> > This is right. Functions need to have apply, bind and other built-in method but
> > function-like callable objects don't have these. I'll look into some other path.
> 
> You're going to need to set the __proto__ property anyway, so that shouldn't be a problem.

It looks implementations of built-in functions don't accept function-like objects.
I tried Function.prototype.apply(funcLike, null, []) then it throws an error that says
funcLike should be a function but should not be an object.

[Adam Barth]

> It looks implementations of built-in functions don't accept function-like objects.

That's too bad.

> I tried Function.prototype.apply(funcLike, null, []) then it throws an error that says
> funcLike should be a function but should not be an object.

How did you create funcLike?  Is typeof funcLike == "function" ?

[Hajime Morrita]

(In reply to comment #31)
> > It looks implementations of built-in functions don't accept function-like objects.
> 
> That's too bad.
> 
> > I tried Function.prototype.apply(funcLike, null, []) then it throws an error that says
> > funcLike should be a function but should not be an object.
> 
> How did you create funcLike?  Is typeof funcLike == "function" ?

No, it's a object which has callAsFunction (or callAsConstructor) callback.
One example is that HTMLEmbedElement.

[Hajime Morrita]

Created attachment 174633 [details]
Patch

[Hajime Morrita]

This version follows Adam's suggestion in general sense, with a tweak
to support function semantics of register()-ed constructors.
I'll write a design doc. This one is, IMO, simpler than the last version though.

[Hajime Morrita]

Note that this this patch is a bit outdated since toV8() and toNative() is under big refactoring and
I don't want to fight against per-rebase merges.

[Hajime Morrita]

Comment on attachment 174633 [details]
Patch

oops. This isn't ready for review yet.

[Hajime Morrita]

Created attachment 175327 [details]
Patch

[Hajime Morrita]

This one is almost ready to review. But before that, I need to announce the feature to webkit-dev.

[Build Bot]

Comment on attachment 175327 [details]
Patch

Attachment 175327 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14913830

[WebKit Review Bot]

Comment on attachment 175327 [details]
Patch

Attachment 175327 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14908789

New failing tests:
editing/inserting/insert-composition-whitespace.html
editing/pasteboard/paste-and-sanitize.html
dom/xhtml/level3/core/documentadoptnode16.xhtml
dom/xhtml/level1/core/hc_documentcreateelementcasesensitive.xhtml
accessibility/canvas-fallback-content.html
editing/execCommand/remove-format-elements.html
editing/execCommand/applyblockelement-visiblepositionforindex-crash.html
dom/xhtml/level3/core/nodeisdefaultnamespace09.xhtml
dom/html/level1/core/hc_elementwrongdocumenterr.html
dom/html/level1/core/hc_attrinsertbefore6.html
editing/selection/extend-by-sentence-001.html
dom/html/level1/core/hc_attrappendchild5.html
editing/deleting/delete-and-cleanup.html
dom/html/level1/core/hc_nodeappendchildnewchilddiffdocument.html
dom/html/level1/core/hc_namednodemapwrongdocumenterr.html
dom/xhtml/level3/core/documentnormalizedocument10.xhtml
accessibility/aria-hidden-updates-alldescendants.html
editing/editability/ignored-content.html
dom/html/level1/core/hc_attrappendchild2.html
dom/html/level1/core/hc_nodeinsertbeforenewchilddiffdocument.html
editing/undo/replace-text-in-node-preserving-markers-crash.html
editing/pasteboard/paste-without-nesting.html
http/tests/misc/script-defer-after-slow-stylesheet.html
editing/style/remove-format-without-enclosing-block.html
editing/execCommand/ident-crashes-topnode-is-text.html
editing/selection/root-inlinebox-selected-children-crash.html
fast/block/child-not-removed-from-parent-lineboxes-crash.html
dom/xhtml/level1/core/hc_attrappendchild2.xhtml
fast/block/merge-anonymous-block-remove-child-crash2.html
dom/html/level1/core/hc_nodereplacechildnewchilddiffdocument.html

[Hajime Morrita]

Created attachment 175349 [details]
Patch

[Build Bot]

Comment on attachment 175349 [details]
Patch

Attachment 175349 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14901862

New failing tests:
fast/dom/custom/document-register-basic.html

[Hajime Morrita]

Created attachment 175368 [details]
Patch

[Adam Barth]

Comment on attachment 175368 [details]
Patch

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

I'm sorry, but I ran out of time to look at this patch.  This version looks like a big improvement over the previous version.  I've left some comments below about ways in which we might be able to improve it further.

> Source/WebCore/ChangeLog:29
> +        This chagne tweaks make_names.pl (or generated HTMLElementFactory)

chagne -> change

> Source/WebCore/ChangeLog:49
> +        [JS Adaptor Fuction] <-(hidden property)-> [JS Wrapper Object] -(internal field)-> [C++ Natie object]

Is there some reason we can't use the JS Adaptor Function as the wrapper for the native C++ object?  I guess I need to read the patch.

typo: Natie -> Native.

> Source/WebCore/ChangeLog:70
> +

Thank you for the detailed ChangeLog entry.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:43
> +    DEFINE_STATIC_LOCAL(ScriptValue, value, (v8::Persistent<v8::Value>::New(V8HTMLSpanElement::GetTemplate()->GetFunction()->Get(v8::String::NewSymbol("prototype")))));

This can't be right.  It needs to be per-context, not static (or even per-isolate).

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:45
> +v8::Persistent<v8::FunctionTemplate> V8AdaptorFunction::configureTemplate(v8::Persistent<v8::FunctionTemplate> desc)

desc -> template?

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:57
> +    Vector<v8::Handle<v8::Value> > argArray(args.Length());
> +    for (int i = 0; i < args.Length(); ++i)
> +        argArray.append(args[i]);

It's lame that we have to copy the arguments like this.  Can we not get a pointer and length from v8::Arguments?  We might want to add that API.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:63
> +v8::Handle<v8::Function> V8AdaptorFunction::wrap(v8::Handle<v8::Object> object, v8::Handle<v8::Value> prototype, AtomicString name)

AtomicString -> const AtomicString& ?

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:68
> +    adaptor->SetName(v8::String::New(name.characters(), name.length()));

Why not use v8String rather than v8::String::New?  It's much faster.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:70
> +    adaptor->SetHiddenValue(V8HiddenPropertyName::detail(), object);
> +    object->SetHiddenValue(V8HiddenPropertyName::detail(), adaptor);

This is also very slow.  Perhaps we should configure the function template to have an internal field we can use here.

It still seems to me like we can use the function object directly as a JS wrapper and avoid this adaptor thing.  What's stopping us from doing that?

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:59
> +    v8::Handle<v8::Context> context;
> +    if (!creationContext.IsEmpty() && creationContext->CreationContext() != v8::Context::GetCurrent()) {
> +        // For performance, we enter the context only if the currently running context
> +        // is different from the context that we are about to enter.
> +        context = v8::Local<v8::Context>::New(creationContext->CreationContext());
> +        ASSERT(!context.IsEmpty());
> +        context->Enter();
> +    }

THis is really old code now.  instantiateV8Object handles this these days.

[Hajime Morrita]

Created attachment 185141 [details]
Patch

[Hajime Morrita]

Comment on attachment 185141 [details]
Patch

Back from long quiet period...

I addressed Adam's comment if possible and added explaining FIXMEs / filed bugs for points I don't want to address in this patch.
I'll announce the feature in webkit-dev once this patch becomes technically sensible for given standard.

[Hajime Morrita]

Created attachment 185147 [details]
Renamed the feature flag

[Hajime Morrita]

Comment on attachment 185147 [details]
Renamed the feature flag

cq- until the feature announcement is made.

[Adam Barth]

Comment on attachment 185147 [details]
Renamed the feature flag

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

> Source/WebCore/ChangeLog:9
> +        - The feature is guarded by ENABLE(CUSTOM_DOM_ELMENTS) and RuntimeEnabledFeatures::customDOMElementsEnabled().

CUSTOM_DOM_ELMENTS -> this name doesn't match the code

> Source/WebCore/ChangeLog:66
> +        Although this first version doesn't support the optinal prototype

optinal -> optional

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:46
> +    return ScriptValue(frame->script()->windowShell(mainThreadNormalWorld())->customElementDefaultPrototype());

mainThreadNormalWorld -> This probably won't work properly with content scripts that run in another isolated world.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:49
> +bool CustomElementHelpers::isCustomElementsAllowed(Frame* frame)

Ah, I see.  :)

Can we assert isCustomElementsAllowed in defaultPrototypeOf?

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:53
> +    return mainWorldScriptState(frame) == ScriptState::current();

There's probably a faster way to check this condition.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:40
> +    // FIXME: Should be stored in per-isolate data store

Yes...  We now pass the isolate around a lot in the bindings.  It should be easy to pass it to this function.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:66
> +    v8::Handle<v8::Function> adaptor = v8::Handle<v8::Function>::Cast(getTemplate()->GetFunction());

getTemplate() <-- Seems like we could pass the isolate here.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:68
> +        adaptor->Set(v8::String::NewSymbol("prototype"), prototype, v8::ReadOnly);

Do we intend to call the setter for "prototype" on Object.prototype ?

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:71
> +    adaptor->SetHiddenValue(V8HiddenPropertyName::detail(), object);
> +    object->SetHiddenValue(V8HiddenPropertyName::detail(), adaptor);

detail <-- We should probably create a new V8HiddenPropertyName for this purpose.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:491
> +    ScopedPersistent<v8::Function> spanConstructor(v8::Handle<v8::Function>::Cast(m_global->Get(v8String("HTMLSpanElement", m_isolate))));

Cast <-- This might be a bad cast.  Someone might have messed with the HTMLSpanElement property.  We need to check whether the object we get back from m_global really is a v8::Function before casting it.

Also, I don't think there's any reason to use ScopedPersistent here.  We can just use v8::Handle.

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:492
> +    m_customElementDefaultPrototype.set(v8::Handle<v8::Object>::Cast(spanConstructor->Get(v8String("prototype", m_isolate))));

Again, this might be a bad cast.  We need to check that we actually get a v8::Object.

> Source/WebCore/bindings/v8/V8DOMWindowShell.h:115
> +    ScopedPersistent<v8::Object> m_customElementDefaultPrototype;

Can this cause a memory leak?  What prevents m_customElementDefaultPrototype from retaining V8DOMWindowShell?  We probably need to clear this out in disposeContext() or clearForNavigation/clearForClose or something.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:44
> +// FIXME: Each custom elements should have its own GetTemplate method so that it can derived from differenct super element.

differenct -> different

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:49
> +    CustomElementConstructor* ctor = CustomElementRegistry::constructorOf(impl.get());

ctor -> constructor

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:51
> +        return v8::Handle<v8::Object>::Cast(WebCore::toV8(toHTMLUnknownElement(impl.get()), creationContext, isolate));

Cast <-- Are you sure this cast is valid?  I guess we know that impl is non-zero?

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:59
> +    if (CustomElementHelpers::isCustomElementsAllowed(impl->document()->frame())) {

We shouldn't need to talk to the Frame here.  The document() and its DOMWindow should be enough for us.  Consider the case when the Frame has navigated away from impl->document().

> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:51
> +    return V8HTMLCustomElement::toV8(element.get());

What about the creationContext and the isolate parameter?  We shouldn't be omitting these parameters in new code, especially not for elements.  In this case, args.Holder() should be the creationContext.

> Source/WebCore/dom/CustomElementConstructor.h:54
> +    explicit CustomElementConstructor(Document*, const QualifiedName&, const String&, const ScriptValue&);

explicit <-- This keyword is only useful on one-argument constructors.

CustomElementConstructor <-- This constructor should be private because all callers should use the "create" function instead.

> Source/WebCore/dom/CustomElementConstructor.h:59
> +    AtomicString name() const { return m_name; }

AtomicString -> const AtomicString&

> Source/WebCore/dom/CustomElementConstructor.h:69
> +    ScriptValue m_prototype;

Isn't this a memory leak?

> Source/WebCore/dom/CustomElementRegistry.h:72
> +    ScriptValue m_defaultPrototype;

This is probably also a memory leak.  What if someone runs the following code:

HTMLSpanElement.prototype.foo = document

Now we have a cycle and we'll leak the entire document.

[Adam Barth]

Comment on attachment 185147 [details]
Renamed the feature flag

This patch has lots of memory leaks.

[Hajime Morrita]

Comment on attachment 185147 [details]
Renamed the feature flag

Thanks for the quick and late night review, Adam!
I realized my change is behind recent "passing-isolate" effort.
And also noticed that there are many cycles. Probably I need to make some references weak to break them.

[Adam Barth]

Yeah, it's going to be slightly tricky to break the cycles.  One thing you have going for you is that the registration created by document.register lasts for the lifetime of the document.  In your patch, you've interpreted that to mean until the document is destroyed.  However, it might be easier to interpret that to mean "as long as the document is displayed in a frame".  For example, you could clear out the registry when ActiveDOMObject::stop is called.  That might be an easier approach that adding a lot of complicated weak handle logic.

[Hajime Morrita]

Comment on attachment 185147 [details]
Renamed the feature flag

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

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:49
>> +bool CustomElementHelpers::isCustomElementsAllowed(Frame* frame)
> 
> Ah, I see.  :)
> 
> Can we assert isCustomElementsAllowed in defaultPrototypeOf?

Yes, we should.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:68
>> +        adaptor->Set(v8::String::NewSymbol("prototype"), prototype, v8::ReadOnly);
> 
> Do we intend to call the setter for "prototype" on Object.prototype ?

Good point. The standard says nothing particular. I'd support to allow it considering that even object serialization does allow it.

>> Source/WebCore/bindings/v8/V8DOMWindowShell.h:115
>> +    ScopedPersistent<v8::Object> m_customElementDefaultPrototype;
> 
> Can this cause a memory leak?  What prevents m_customElementDefaultPrototype from retaining V8DOMWindowShell?  We probably need to clear this out in disposeContext() or clearForNavigation/clearForClose or something.

Right. I should figure out some trigger to start cycle breaking as you suggested.

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:51
>> +        return v8::Handle<v8::Object>::Cast(WebCore::toV8(toHTMLUnknownElement(impl.get()), creationContext, isolate));
> 
> Cast <-- Are you sure this cast is valid?  I guess we know that impl is non-zero?

Yes, well, no. I updated the caller. The condition also should be ASSERT-ed.

[Hajime Morrita]

Created attachment 185411 [details]
Patch

[Hajime Morrita]

Comment on attachment 185411 [details]
Patch

This change gets rid of ScriptValue member variables to prevent cycles.
The reference to JavaScript objects are pushed into wrapper object side. 
For example, CustomElementConstructor::m_prototype in the last version is moved to
"prototype" property of the wrapper object.

[Dominic Cooney]

> >> Source/WebCore/bindings/v8/V8DOMWindowShell.h:115
> >> +    ScopedPersistent<v8::Object> m_customElementDefaultPrototype;
> > 
> > Can this cause a memory leak?  What prevents m_customElementDefaultPrototype from retaining V8DOMWindowShell?  We probably need to clear this out in disposeContext() or clearForNavigation/clearForClose or something.
> 
> Right. I should figure out some trigger to start cycle breaking as you suggested.

What about the venerable hidden reference on the V8 side? ie make the C++ handle weak, and put a hidden reference from the document wrapper to the prototype?

[Dominic Cooney]

Comment on attachment 185411 [details]
Patch

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

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:48
> +        wrapper->Set(v8String("prototype", state->context()->GetIsolate()), prototypeValue, v8::ReadOnly);

Does v8::ReadOnly mean not writable, not configurable? If not this code needs hardening.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:57
> +    v8::Handle<v8::Value> interfaceObject = global->Get(v8String("HTMLSpanElement", isolate));

Isn’t it easy for the script author to set this to anything that they want?

It would be better to consult V8HTMLSpanElement directly to get the interface object, then the interface prototype object.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:66
> +bool CustomElementHelpers::isCustomElementsAllowed(ScriptState* state)

It seems grammatically weird to have is with plural elements. But maybe the pull of is for a boolean accessor is irresistibile?

> Source/WebCore/bindings/v8/V8AdaptorFunction.h:47
> +//      WebKit need it to associate each wrapper to its backing C++ object. We store it in an internal field of the wrapped object.

It looks like you use a hidden value, which is slightly different to an internal field.

> Source/WebCore/bindings/v8/V8AdaptorFunction.h:49
> +// Once 1) is addresssed, we could attack 2) with it. 

How would 2 be attacked?

Because you can alternatively work around the problem with #1 by creating an object with a call-as-instance handler, right? The downside of this is that the type of custom element constructors will be object and not function until #1 is fixed.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:65
> +        wrapper->Set(v8String("constructor", isolate), constructorWapper);

In JavaScript, typically the constructor property is set on the prototype, not the instance.

> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:43
> +        return throwTypeError("DOM object constructor cannot be called as a function.", args.GetIsolate());

For consistency it might be nice to produce the same error V8 produces for the built-in constructors, namely a TypeError with "Illegal constructor"?

Does WebIDL define an error message for calling constructors as functions?

[Adam Barth]

Comment on attachment 185411 [details]
Patch

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

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:57
>> +    v8::Handle<v8::Value> interfaceObject = global->Get(v8String("HTMLSpanElement", isolate));
> 
> Isn’t it easy for the script author to set this to anything that they want?
> 
> It would be better to consult V8HTMLSpanElement directly to get the interface object, then the interface prototype object.

We should at least do an instanceOf check.

> Source/WebCore/bindings/v8/CustomElementHelpers.h:45
> +    static void initializeConstructorWrapper(CustomElementConstructor*, const ScriptValue&, ScriptState*);

I'd name the second parameter.  It's not clear what ScriptValue you're supposed to pass here.

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:51
> +    v8::Persistent<v8::FunctionTemplate> newTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New());

I might add a comment that this leak is intentional so that folks don't copy/paste the pattern elsewhere without realizing that it's a leak.

> Source/WebCore/bindings/v8/V8DOMWrapper.cpp:154
> +v8::Handle<v8::Function> V8DOMWrapper::toFunction(v8::Handle<v8::Object> object, AtomicString name, v8::Isolate* isolate)

AtomicString -> const AtomicString& ?

> Source/WebCore/dom/CustomElementConstructor.h:60
> +    void willDestroyDocument();

Should this class be a ContextDestructionObserver?

> Source/WebCore/dom/Document.h:1548
> +    OwnPtr<CustomElementRegistry> m_registry;

CustomElementRegistry is RefCounted but here you're using an OwnPtr.  Perhaps CustomElementRegistry should not be RefCounted?

[Adam Barth]

This iteration looks like a big improvement.  I don't fully understand where the storage went, which means I'll need to study it some more.  dominicc has a bunch of good comments.

One thing that worries me is all the calls to V8's Get() function.  It's tricky to read back from the JavaScript environment because the web page might have rewired things in a strange way.  Also, every call to Get() can end up running JavaScript, we need to be prepared to re-enter WebCore at each of these call sites.  If we can't eliminate the Get() calls, perhaps we should have tests that use __defineGetter__ to hook each one so that ClusterFuzz will have some seed material to hammer us on re-entrancy.

[Hajime Morrita]

Comment on attachment 185411 [details]
Patch

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

Dominic, Adam, Thanks for another review!
Since I'm under gardener duty, I'll be back this next week.

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:66
>> +bool CustomElementHelpers::isCustomElementsAllowed(ScriptState* state)
> 
> It seems grammatically weird to have is with plural elements. But maybe the pull of is for a boolean accessor is irresistibile?

Hm. Renamed this to avoid isFeatureAllowed() to avoid such a complication.

>> Source/WebCore/bindings/v8/CustomElementHelpers.h:45
>> +    static void initializeConstructorWrapper(CustomElementConstructor*, const ScriptValue&, ScriptState*);
> 
> I'd name the second parameter.  It's not clear what ScriptValue you're supposed to pass here.

Done.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:51
>> +    v8::Persistent<v8::FunctionTemplate> newTemplate = v8::Persistent<v8::FunctionTemplate>::New(v8::FunctionTemplate::New());
> 
> I might add a comment that this leak is intentional so that folks don't copy/paste the pattern elsewhere without realizing that it's a leak.

Right. Should do.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.h:47
>> +//      WebKit need it to associate each wrapper to its backing C++ object. We store it in an internal field of the wrapped object.
> 
> It looks like you use a hidden value, which is slightly different to an internal field.

Right. Fixed.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.h:49
>> +// Once 1) is addresssed, we could attack 2) with it. 
> 
> How would 2 be attacked?
> 
> Because you can alternatively work around the problem with #1 by creating an object with a call-as-instance handler, right? The downside of this is that the type of custom element constructors will be object and not function until #1 is fixed.

My first version exposed the callable object directly. 
But it doesn't have any methods from Function since it is not a function as you mentioned.

Once #1 is fixed, we could just create a function instance for the specific type of C++ object.
I expect it not be a big deal since Function is inherited from Object and in many case they are compatible.

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:65
>> +        wrapper->Set(v8String("constructor", isolate), constructorWapper);
> 
> In JavaScript, typically the constructor property is set on the prototype, not the instance.

Right. This is simply wrong. Altering the constructor is done when the prototype object is registered.
And, well, you are making a good point. I might need to spec clarification.

>> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:43
>> +        return throwTypeError("DOM object constructor cannot be called as a function.", args.GetIsolate());
> 
> For consistency it might be nice to produce the same error V8 produces for the built-in constructors, namely a TypeError with "Illegal constructor"?
> 
> Does WebIDL define an error message for calling constructors as functions?

This error message is copied from a generated constructor. So I think this is same as other binding-backed constructors.
Try "Image()" for example.

>> Source/WebCore/dom/CustomElementConstructor.h:60
>> +    void willDestroyDocument();
> 
> Should this class be a ContextDestructionObserver?

That might be possible but I rather prefer to explicitly notify this from the Registry to emphasize the ownership.
DestructionObserver could be generalized to use both places though.

>> Source/WebCore/dom/Document.h:1548
>> +    OwnPtr<CustomElementRegistry> m_registry;
> 
> CustomElementRegistry is RefCounted but here you're using an OwnPtr.  Perhaps CustomElementRegistry should not be RefCounted?

Good catch! The class shoudn't ref-counted.

[Dominic Cooney]

(In reply to comment #59)
> One thing that worries me is all the calls to V8's Get() function.  It's tricky to read back from the JavaScript environment because the web page might have rewired things in a strange way.  Also, every call to Get() can end up running JavaScript, we need to be prepared to re-enter WebCore at each of these call sites.

This was my visceral reaction, but Get("prototype") on the constructor, provided you did not mix up setting prototype as v8::ReadOnly or mix up the constructor for another object, is safe I think since that will be non-writable, non-configurable?

But maybe I am missing a more creative way to attack this.

The 'prototype' option once implemented will provide more opportunity for mischief.

[Adam Barth]

> This was my visceral reaction, but Get("prototype") on the constructor, provided you did not mix up setting prototype as v8::ReadOnly or mix up the constructor for another object, is safe I think since that will be non-writable, non-configurable?

We should add LayoutTests for a bunch of ways of trying to attack it.  For example, we should try assignment, deleting and then assigning, as well as using __defineGetter__ to shadow it.

[Hajime Morrita]

Created attachment 187003 [details]
Patch

[Hajime Morrita]

I tried to write some fuzzing code but Bug 109129 prevented me to do it.
The last patch changed to accept "prototype" parameter since I found that 
the default parameter behavior has ugliness in the spec (I filed a w3 bug for that.)
This patch doesn't support the default "prototype" parameter.

[WebKit Review Bot]

Comment on attachment 187003 [details]
Patch

Attachment 187003 [details] did not pass cr-linux-debug-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16443015

[Peter Beverloo (cr-android ews)]

Comment on attachment 187003 [details]
Patch

Attachment 187003 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16431023

[Peter Beverloo (cr-android ews)]

Comment on attachment 187003 [details]
Patch

Attachment 187003 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16445039

[Hajime Morrita]

Created attachment 187035 [details]
Patch

[Hajime Morrita]

Created attachment 187223 [details]
Patch

[Hajime Morrita]

Created attachment 188486 [details]
Patch

[Hajime Morrita]

The last one contains a few fuzzing tests that I could figure out.
And they actually hit some crashes.

[Build Bot]

Comment on attachment 188486 [details]
Patch

Attachment 188486 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16556033

New failing tests:
media/video-controls-captions-trackmenu.html

[Hajime Morrita]

(In reply to comment #72)
> (From update of attachment 188486 [details])
> Attachment 188486 [details] did not pass mac-ews (mac):
Seems a false positive. The new code isn't enabled on mac.

[Adam Barth]

Comment on attachment 188486 [details]
Patch

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

Some minor comments below.  I have to run to a meeting.  I'll look more later today.

> Source/WebCore/ChangeLog:3
> +

You've got an extra blank line here.

> Source/WebCore/ChangeLog:155
> [phone]  Hajime Morrita  <[e-mail]>

Looks like you've got a second copy of the ChangeLog entry.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:47
> +    v8::Handle<v8::Value> wrapperValue = WebCore::toV8(constructor, state->context()->Global(), state->context()->GetIsolate());

"WebCore::" <-- You should be able to drop this prefix.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:53
> +    //   - __defineSetter__ nor __defineGetter__ also doesn't work against function objects and

I'm not sure that's true.  Try typing the following into the web inspector:

window.alert.__defineGetter__("foo", function() { console.log("hi") } ); window.alert.foo

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:83
> +    // document.register() sets the construcdtor property, so the prototype shouldn't have one.

typo: construcdtor

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:92
> +    v8::Handle<v8::Object> spanPrototype = v8::Handle<v8::Object>::Cast(spanConstructor->Get(v8String("prototype", state->context()->GetIsolate())));

This execute arbitrary script.

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:95
> +    v8::Handle<v8::Object> paragraphConstructor = v8::Handle<v8::Object>::Cast(perContextData->constructorForType(&V8HTMLParagraphElement::info));

Why does this fall back to the <p> prototype?

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:46
> +    if (!isolate)
> +        isolate = v8::Isolate::GetCurrent();

Aren't isolate parameters required for getTemplate now?

[Adam Barth]

Comment on attachment 188486 [details]
Patch

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

> Source/WebCore/dom/CustomElementConstructor.cpp:65
> +void CustomElementConstructor::willDestroyDocument()
> +{
> +    m_document = 0;
> +}

Should this just be a ContextDestructionObserver?

> Source/WebCore/dom/CustomElementRegistry.cpp:55
> +    for (ConstructorMap::iterator i = m_constructors.begin(); i != m_constructors.end(); ++i)
> +        i->value->willDestroyDocument();

This isn't needed if the constructors are ContextDestructionObservers.

> Source/WebCore/dom/CustomElementRegistry.cpp:81
> +        reservedNames.append("annotation-xml");
> +        reservedNames.append("color-profile");
> +        reservedNames.append("font-face");
> +        reservedNames.append("font-face-src");
> +        reservedNames.append("font-face-uri");
> +        reservedNames.append("font-face-format");
> +        reservedNames.append("font-face-name");
> +        reservedNames.append("missing-glyph");

We should have constants for these already in MathMLNames or SVGNames, right?

> Source/WebCore/dom/CustomElementRegistry.cpp:128
> +    RefPtr<CustomElementConstructor> fresh = CustomElementConstructor::create(state, m_document, newName, "HTMLCustomElement", prototypeValue);
> +    if (!fresh) {
> +        ec = INVALID_STATE_ERR;
> +        return 0;
> +    }
> +        
> +    m_constructors.add(fresh->tagName().impl(), fresh);

The main problem with this patch is that if CustomElementHelpers::isValidPrototypeParameter or similar executes JavaScript, then the member variables touched in the remainder of this function might have already been destroyed.

> Source/WebCore/dom/CustomElementRegistry.cpp:132
> +CustomElementConstructor* CustomElementRegistry::find(const QualifiedName& name) const

So this function return a PassRefPtr to ensure that all callers keep a reference to the CustomElementConstructor?

> Source/WebCore/dom/CustomElementRegistry.cpp:141
> +    if (CustomElementConstructor* found = find(name))
> +        return found->createElement();

For example, here |found| might be destoryed by JavaScript called via createElement.

[Adam Barth]

This patch generally looks good.  I'm only really worried about use-after-free issues that might be caused by re-entering JavaScript via setters and getters as noted above.

[Hajime Morrita]

Created attachment 189493 [details]
Patch

[Hajime Morrita]

Comment on attachment 188486 [details]
Patch

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

Adam, thanks for your patient and thoughtful review.
I tried to tighten up the guard against script execution which comes from the reentrancy.

>> Source/WebCore/ChangeLog:3
>> +
> 
> You've got an extra blank line here.

Oops. Removed.

>> Source/WebCore/ChangeLog:155
>> [phone]  Hajime Morrita  <[e-mail]>
> 
> Looks like you've got a second copy of the ChangeLog entry.

Removed.

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:47
>> +    v8::Handle<v8::Value> wrapperValue = WebCore::toV8(constructor, state->context()->Global(), state->context()->GetIsolate());
> 
> "WebCore::" <-- You should be able to drop this prefix.

Right. Removed.

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:53
>> +    //   - __defineSetter__ nor __defineGetter__ also doesn't work against function objects and
> 
> I'm not sure that's true.  Try typing the following into the web inspector:
> 
> window.alert.__defineGetter__("foo", function() { console.log("hi") } ); window.alert.foo

My explanation was unclear. __defineGetter__ doesn't work for "prototype" and property of function objects. 
I revised this comment.

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:83
>> +    // document.register() sets the construcdtor property, so the prototype shouldn't have one.
> 
> typo: construcdtor

Fixed.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:46
>> +        isolate = v8::Isolate::GetCurrent();
> 
> Aren't isolate parameters required for getTemplate now?

Yes it is since the template is stored in per-isolate data.

>> Source/WebCore/dom/CustomElementConstructor.cpp:65
>> +}
> 
> Should this just be a ContextDestructionObserver?

It should. Fixed.

>> Source/WebCore/dom/CustomElementRegistry.cpp:81
>> +        reservedNames.append("missing-glyph");
> 
> We should have constants for these already in MathMLNames or SVGNames, right?

Good point. Will use such constants.

>> Source/WebCore/dom/CustomElementRegistry.cpp:128
>> +    m_constructors.add(fresh->tagName().impl(), fresh);
> 
> The main problem with this patch is that if CustomElementHelpers::isValidPrototypeParameter or similar executes JavaScript, then the member variables touched in the remainder of this function might have already been destroyed.

Well, that's a frightening idea, but that's possible...
I'll turn this registory class a Refcounted and protect it self during this function.

[WebKit Review Bot]

Comment on attachment 189493 [details]
Patch

Attachment 189493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16652950

New failing tests:
fast/dom/custom/document-register-reentrant-returning-fake.html
fast/dom/prototype-chain.html
fast/dom/custom/document-register-reentrant-null-constructor.html

[WebKit Review Bot]

Comment on attachment 189493 [details]
Patch

Attachment 189493 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16694008

New failing tests:
fast/dom/custom/document-register-reentrant-returning-fake.html
fast/dom/prototype-chain.html
fast/dom/custom/document-register-reentrant-null-constructor.html

[Erik Arvidsson]

(In reply to comment #78)
> (From update of attachment 188486 [details])

> >> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:53
> >> +    //   - __defineSetter__ nor __defineGetter__ also doesn't work against function objects and
> > 
> > I'm not sure that's true.  Try typing the following into the web inspector:
> > 
> > window.alert.__defineGetter__("foo", function() { console.log("hi") } ); window.alert.foo
> 
> My explanation was unclear. __defineGetter__ doesn't work for "prototype" and property of function objects. 
> I revised this comment.

The 'prototype' property of functions are non configurable and for the WebIDL interface object we even make it non writable. http://www.w3.org/TR/WebIDL/#interface-object

Also, we should stop using __defineGetter__ (both in code and in prose). Please refer to the standard ES5 object model terms and APIs. If you used the relevant terms, (non configurable in this case) it would have been a lot easier to follow why user code cannot add a getter.

[Adam Barth]

> Also, we should stop using __defineGetter__ (both in code and in prose). Please refer to the standard ES5 object model terms and APIs. If you used the relevant terms, (non configurable in this case) it would have been a lot easier to follow why user code cannot add a getter.

Sorry, I haven't learned the details of the new object model.  Is there something less intimidating than the ES5 spec that I should read to better understand how this works in practice?

[Erik Arvidsson]

(In reply to comment #82)
> > Also, we should stop using __defineGetter__ (both in code and in prose). Please refer to the standard ES5 object model terms and APIs. If you used the relevant terms, (non configurable in this case) it would have been a lot easier to follow why user code cannot add a getter.
> 
> Sorry, I haven't learned the details of the new object model.  Is there something less intimidating than the ES5 spec that I should read to better understand how this works in practice?

I apologize if my tone was a bit harsh. That was not my intention.

I haven't read the link below in detail but Tom is a member of TC39 so I assume it is all correct.

http://soft.vub.ac.be/~tvcutsem/invokedynamic/js-object-model

[Hajime Morrita]

Created attachment 189666 [details]
Patch

[Hajime Morrita]

Updated the test to fix the failure, and revised the comment to in the standard term.
The fuzzing test happened to have been using definedProperty() already :-)

[WebKit Review Bot]

Comment on attachment 189666 [details]
Patch

Attachment 189666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16690616

New failing tests:
fast/dom/prototype-chain.html

[Hajime Morrita]

Created attachment 189706 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 189706 [details]
Patch

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

> Source/WebCore/ChangeLog:49
> +        [JS Adaptor Fiction] <-(hidden property)-> [JS Wrapper Object] -(internal field)-> [C++ Native object]

fiction?

[Adam Barth]

Comment on attachment 189706 [details]
Patch

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

> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:66
> +static bool hasNoBuiltinsInPrototype(v8::Handle<v8::Object> htmlPrototype, v8::Handle<v8::Value> chain)

What a strange function.  Why is this needed?

> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:46
> +    if (!isolate)
> +        isolate = v8::Isolate::GetCurrent();

I meant that I think everyone that calls this function will pass a non-zero isolate argument.

> Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:71
> +        prototype->SetInternalFieldCount(prototypeInternalFieldcount);

This seems like a bit of a hack.  Perhaps we should skip this hack and make sure we fix bug 110436 before enabling this feature.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:42
> +#if ENABLE(CUSTOM_ELEMENTS)

We usually put file-level ifdefs just after config.h with a blank line above and below.

> Source/WebCore/bindings/v8/V8HTMLCustomElement.h:56
> +        return v8::Handle<v8::Object>::Cast(v8NullWithCheck(isolate));

This is a bad cast.  v8::Null is not a v8::Object.  This function should probably return a v8::Value rather than a v8::Object.

> Source/WebCore/bindings/v8/custom/V8CustomElementConstructorCustom.cpp:51
> +    return V8HTMLCustomElement::toV8(element.get(), args.Holder(), args.GetIsolate());

Yeah, looks like the caller can handle a v8::Value.

> Source/WebCore/dom/CustomElementRegistry.cpp:51
> +#if ENABLE(CUSTOM_ELEMENTS)

We should move all these ifdefs to the usual place below config.h

> Source/WebCore/dom/CustomElementRegistry.cpp:95
> +    if (WTF::notFound != reservedNames.find(name))

"WTF::" shouldn't be needed

> Source/WebCore/dom/CustomElementRegistry.cpp:141
> +    RefPtr<CustomElementConstructor> fresh = CustomElementConstructor::create(state, document(), newName, "HTMLCustomElement", prototypeValue);

fresh <-- I probably would have called this "constructor"

[Adam Barth]

The above are just nits.  The only think I'm not super happy about is the abuse of the internal field count.  I'll leave it up to you to decide how to resolve that issue.

[Hajime Morrita]

Comment on attachment 189706 [details]
Patch

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

Adam, Dimitri, thanks for taking a look and for the thorough review.
I'll re-think how to address prototype hack when extending this to allow arbitrary HTMLElement subclasses on Bug 110436.
It will need a certain amount of hassle thus will be done in separate change.

>> Source/WebCore/ChangeLog:49
>> +        [JS Adaptor Fiction] <-(hidden property)-> [JS Wrapper Object] -(internal field)-> [C++ Native object]
> 
> fiction?

Ouch, my spellchecker went too far :-(

>> Source/WebCore/bindings/v8/CustomElementHelpers.cpp:66
>> +static bool hasNoBuiltinsInPrototype(v8::Handle<v8::Object> htmlPrototype, v8::Handle<v8::Value> chain)
> 
> What a strange function.  Why is this needed?

As you pointed out below, this is because of bug 110436.

>> Source/WebCore/bindings/v8/V8AdaptorFunction.cpp:46
>> +        isolate = v8::Isolate::GetCurrent();
> 
> I meant that I think everyone that calls this function will pass a non-zero isolate argument.

Ah, good news! I'll turn this to ASSERT().

>> Source/WebCore/bindings/v8/V8DOMConfiguration.cpp:71
>> +        prototype->SetInternalFieldCount(prototypeInternalFieldcount);
> 
> This seems like a bit of a hack.  Perhaps we should skip this hack and make sure we fix bug 110436 before enabling this feature.

True. This is kind of a bandaid.
I don't have definitive idea on how to address bug 110436, but this will surely be changed significantly during the fix.

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.cpp:42
>> +#if ENABLE(CUSTOM_ELEMENTS)
> 
> We usually put file-level ifdefs just after config.h with a blank line above and below.

Will fix.

>> Source/WebCore/bindings/v8/V8HTMLCustomElement.h:56
>> +        return v8::Handle<v8::Object>::Cast(v8NullWithCheck(isolate));
> 
> This is a bad cast.  v8::Null is not a v8::Object.  This function should probably return a v8::Value rather than a v8::Object.

Right. Will fix.

>> Source/WebCore/dom/CustomElementRegistry.cpp:51
>> +#if ENABLE(CUSTOM_ELEMENTS)
> 
> We should move all these ifdefs to the usual place below config.h

Will fix.

>> Source/WebCore/dom/CustomElementRegistry.cpp:95
>> +    if (WTF::notFound != reservedNames.find(name))
> 
> "WTF::" shouldn't be needed

Will fix.

>> Source/WebCore/dom/CustomElementRegistry.cpp:141
>> +    RefPtr<CustomElementConstructor> fresh = CustomElementConstructor::create(state, document(), newName, "HTMLCustomElement", prototypeValue);
> 
> fresh <-- I probably would have called this "constructor"

Right. there are no non-fresh constructor...

[Hajime Morrita]

Created attachment 189917 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189917 [details]
Patch for landing

Rejecting attachment 189917 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 189917, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
er-reentrant-returning-fake.html
patching file LayoutTests/fast/dom/custom/document-register-reentrant-throwing-constructor-expected.txt
patching file LayoutTests/fast/dom/custom/document-register-reentrant-throwing-constructor.html
patching file LayoutTests/fast/dom/custom/resources/document-register-fuzz.js
patching file LayoutTests/platform/mac/TestExpectations

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16716631

[Hajime Morrita]

Created attachment 189963 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189963 [details]
Patch for landing

Clearing flags on attachment: 189963

Committed r143865: <http://trac.webkit.org/changeset/143865>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.