1378079 - Creating an custom element from parser will synchronously call constructor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Edgar Chen [:edgar]]

See https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token (step 7) and https://dom.spec.whatwg.org/#concept-create-element

Comment 1

[Edgar Chen [:edgar]]

In WIP patch of bug 1301024 (Attachment 8856843 [details] [diff]), we try to implement https://dom.spec.whatwg.org/#concept-create-element in NS_NewHTMLElement() [1] which also benefits the parser. However, it might be a problem that calling constructor or invoking callback synchronously during parsing, so I split parser part to this bug. bug 1301024 will only handle NOT_FROM_PARSER case.

Hi William, could you kindly give us some suggestions about what we should do on parser side? Thank you.

[1] http://searchfox.org/mozilla-central/rev/5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/html/nsHTMLContentSink.cpp#233

Comment 2

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

(In reply to Edgar Chen [:edgar] (parental leave ~ 10/1; slow response) from comment #1)
> In WIP patch of bug 1301024 (Attachment 8856843 [details] [diff]), we try to
> implement https://dom.spec.whatwg.org/#concept-create-element in
> NS_NewHTMLElement() [1] which also benefits the parser. However, it might be
> a problem that calling constructor or invoking callback synchronously during
> parsing, so I split parser part to this bug. bug 1301024 will only handle
> NOT_FROM_PARSER case.
> 
> Hi William, could you kindly give us some suggestions about what we should
> do on parser side? Thank you.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 5e1e8d2f244bd8c210a578ff1f65c3b720efe34e/dom/html/nsHTMLContentSink.cpp#233

As Henri mentioned the issue at SF allhands, it would be great to know his suggestions. :)

Comment 3

[Henri Sivonen (:hsivonen)]

We should
 1) Introduce a counter for making document.open(), etc. throw: https://html.spec.whatwg.org/#throw-on-dynamic-markup-insertion-counter
 2) Make the relevant methods throw if the counter is non-zero.
 3) When creating a custom element from the parser, increment the counter, break out of the doc update with mDocument->EndUpdate(UPDATE_CONTENT_MODEL), create the custom element, re-open the doc update with aCurrentDoc->BeginUpdate(UPDATE_CONTENT_MODEL) and decrement the counter.

We should test that things work reasonably when a script has moved the element that the parser is inserting the custom element to to another document before the custom element creation happens.

(Discussion with bz in SF indicated that BeginUpdate/EndUpdate is reasonable cheap these days, so it's probably not worthwhile to request a spec change to avoid them.)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Thanks Henri for the input.

Looking at our implementation and comparing with the spec [1], it seems that we still need to implement some more steps, like step 3-6 and step 9, and remove the old part [2]. I can start adding these steps while bug 1299363 and bug 1301024 are being worked on.


[1] https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token
[2] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/parser/html/nsHtml5TreeOperation.cpp#437-439

Comment 5

[Jessica Jong [:jessica] (inactive)]

Hi Henri,

For implementing step 3 in https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token, it seems that we need to add a nsHtml5AttributeName for "is". Is there any documentation on how to add a new nsHtml5AttributeName? Looks like I need to clone hg.mozilla.org/projects/htmlparser/ and generate a hash with it?

I'm totally new to the parser code, so any advice/suggestion is more than welcome. :)

Thanks.

Comment 6

[Henri Sivonen (:hsivonen)]

(In reply to Jessica Jong [:jessica] from comment #5)
> For implementing step 3 in
> https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-
> the-token, it seems that we need to add a nsHtml5AttributeName for "is".

You don't strictly *need* to. That is, at this point, adding it is an optimization and not a correctness issue. It should still be added, though.

> Is
> there any documentation on how to add a new nsHtml5AttributeName?

I don't recall. I should get around to writing it.

> Looks like
> I need to clone hg.mozilla.org/projects/htmlparser/ and generate a hash with
> it?

Yes. Here's how:

cd parser/html/java/
make sync
# now you have a clone of https://hg.mozilla.org/projects/htmlparser/ in parser/html/java/htmlparser/
cd htmlparser/src/
$EDITOR nu/validator/htmlparser/impl/AttributeName.java
# Search for the word "uncomment" and uncomment stuff according to the two comments that talk about uncommenting
# Duplicate the declaration a normal attribute (nothings special in SVG mode, etc.). Let's use "alt", since it's the first one.
# In the duplicate, replace ALT with IS and "alt" with "is".
# Search for "ALT,", duplicate that line and change the duplicate to say "IS,"
# Save.
javac nu/validator/htmlparser/impl/AttributeName.java
java nu.validator.htmlparser.impl.AttributeName
# Copy and paste the output into nu/validator/htmlparser/impl/AttributeName.java replacing the text below the comment "START GENERATED CODE" and above the very last "}".
# Recomment the bits that you uncommented earlier.
# Save.
cd ../.. # Back to parser/html/java/
make translate
cd ../../..
./mach clang-format

Comment 7

[Henri Sivonen (:hsivonen)]

And the changes made to the local copy of the htmlparser repo need to be landed to the htmlparser repo manually once the corresponding mozilla-central patch containing the translation output lands. The commit comment for the htmlparser repo should say "Mozilla bug 1378079 - ..." instead of "Bug 1378079 - ..." to disambiguate which bug database's bug number is being referred to.

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: Add attribute 'is' to HTML parser (htmlparser repo)

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Gecko changes for adding attribute 'is' to parser.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: Parse 'is' attribute in parser and pass it to NS_NewElement.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8894818 [details] [diff] [review]
Part 1: Gecko changes for adding attribute 'is' to parser.

Review of attachment 8894818 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Henri, it seems that the code generated had involved some reordering of attributes, is this ok?

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 2: Introduce throw-on-dynamic-markup-insertion counter.

I decided to add this in a separate patch. Can I have your feedback on this?

Comment 13

[Jessica Jong [:jessica] (inactive)]

Attachment: (WIP) Part 3: Complete the steps related to custom elements in "create an element for a token".

Henri, here is the implementation to complete the steps related to custom elements in https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token.
I haven't fully tested it yet, since it depends on bug 1299363 and bug 1301024. But I'd like to have your early feedback to see if I'm on the right track. Thanks.

Note that I haven't added the "synchronous custom elements flag" yet since we might add it in bug 1301024.

Comment 14

[Henri Sivonen (:hsivonen)]

Comment on attachment 8894818 [details] [diff] [review]
Part 1: Gecko changes for adding attribute 'is' to parser.

Review of attachment 8894818 [details] [diff] [review]:
-----------------------------------------------------------------

> it seems that the code generated had involved some reordering of attributes, is this ok?

This is expected due to the ordering reflecting search through a binary tree (bug 1366241).

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 8896169 [details] [diff] [review]
(WIP) Part 2: Introduce throw-on-dynamic-markup-insertion counter.

Review of attachment 8896169 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsIDocument.h
@@ +2977,5 @@
>    virtual bool IsThirdParty() = 0;
>  
>    bool IsScopedStyleEnabled();
>  
> +  bool IsThrowOnDynamicMarkupInsertionCounterSet()

I suggest renaming this to something that describes the conclusion rather than there being a counter. Something like `ShouldThrowOnDynamicMarkupInsertion()`.

@@ +2979,5 @@
>    bool IsScopedStyleEnabled();
>  
> +  bool IsThrowOnDynamicMarkupInsertionCounterSet()
> +  {
> +    return mThrowOnDynamicMarkupInsertionCounter > 0;

Since the counter is unsigned, please just use: `return mThrowOnDynamicMarkupInsertionCounter;`

@@ +2989,5 @@
> +  }
> +
> +  void DecrementThrowOnDynamicMarkupInsertionCounter()
> +  {
> +    MOZ_ASSERT(mThrowOnDynamicMarkupInsertionCounter > 0);

Also here, just `MOZ_ASSERT(mThrowOnDynamicMarkupInsertionCounter);`

Comment 16

[Henri Sivonen (:hsivonen)]

Comment on attachment 8896171 [details] [diff] [review]
(WIP) Part 3: Complete the steps related to custom elements in "create an element for a token".

Review of attachment 8896171 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs rebasing due to bug 483155. Sorry.

Looks good generally. The microtask stuff looks ok on surface, but I'm not familiar with the microtask details and smaug could say for sure.

::: parser/html/nsHtml5TreeOperation.cpp
@@ +362,5 @@
> +        is.ToString(isValue);
> +      }
> +    }
> +
> +    if (aFromParser != dom::FROM_PARSER_FRAGMENT) {

I see this matches the spec, but I'm curious: How do custom elements work with `innerHTML`?

@@ +410,2 @@
>  
> +    if (MOZ_UNLIKELY(aName == nsGkAtoms::style || aName == nsGkAtoms::link)) {

Is there something in the spec that makes the use of `is` on `style` and `link` not take effect?

@@ +413,5 @@
> +      if (ssle) {
> +        ssle->InitStyleLinkElement(false);
> +        ssle->SetEnableUpdates(false);
> +      }
> +    } else if (MOZ_UNLIKELY(isKeygen)) {

I guess customizing `keygen` is just going to be broken, since it looks like `keygen` be removed before bug 101019 ever gets fixed.

Comment 17

[Jessica Jong [:jessica] (inactive)]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Comment on attachment 8894818 [details] [diff] [review]
> Part 1: Gecko changes for adding attribute 'is' to parser.
> 
> Review of attachment 8894818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > it seems that the code generated had involved some reordering of attributes, is this ok?
> 
> This is expected due to the ordering reflecting search through a binary tree
> (bug 1366241).

I see, thanks for the info.

(In reply to Henri Sivonen (:hsivonen) from comment #15)
> Comment on attachment 8896169 [details] [diff] [review]
> (WIP) Part 2: Introduce throw-on-dynamic-markup-insertion counter.
> 
> Review of attachment 8896169 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsIDocument.h
> @@ +2977,5 @@
> >    virtual bool IsThirdParty() = 0;
> >  
> >    bool IsScopedStyleEnabled();
> >  
> > +  bool IsThrowOnDynamicMarkupInsertionCounterSet()
> 
> I suggest renaming this to something that describes the conclusion rather
> than there being a counter. Something like
> `ShouldThrowOnDynamicMarkupInsertion()`.

Sounds good, will change it to ShouldThrowOnDynamicMarkupInsertion().

> 
> @@ +2979,5 @@
> >    bool IsScopedStyleEnabled();
> >  
> > +  bool IsThrowOnDynamicMarkupInsertionCounterSet()
> > +  {
> > +    return mThrowOnDynamicMarkupInsertionCounter > 0;
> 
> Since the counter is unsigned, please just use: `return
> mThrowOnDynamicMarkupInsertionCounter;`

Right, will do.

> 
> @@ +2989,5 @@
> > +  }
> > +
> > +  void DecrementThrowOnDynamicMarkupInsertionCounter()
> > +  {
> > +    MOZ_ASSERT(mThrowOnDynamicMarkupInsertionCounter > 0);
> 
> Also here, just `MOZ_ASSERT(mThrowOnDynamicMarkupInsertionCounter);`

Will do.

Comment 18

[Jessica Jong [:jessica] (inactive)]

(In reply to Henri Sivonen (:hsivonen) from comment #16)
> Comment on attachment 8896171 [details] [diff] [review]
> (WIP) Part 3: Complete the steps related to custom elements in "create an
> element for a token".
> 
> Review of attachment 8896171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch needs rebasing due to bug 483155. Sorry.

Sure, thanks for the feedback.

> 
> Looks good generally. The microtask stuff looks ok on surface, but I'm not
> familiar with the microtask details and smaug could say for sure.

I'm not familiar with it either, I'll make sure to ask smaug about it when doing the official review.

> 
> ::: parser/html/nsHtml5TreeOperation.cpp
> @@ +362,5 @@
> > +        is.ToString(isValue);
> > +      }
> > +    }
> > +
> > +    if (aFromParser != dom::FROM_PARSER_FRAGMENT) {
> 
> I see this matches the spec, but I'm curious: How do custom elements work
> with `innerHTML`?

In that case, the `synchronous custom elements flag` will not be set, so instead of running the custom element constructors synchronously, we'll enqueue a custom element upgrade reaction.

> 
> @@ +410,2 @@
> >  
> > +    if (MOZ_UNLIKELY(aName == nsGkAtoms::style || aName == nsGkAtoms::link)) {
> 
> Is there something in the spec that makes the use of `is` on `style` and
> `link` not take effect?

No, it should take effect on anything that implements the HTMLElement interface [1]. I guess I should add this in the `willExecuteScript` section too.

> 
> @@ +413,5 @@
> > +      if (ssle) {
> > +        ssle->InitStyleLinkElement(false);
> > +        ssle->SetEnableUpdates(false);
> > +      }
> > +    } else if (MOZ_UNLIKELY(isKeygen)) {
> 
> I guess customizing `keygen` is just going to be broken, since it looks like
> `keygen` be removed before bug 101019 ever gets fixed.

keygen is defined to use HTMLUnknownElement as their element interface, so a built-in custom element can not extend keygen [1]. I'll leave this part as is since bug 1315460 is not final yet.


[1] https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-customized-builtin-example

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: Gecko changes for adding attribute 'is' to parser, v1.

Comment 20

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: Introduce throw-on-dynamic-markup-insertion counter, v1.

Addressed review comment 15.

Comment 21

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: Complete the steps related to custom elements in "create an element for a token", v1.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8912604 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v1.

Hi Olli, could you check the global object for error reporting part first?
I'll ask for Henri's review once his review queue is open again.

Thanks.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8912604 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v1.

>+++ b/dom/base/CustomElementRegistry.cpp
>@@ -991,18 +991,20 @@ CustomElementReactionsStack::PopAndInvok
>   const uint32_t lastIndex = mReactionsStack.Length() - 1;
>   ElementQueue* elementQueue = mReactionsStack.ElementAt(lastIndex).get();
>   // Check element queue size in order to reduce function call overhead.
>   if (!elementQueue->IsEmpty()) {
>     // It is still not clear what error reporting will look like in custom
>     // element, see https://github.com/w3c/webcomponents/issues/635.
>     // We usually report the error to entry global in gecko, so just follow the
>     // same behavior here.
>+    // This may be null if it's called from parser. However, upgrade reactions
>+    // won't be scheduled in this case and the exception of callback reactions
>+    // will be automatically reported in CallSetup.
>     nsIGlobalObject* global = GetEntryGlobal();
>-    MOZ_ASSERT(global, "Should always have a entry global here!");

Hmm, doesn't this break CustomElementReactionsStack::InvokeReactions.
I don't understand why that let's one to not pass global.
And doesn't this contradict https://bugzilla.mozilla.org/show_bug.cgi?id=1299363#c101
Or is nsHTMLContentSink.cpp trying to capture that. If so, how?


>+++ b/dom/html/nsHTMLContentSink.cpp
>@@ -286,16 +286,20 @@ NS_NewHTMLElement(Element** aResult, alr
>      * For the unset case which is non-synchronous only applied for
>      * inner/outerHTML.
>      */
>     bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT ||
>                                      aFromParser == dom::NOT_FROM_PARSER;
>     // Per discussion in https://github.com/w3c/webcomponents/issues/635,
>     // use entry global in those places that are called from JS APIs.
>     nsIGlobalObject* global = GetEntryGlobal();
>+    if (!global) {
>+      // If this is called from parser, use the node document's global object.
>+      global = nodeInfo->GetDocument()->GetScopeObject();
>+    }
Hmm, what if we are using innerHTML? What should happen per spec?
Could we call GetEntryGlobal only when we aren't called by the parser, since what if
there is some entry global on stack when parser runs. (like, document.write?)

This is kind of vague r- since I don't quite understand why we don't need to pass global to CustomElementReactionsStack::InvokeReactions

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

I chatted with Domenic about https://github.com/w3c/webcomponents/issues/635, and he hasn't had time to looked at it, so we can basically do whatever. But I still don't quite understand why we can pass null global. Could we pass GetScopeObject there or so?

Comment 25

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #23)
> Comment on attachment 8912604 [details] [diff] [review]
> Part 3: Complete the steps related to custom elements in "create an element
> for a token", v1.
> 
> >+++ b/dom/base/CustomElementRegistry.cpp
> >@@ -991,18 +991,20 @@ CustomElementReactionsStack::PopAndInvok
> >   const uint32_t lastIndex = mReactionsStack.Length() - 1;
> >   ElementQueue* elementQueue = mReactionsStack.ElementAt(lastIndex).get();
> >   // Check element queue size in order to reduce function call overhead.
> >   if (!elementQueue->IsEmpty()) {
> >     // It is still not clear what error reporting will look like in custom
> >     // element, see https://github.com/w3c/webcomponents/issues/635.
> >     // We usually report the error to entry global in gecko, so just follow the
> >     // same behavior here.
> >+    // This may be null if it's called from parser. However, upgrade reactions
> >+    // won't be scheduled in this case and the exception of callback reactions
> >+    // will be automatically reported in CallSetup.
> >     nsIGlobalObject* global = GetEntryGlobal();
> >-    MOZ_ASSERT(global, "Should always have a entry global here!");
> 
> Hmm, doesn't this break CustomElementReactionsStack::InvokeReactions.
> I don't understand why that let's one to not pass global.
> And doesn't this contradict
> https://bugzilla.mozilla.org/show_bug.cgi?id=1299363#c101
> Or is nsHTMLContentSink.cpp trying to capture that. If so, how?

CustomElementReactionsStack::InvokeReactions lets one to pass null global since if the reactions are all callback reactions and if it throw exceptions, it'll be handled and reported in Lifecycle*Callback::Call function.

I thought about using GetScopeObject, but we don't have the node document's here. Should we pass the node document as an argument in AutoCEReaction or is there a better way to do it?

> 
> 
> >+++ b/dom/html/nsHTMLContentSink.cpp
> >@@ -286,16 +286,20 @@ NS_NewHTMLElement(Element** aResult, alr
> >      * For the unset case which is non-synchronous only applied for
> >      * inner/outerHTML.
> >      */
> >     bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT ||
> >                                      aFromParser == dom::NOT_FROM_PARSER;
> >     // Per discussion in https://github.com/w3c/webcomponents/issues/635,
> >     // use entry global in those places that are called from JS APIs.
> >     nsIGlobalObject* global = GetEntryGlobal();
> >+    if (!global) {
> >+      // If this is called from parser, use the node document's global object.
> >+      global = nodeInfo->GetDocument()->GetScopeObject();
> >+    }
> Hmm, what if we are using innerHTML? What should happen per spec?
> Could we call GetEntryGlobal only when we aren't called by the parser, since
> what if
> there is some entry global on stack when parser runs. (like, document.write?)

If we are using innerHTML, the synchronousCustomElements flag won't be set, so there is no error reporting here.
But I think it's still fine if we change it to use GetEntryGlobal only when not from parser.

> 
> This is kind of vague r- since I don't quite understand why we don't need to
> pass global to CustomElementReactionsStack::InvokeReactions

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jessica Jong [:jessica] (away Sep 29 - Oct 4) from comment #25)
> CustomElementReactionsStack::InvokeReactions lets one to pass null global
> since if the reactions are all callback reactions and if it throw
> exceptions, it'll be handled and reported in Lifecycle*Callback::Call
> function.
> 
> I thought about using GetScopeObject, but we don't have the node document's
> here. Should we pass the node document as an argument in AutoCEReaction or
> is there a better way to do it?

Ok, so we're never dealing with upgrades here?


> > >+++ b/dom/html/nsHTMLContentSink.cpp
> > >@@ -286,16 +286,20 @@ NS_NewHTMLElement(Element** aResult, alr
> > >      * For the unset case which is non-synchronous only applied for
> > >      * inner/outerHTML.
> > >      */
> > >     bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT ||
> > >                                      aFromParser == dom::NOT_FROM_PARSER;
> > >     // Per discussion in https://github.com/w3c/webcomponents/issues/635,
> > >     // use entry global in those places that are called from JS APIs.
> > >     nsIGlobalObject* global = GetEntryGlobal();
> > >+    if (!global) {
> > >+      // If this is called from parser, use the node document's global object.
> > >+      global = nodeInfo->GetDocument()->GetScopeObject();
> > >+    }
> > Hmm, what if we are using innerHTML? What should happen per spec?
> > Could we call GetEntryGlobal only when we aren't called by the parser, since
> > what if
> > there is some entry global on stack when parser runs. (like, document.write?)
> 
> If we are using innerHTML, the synchronousCustomElements flag won't be set,
> so there is no error reporting here.
> But I think it's still fine if we change it to use GetEntryGlobal only when
> not from parser.
yeah, I think we want to be explicit about that, so that if there is some random entry global on stack, we don't end up using that.
(I need to re-check what flags we have when using document.write and what the spec says about that)

Comment 27

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #26)
> (In reply to Jessica Jong [:jessica] (away Sep 29 - Oct 4) from comment #25)
> > CustomElementReactionsStack::InvokeReactions lets one to pass null global
> > since if the reactions are all callback reactions and if it throw
> > exceptions, it'll be handled and reported in Lifecycle*Callback::Call
> > function.
> > 
> > I thought about using GetScopeObject, but we don't have the node document's
> > here. Should we pass the node document as an argument in AutoCEReaction or
> > is there a better way to do it?
> 
> Ok, so we're never dealing with upgrades here?

Right, no upgrades here if it's called from parser.
However, if we're creating an element from .innerHTML, we will enqueue the upgrade reaction into the backup queue, so we need to fix this part instead:
http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/base/CustomElementRegistry.cpp#1084

> 
> 
> > > >+++ b/dom/html/nsHTMLContentSink.cpp
> > > >@@ -286,16 +286,20 @@ NS_NewHTMLElement(Element** aResult, alr
> > > >      * For the unset case which is non-synchronous only applied for
> > > >      * inner/outerHTML.
> > > >      */
> > > >     bool synchronousCustomElements = aFromParser != dom::FROM_PARSER_FRAGMENT ||
> > > >                                      aFromParser == dom::NOT_FROM_PARSER;
> > > >     // Per discussion in https://github.com/w3c/webcomponents/issues/635,
> > > >     // use entry global in those places that are called from JS APIs.
> > > >     nsIGlobalObject* global = GetEntryGlobal();
> > > >+    if (!global) {
> > > >+      // If this is called from parser, use the node document's global object.
> > > >+      global = nodeInfo->GetDocument()->GetScopeObject();
> > > >+    }
> > > Hmm, what if we are using innerHTML? What should happen per spec?
> > > Could we call GetEntryGlobal only when we aren't called by the parser, since
> > > what if
> > > there is some entry global on stack when parser runs. (like, document.write?)
> > 
> > If we are using innerHTML, the synchronousCustomElements flag won't be set,
> > so there is no error reporting here.
> > But I think it's still fine if we change it to use GetEntryGlobal only when
> > not from parser.
> yeah, I think we want to be explicit about that, so that if there is some
> random entry global on stack, we don't end up using that.
> (I need to re-check what flags we have when using document.write and what
> the spec says about that)

Sure, will do.

Comment 28

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #27)
> (In reply to Olli Pettay [:smaug] from comment #26)
> > (In reply to Jessica Jong [:jessica] (away Sep 29 - Oct 4) from comment #25)
> > > CustomElementReactionsStack::InvokeReactions lets one to pass null global
> > > since if the reactions are all callback reactions and if it throw
> > > exceptions, it'll be handled and reported in Lifecycle*Callback::Call
> > > function.
> > > 
> > > I thought about using GetScopeObject, but we don't have the node document's
> > > here. Should we pass the node document as an argument in AutoCEReaction or
> > > is there a better way to do it?
> > 
> > Ok, so we're never dealing with upgrades here?
> 
> Right, no upgrades here if it's called from parser.
> However, if we're creating an element from .innerHTML, we will enqueue the
> upgrade reaction into the backup queue, so we need to fix this part instead:
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/base/CustomElementRegistry.
> cpp#1084

Edgar just reminded me that .innerHTML has a [CEReaction] annotation, so it will still enqueue into the element queue. This means we'll still have upgrade reaction here if we're creating a custom element from .innerHTML, but that's ok since GetEntryGlobal() would be non-null. We'll only have null entry global for the case of https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token step 8.

Comment 29

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: Complete the steps related to custom elements in "create an element for a token", v2.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8915531 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v2.

Review of attachment 8915531 [details] [diff] [review]:
-----------------------------------------------------------------

Addressed review comment 26 and added mochitest (not wpt) since spec about error reporting is not final yet.

Comment 31

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8915531 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v2.

Will also ask for :smaug's review once his review queue is open :)

Comment 32

[Henri Sivonen (:hsivonen)]

Comment on attachment 8915531 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v2.

Review of attachment 8915531 [details] [diff] [review]:
-----------------------------------------------------------------

I believe this patch works correctly, but I'm setting r-, because the patch seems to lose the optimization based on the aCreator == NS_NewCustomElement check.

::: dom/html/nsHTMLContentSink.cpp
@@ +299,5 @@
> +    if (aFromParser == dom::NOT_FROM_PARSER) {
> +      global = GetEntryGlobal();
> +    } else {
> +      global = nodeInfo->GetDocument()->GetScopeObject();
> +    }

I'm not competent to review this bit.

::: parser/html/nsHtml5TreeOperation.cpp
@@ -355,2 @@
>  
> -  if (aCreator == NS_NewCustomElement) {

Why doesn't the new code make use of this information? (I.e. if aCreator is NS_NewCustomElement, the element has a hyphen in its name.)

It seems to me that if the element has no attribute named "is" and aCreator isn't NewCustomElement, the code should avoid calling nsContentUtils::LookupCustomElementDefinition.

@@ +446,5 @@
>        return newContent;
>      }
>  
> +    SetHTMLElementAttributes(newContent, aName, aAttributes);
> +    document->BeginUpdate(UPDATE_CONTENT_MODEL);

An update batch is re-opened in two places, which is correct but error prone in the case of future edits. A RAII class for calling document->EndUpdate(UPDATE_CONTENT_MODEL); in the constructor and document->BeginUpdate(UPDATE_CONTENT_MODEL); in the destructor would be safer.

Comment 33

[Jessica Jong [:jessica] (inactive)]

(In reply to Henri Sivonen (:hsivonen) from comment #32)
> Comment on attachment 8915531 [details] [diff] [review]
> Part 3: Complete the steps related to custom elements in "create an element
> for a token", v2.
> 
> Review of attachment 8915531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe this patch works correctly, but I'm setting r-, because the patch
> seems to lose the optimization based on the aCreator == NS_NewCustomElement
> check.
> 
> ::: dom/html/nsHTMLContentSink.cpp
> @@ +299,5 @@
> > +    if (aFromParser == dom::NOT_FROM_PARSER) {
> > +      global = GetEntryGlobal();
> > +    } else {
> > +      global = nodeInfo->GetDocument()->GetScopeObject();
> > +    }
> 
> I'm not competent to review this bit.

No problem, smaug is also looking at this part.

> 
> ::: parser/html/nsHtml5TreeOperation.cpp
> @@ -355,2 @@
> >  
> > -  if (aCreator == NS_NewCustomElement) {
> 
> Why doesn't the new code make use of this information? (I.e. if aCreator is
> NS_NewCustomElement, the element has a hyphen in its name.)
> 
> It seems to me that if the element has no attribute named "is" and aCreator
> isn't NewCustomElement, the code should avoid calling
> nsContentUtils::LookupCustomElementDefinition.

Right, we can make use of NS_NewCustomElement and attribute "is" to skip nsContentUtils::LookupCustomElementDefinition. I'll add this part in the upcoming patch.

> 
> @@ +446,5 @@
> >        return newContent;
> >      }
> >  
> > +    SetHTMLElementAttributes(newContent, aName, aAttributes);
> > +    document->BeginUpdate(UPDATE_CONTENT_MODEL);
> 
> An update batch is re-opened in two places, which is correct but error prone
> in the case of future edits. A RAII class for calling
> document->EndUpdate(UPDATE_CONTENT_MODEL); in the constructor and
> document->BeginUpdate(UPDATE_CONTENT_MODEL); in the destructor would be
> safer.

Sure, will add a RAII class for this.

Comment 34

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: Complete the steps related to custom elements in "create an element for a token", v3.

Addressed review comment 32.

Also in this patch, we make the error thrown in custom element constructor to propagate correctly when creating an element. Bug 1406297 will handle the rest of the cases to match the spec better.

Comment 35

[Henri Sivonen (:hsivonen)]

Comment on attachment 8915890 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v3.

Review of attachment 8915890 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

::: parser/html/nsHtml5TreeOperation.cpp
@@ +427,5 @@
> +    mozAutoPauseContentUpdate autoPauseContentUpdate(document);
> +
> +    nsCOMPtr<dom::Element> newElement;
> +    NS_NewHTMLElement(getter_AddRefs(newElement), nodeInfo.forget(),
> +                      aFromParser, (isValue.IsEmpty() ? nullptr : &isValue));

As a follow-up, please introduce a variant of NS_NewHTMLElement that takes dom::CustomElementDefinition* as an argument in order to avoid looking it up a second time.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8915890 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v3.


>+  if (willExecuteScript) { // This will cause custom element constructors to run
>+    AutoSetThrowOnDynamicMarkupInsertionCounter
>+      throwOnDynamicMarkupInsertionCounter(document);
>+    {
>+      nsAutoMicroTask mt;
>     }
>-  } else if (MOZ_UNLIKELY(isKeygen)) {
>-    // Adapted from CNavDTD
>-    nsresult rv;
>-    nsCOMPtr<nsIFormProcessor> theFormProcessor =
>-      do_GetService(kFormProcessorCID, &rv);
>-    if (NS_FAILED(rv)) {
>+    dom::AutoCEReaction
>+      autoCEReaction(document->GetDocGroup()->CustomElementReactionsStack());
>+    mozAutoPauseContentUpdate autoPauseContentUpdate(document);
Something here looks scary. If we're in doc update (in which case there is script blocker on stack), we shouldn't be running microtasks.
In practice those microtasks end up postponed to run when scripts are safe to run, so I when mozAutoPauseContentUpdate is run.

But are we sure everything on stack is ok that scripts may run at this point? Where do we have the mozAutoDocUpdate?
At least I'd expect nsAutoMicroTask mt; after mozAutoPauseContentUpdate, but still hard to prove mozAutoPauseContentUpdate usage is safe.

Do we have tests for normal parsing, document.write() during parsing, document.open()/write()/close() after page load and innerHTML?
I think I don't see document.open()/write()/close() after page load tests.

r- because of mozAutoPauseContentUpdate usage. I'm having hard time to prove it is safe. Could you explain where we have mozAutoDocUpdate on stack when it is executed, and
does all the code which is executed between mozAutoPauseContentUpdate dtor and those mozAutoDocUpdate dtors expect that the world may have changed underneath?
Remember that any script may close windows, do navigations, trigger sync XHR (which spin event loop) etc. etc.

Comment 37

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #36)
> Something here looks scary. If we're in doc update (in which case there is
> script blocker on stack), we shouldn't be running microtasks.
> In practice those microtasks end up postponed to run when scripts are safe
> to run, so I when mozAutoPauseContentUpdate is run.
> 
> But are we sure everything on stack is ok that scripts may run at this
> point? Where do we have the mozAutoDocUpdate?
> At least I'd expect nsAutoMicroTask mt; after mozAutoPauseContentUpdate, but
> still hard to prove mozAutoPauseContentUpdate usage is safe.

Right, I should move nsAutoMicroTask mt; after mozAutoPauseContentUpdate.

> 
> Do we have tests for normal parsing, document.write() during parsing,
> document.open()/write()/close() after page load and innerHTML?
> I think I don't see document.open()/write()/close() after page load tests.

Do you mean document.open()/write()/close() after page load tests that also involves creating custom element synchronously during parsing or just normal tests without custom elements?
In this patch, I only added document.open()/write()/close() on custom element constructor, which should throw, per spec.

> 
> r- because of mozAutoPauseContentUpdate usage. I'm having hard time to prove
> it is safe. Could you explain where we have mozAutoDocUpdate on stack when
> it is executed, and
> does all the code which is executed between mozAutoPauseContentUpdate dtor
> and those mozAutoDocUpdate dtors expect that the world may have changed
> underneath?
> Remember that any script may close windows, do navigations, trigger sync XHR
> (which spin event loop) etc. etc.

BeginDocUpdate/EndDocUpdate on stack is called in 'nsHtml5TreeOpExecutor::RunFlushLoop()' [1] or 'nsHtml5TreeOpExecutor::FlushDocumentWrite()' [2].
Is your concern here that when we pause doc update, blocked script runners will start running? Per spec, we only forbid dynamic markup insertion here, I'm not sure how to prove it's safe for other cases, but I see there're other places that pause doc update too, like the case of 'eTreeOpAppendToDocument' [3] and 'eTreeOpStartLayout' [4].


[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/parser/html/nsHtml5TreeOpExecutor.cpp#447,490
[2] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/parser/html/nsHtml5TreeOpExecutor.cpp#555,579
[3] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/parser/html/nsHtml5TreeOperation.cpp#778,782
[4] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/parser/html/nsHtml5TreeOperation.cpp#1146

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jessica Jong [:jessica] from comment #37)
> (In reply to Olli Pettay [:smaug] from comment #36)
> > Something here looks scary. If we're in doc update (in which case there is
> > script blocker on stack), we shouldn't be running microtasks.
> > In practice those microtasks end up postponed to run when scripts are safe
> > to run, so I when mozAutoPauseContentUpdate is run.
> > 
> > But are we sure everything on stack is ok that scripts may run at this
> > point? Where do we have the mozAutoDocUpdate?
> > At least I'd expect nsAutoMicroTask mt; after mozAutoPauseContentUpdate, but
> > still hard to prove mozAutoPauseContentUpdate usage is safe.
> 
> Right, I should move nsAutoMicroTask mt; after mozAutoPauseContentUpdate.
> 
> > 
> > Do we have tests for normal parsing, document.write() during parsing,
> > document.open()/write()/close() after page load and innerHTML?
> > I think I don't see document.open()/write()/close() after page load tests.
> 
> Do you mean document.open()/write()/close() after page load tests that also
> involves creating custom element synchronously during parsing or just normal
> tests without custom elements?
tests which create custom elements during .write() calls


> BeginDocUpdate/EndDocUpdate on stack is called in
> 'nsHtml5TreeOpExecutor::RunFlushLoop()' [1] or
> 'nsHtml5TreeOpExecutor::FlushDocumentWrite()' [2].
> Is your concern here that when we pause doc update, blocked script runners
> will start running?
Yes. Suddenly starting to run scripts at a pointer where we explicitly have a script blocker on stack is super scary. Need to be very careful to change that setup.

> Per spec, we only forbid dynamic markup insertion here,
Not sure what this means.

> I'm not sure how to prove it's safe for other cases,
Need to audit all the code after 
mozAutoPauseContentUpdate dtor has run and before the mozAutoDocUpdate comes out of stack to
see if the code has proper checks for document going away or elements moved to different document or what not.

Comment 39

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8915890 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v3.

Review of attachment 8915890 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/html/nsHtml5TreeOperation.cpp
@@ +458,5 @@
> +
> +    if (mozilla::dom::CustomElementRegistry::IsCustomElementEnabled() &&
> +        (nsContentUtils::IsCustomElementName(aName) || !isValue.IsEmpty())) {
> +      nsContentUtils::SetupCustomElement(
> +        newContent, (isValue.IsEmpty() ? nullptr : &isValue));

We notice that for this case, we still need to call 'NS_NewHTMLElement' here. Consider the case where we create a custom element that is already defined from .innertHTML. 'willExecuteScript' is false for innerHTML, so it falls into this condition, but we don't enqueue an upgrade reaction for it, so the element does not get upgraded.

This issue caught by test case when removing the created callback in Bug 1396620. I'll fix it in the upcoming patch.

Comment 40

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: Complete the steps related to custom elements in "create an element for a token", v4.

Changes in this patch:

* Move `nsAutoMicroTask mt` after `mozAutoPauseContentUpdate`
* Add `dom::CustomElementDefinition*` to `NS_NewHTMLElement`'s arguments in order to avoid looking it up a second time
* Use `NS_NewHTMLElement` for creating non-synchronous custom element (this fixes the case for custom elements created from innerHTML and lots of wpt)
* Leave out the part about error reporting, that has been fixed in bug 1406297 

Note that the concern about pausing doc update is still being discussed via email.

Comment 41

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 4: Use normal for-loop instead of pointers to iterate over mOpQueue.

Per our discussion via email, Henri mentioned that "If exiting a document update batch otherwise ran scripts, the insertion point should be undefined and document.write should blow away the document instead of re-entrantly flushing the op queue."

This patch makes the code a bit safer by changing to use normal for-loop and indexing instead of pointers, so that we don't access random memory in case mOpQueue was modified.

Do you think this patch is enough to move this bug forward? Thanks.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8919216 [details] [diff] [review]
Part 4: Use normal for-loop instead of pointers to iterate over mOpQueue.

Yes, I do prefer this. At least safer, and shouldn't be too much slower.
hsivonen should review though. 

(I wonder if mOpQueue should be a linked list. But that isn't about this bug.)

Comment 43

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] (work week Oct 16-20) from comment #42)
> Comment on attachment 8919216 [details] [diff] [review]
> Part 4: Use normal for-loop instead of pointers to iterate over mOpQueue.
> 
> Yes, I do prefer this. At least safer, and shouldn't be too much slower.
> hsivonen should review though. 

It's safer as a matter of defense-in-depth, but if this change is necessary, something else is wrong.

Has the effect on speed been measured?

Comment 44

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8919216 [details] [diff] [review]
Part 4: Use normal for-loop instead of pointers to iterate over mOpQueue.

We'll deal with this in a separate bug.

Comment 45

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8918165 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v4.

Review of attachment 8918165 [details] [diff] [review]:
-----------------------------------------------------------------

Henri, I've made a small change to the case where `willExecuteScript` is false and element is a custom element (innerHTML case): we still need to use `NS_NewHTMLElement` in this case. Could you take a look again?

Olli, do you think the rest is okay? 

Thanks.

Comment 46

[Henri Sivonen (:hsivonen)]

Comment on attachment 8918165 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v4.

Review of attachment 8918165 [details] [diff] [review]:
-----------------------------------------------------------------

r+ provided that the comments are addressed. Thanks.

::: dom/html/nsHTMLContentSink.cpp
@@ +269,5 @@
>    // We only handle the "synchronous custom elements flag is set" now.
>    // For the unset case (e.g. cloning a node), see bug 1319342 for that.
>    // Step 4.
> +  CustomElementDefinition* definition = aDefinition;
> +  if (CustomElementRegistry::IsCustomElementEnabled() && !definition) {

Probably better for performance to do the simpler check first:
!definition && CustomElementRegistry::IsCustomElementEnabled()

::: parser/html/nsHtml5TreeOperation.cpp
@@ +451,5 @@
> +  } else {
> +    nsCOMPtr<dom::Element> newElement;
> +
> +    if (mozilla::dom::CustomElementRegistry::IsCustomElementEnabled() &&
> +        (nsContentUtils::IsCustomElementName(aName) || !isValue.IsEmpty())) {

Instead of running these checks again (and checking nsContentUtils::IsCustomElementName(aName) instead of aCreator == NS_NewCustomElement), I suggest storing the result of the (aCreator == NS_NewCustomElement || !isValue.IsEmpty()) check in a boolean variable before the aFromParser != dom::FROM_PARSER_FRAGMENT check earlier.

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8918165 [details] [diff] [review]
Part 3: Complete the steps related to custom elements in "create an element for a token", v4.

> 
>+class MOZ_RAII mozAutoPauseContentUpdate final
>+{
Could you move this to the parser. This class is doing something so scary that I'd
prefer to keep it close to the place where it is used, and with a comment that use with caution.

>+    } else {
>+      global = nodeInfo->GetDocument()->GetScopeObject();
>+    }
>     MOZ_ASSERT(global);
Handle the case when global is null properly. One may be able to get access to a document which don't have scope object anymore, at least in browser chrome code.
Returning some invalid state error or such should be ok.

>+  if (willExecuteScript) { // This will cause custom element constructors to run
>+    AutoSetThrowOnDynamicMarkupInsertionCounter
>+      throwOnDynamicMarkupInsertionCounter(document);
>+    mozAutoPauseContentUpdate autoPauseContentUpdate(document);
>+    {
>+      nsAutoMicroTask mt;
(just a reminder. The world may have changed after this point. Window closed or whatever. But looks like aBuilder is kept alive and so.)

Comment 48

[Jessica Jong [:jessica] (inactive)]

(In reply to Henri Sivonen (:hsivonen) from comment #46)
> Comment on attachment 8918165 [details] [diff] [review]
> Part 3: Complete the steps related to custom elements in "create an element
> for a token", v4.
> 
> Review of attachment 8918165 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ provided that the comments are addressed. Thanks.
> 
> ::: dom/html/nsHTMLContentSink.cpp
> @@ +269,5 @@
> >    // We only handle the "synchronous custom elements flag is set" now.
> >    // For the unset case (e.g. cloning a node), see bug 1319342 for that.
> >    // Step 4.
> > +  CustomElementDefinition* definition = aDefinition;
> > +  if (CustomElementRegistry::IsCustomElementEnabled() && !definition) {
> 
> Probably better for performance to do the simpler check first:
> !definition && CustomElementRegistry::IsCustomElementEnabled()

Will do.

> 
> ::: parser/html/nsHtml5TreeOperation.cpp
> @@ +451,5 @@
> > +  } else {
> > +    nsCOMPtr<dom::Element> newElement;
> > +
> > +    if (mozilla::dom::CustomElementRegistry::IsCustomElementEnabled() &&
> > +        (nsContentUtils::IsCustomElementName(aName) || !isValue.IsEmpty())) {
> 
> Instead of running these checks again (and checking
> nsContentUtils::IsCustomElementName(aName) instead of aCreator ==
> NS_NewCustomElement), I suggest storing the result of the (aCreator ==
> NS_NewCustomElement || !isValue.IsEmpty()) check in a boolean variable
> before the aFromParser != dom::FROM_PARSER_FRAGMENT check earlier.

Ok, I think it'd fine to use nsContentUtils::IsCustomElementName(aName) instead of aCreator == NS_NewCustomElement, since if custom element name is not valid, it won't have a definition, and if it doesn't have a definition, we'll have a stricter check of valid custom element name in NS_NewHTMLElement.

Comment 49

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #48)
> (In reply to Henri Sivonen (:hsivonen) from comment #46)
> > ::: parser/html/nsHtml5TreeOperation.cpp
> > @@ +451,5 @@
> > > +  } else {
> > > +    nsCOMPtr<dom::Element> newElement;
> > > +
> > > +    if (mozilla::dom::CustomElementRegistry::IsCustomElementEnabled() &&
> > > +        (nsContentUtils::IsCustomElementName(aName) || !isValue.IsEmpty())) {
> > 
> > Instead of running these checks again (and checking
> > nsContentUtils::IsCustomElementName(aName) instead of aCreator ==
> > NS_NewCustomElement), I suggest storing the result of the (aCreator ==
> > NS_NewCustomElement || !isValue.IsEmpty()) check in a boolean variable
> > before the aFromParser != dom::FROM_PARSER_FRAGMENT check earlier.
> 
> Ok, I think it'd fine to use nsContentUtils::IsCustomElementName(aName)
> instead of aCreator == NS_NewCustomElement, since if custom element name is

Oops, I meant: aCreator == NS_NewCustomElement instead of nsContentUtils::IsCustomElementName(aName).

> not valid, it won't have a definition, and if it doesn't have a definition,
> we'll have a stricter check of valid custom element name in
> NS_NewHTMLElement.

Comment 50

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 3: Complete the steps related to custom elements in "create an element for a token", v5.

Addressed review comment 46 and 47 and carrying r+.

Thanks Henri and Olli for the review!

Comment 51

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f50fe4f630ce07d6368975f4035a7139509f74e&group_state=expanded

try run on mozilla-inbound, the oranges are known failures.

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1483f7233ec
Part 1: Gecko changes for adding attribute 'is' to parser. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/84b813dad9e4
Part 2: Introduce throw-on-dynamic-markup-insertion counter. r=hsivonen
https://hg.mozilla.org/integration/mozilla-inbound/rev/11ef829cb8e6
Part 3: Complete the steps related to custom elements in "create an element for a token". r=hsivonen,smaug

Comment 53

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/e1483f7233ec
https://hg.mozilla.org/mozilla-central/rev/84b813dad9e4
https://hg.mozilla.org/mozilla-central/rev/11ef829cb8e6

Comment 55

[Jessica Jong [:jessica] (inactive)]

Sorry, I should have done this a long time ago. I'm trying to land attachment 8894791 [details] [diff] [review] into the htmlparser repo, and got 'abort: authorization failed'. Could you help me with this? Thanks.
(I can still apply the patch without conflicts with the latest code)

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Ahaa, this didn't land to htmlparser repo yet, and when I was trying to remove support for content element, is attribute handling in gecko breaks.

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Hopefully I pushed the right version of the patch
https://hg.mozilla.org/projects/htmlparser/rev/449e628fa195a15df9431abcf23def5c4078c87a

Comment 58

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #56)
> Ahaa, this didn't land to htmlparser repo yet, and when I was trying to
> remove support for content element, is attribute handling in gecko breaks.

I remembered about this when you were saying about modifying parser code today :(

Comment 59

[Henri Sivonen (:hsivonen)]

Looks like smaug already pushed the patch.

Jessica, I don't know why you got an error. I've thought anyone who has a permission to push to mozilla-inbound has the permission to push to the htmlparser repo, too. I suggest asking asking the infrastructure operations folks on IRC.