1462806 - The [is] attribute is not passed if element is created by nsXMLContentSink (and maybe elsewhere)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

We will need to be able to insert element from a mark-up string, possible from the document fragment created by the function in bug 1462798.

The test cases in bug 1460962 covered all the other element creation cases and they all work, but not this one, which have the element created from here.

https://searchfox.org/mozilla-central/rev/9769f2300a17d3dfbebcfb457b1244bd624275e3/dom/xml/nsXMLContentSink.cpp#478

There are other problems in the code too in which element created by DOMParser won't even pick up autonomous custom element, but it works in the browser console. I had to figure out that too to properly test this.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: diff.patch

The nsXMLContentSink case has been fixed. Still, need to fix the test part.

For some reasons, the document created by the DOMParser does not have custom element enabled. This however only happens in the test file?

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1462806 - Extract is value from nsXMLContentSink

Nodes copied from DOMParser document fragment would need to be
created with the proper custom element data.

CustomElementRegistry::IsCustomElementEnabled() is changed to allow
it to run in the test document.

Review commit: https://reviewboard.mozilla.org/r/245266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245266/

Comment 4

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

(bug 1402796 was tracking this work)

Comment 5

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

We really need wpt tests for this, assuming [is] isn't handled properly on xhtml documents.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

https://reviewboard.mozilla.org/r/245266/#review251270

::: dom/xml/nsXMLContentSink.cpp:482
(Diff revision 1)
>    RefPtr<Element> content;
> -  rv = NS_NewElement(getter_AddRefs(content), ni.forget(), aFromParser);
> +
> +  const nsAString* is = nullptr;
> +  if (aNodeInfo->NamespaceEquals(kNameSpaceID_XHTML) ||
> +      aNodeInfo->NamespaceEquals(kNameSpaceID_XUL)) {
> +    RefPtr<nsAtom> prefix, localName;

Maybe I should move `nsXBLContentSink::FindValue()` to `nsXMLContentSink` and use it here?

https://searchfox.org/mozilla-central/rev/9769f2300a17d3dfbebcfb457b1244bd624275e3/dom/xbl/nsXBLContentSink.cpp#576

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #5)
> We really need wpt tests for this, assuming [is] isn't handled properly on
> xhtml documents.

Is this the test?
https://raw.githubusercontent.com/w3c/web-platform-tests/master/custom-elements/parser/parser-uses-create-an-element-for-a-token-svg.svg

We disable it currently.
https://searchfox.org/mozilla-central/rev/0e80033a10ca4f9940c083a33d175c99ab3568e5/testing/web-platform/meta/custom-elements/parser/parser-uses-create-an-element-for-a-token-svg.svg.ini

I can run the test in my local build later to see if my patch fixes this.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Yeah, it passes with my patch locally.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245266/diff/1-2/

Comment 10

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

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

https://reviewboard.mozilla.org/r/245266/#review251218

::: dom/base/CustomElementRegistry.cpp:303
(Diff revision 1)
>      return true;
>    }
>  
> -  return XRE_IsParentProcess() && nsContentUtils::AllowXULXBLForPrincipal(aDoc->NodePrincipal());
> +  return XRE_IsParentProcess() &&
> +    (nsContentUtils::AllowXULXBLForPrincipal(aDoc->NodePrincipal()) ||
> +      aDoc->AllowXULXBL());

extra space before aDoc

::: dom/xml/nsXMLContentSink.cpp:490
(Diff revision 2)
> +      nsContentUtils::SplitExpatName(aAtts[0], getter_AddRefs(prefix),
> +                                     getter_AddRefs(localName), &nameSpaceID);
> +
> +      if (localName == nsGkAtoms::is) {
> +        nsDependentString val(aAtts[1]);
> +        is = &val;

So, val is a valid object while it is on stack, and it is on stack only until the end of the block.
So /is/ ends up pointing to a deleted object once we're out of the scope.

Comment 11

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

thanks for looking into wpt results and I guess that existing test is good enough for now.

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245266/diff/2-3/

Comment 13

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

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

https://reviewboard.mozilla.org/r/245266/#review251364

::: dom/base/CustomElementRegistry.cpp:305
(Diff revision 3)
>      return true;
>    }
>  
> -  return XRE_IsParentProcess() && nsContentUtils::AllowXULXBLForPrincipal(aDoc->NodePrincipal());
> +  return XRE_IsParentProcess() &&
> +    (nsContentUtils::AllowXULXBLForPrincipal(aDoc->NodePrincipal()) ||
> +     aDoc->AllowXULXBL());;

You don't need these both. aDoc->AllowXULXBL() should be enough.
AllowXULXBL calls the contentutils method internally
https://searchfox.org/mozilla-central/rev/8affe6e83188787eb61fe0528eeb6eef6081ba06/dom/base/nsDocument.cpp#4958

::: dom/xml/nsXMLContentSink.cpp:465
(Diff revision 3)
>    mParser = aParser;
>    return NS_OK;
>  }
>  
> +static bool
> +FindIsAttrValue(const char16_t **aAtts, const char16_t **aResult)

** should go with the type, not with the argument name.
So, const char16_t** aAtts
and same with aResult

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8978475 [details]
Bug 1462806 - Extract is value from nsXMLContentSink

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/245266/diff/3-4/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c00eeb83675
Extract is value from nsXMLContentSink r=smaug

Comment 16

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2c00eeb83675