Bug 126553 – createElementNS does not handle 'xmlns' name correctly

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

[Victor Costan]

== What steps will reproduce the problem?

1. document.createElementNS('http://www.w3.org/XML/1998/namespace', 'xml:abc', 'foo')
2. document.createElementNS('http://www.w3.org/not-XML/1998/namespace', 'xml:abc', 'foo')

== What is the expected result?

The first line should create an element, the second one should throw a NamespaceError.

== What happens instead?

The first line throws NamespaceError, the second line creates an element.

== Please provide any additional information below.


DOM3 spec: http://www.w3.org/TR/2004/REC-DOM-Level-3-Core-20040407/core.html#ID-DocCrElNS

NAMESPACE_ERR: Raised if [[irrelevant part removed]] , or if the qualifiedName or its prefix is "xmlns" and the namespaceURI is different from "http://www.w3.org/2000/xmlns/", or if the namespaceURI is "http://www.w3.org/2000/xmlns/" and neither the qualifiedName nor its prefix is "xmlns".

DOM4 spec: http://www.w3.org/TR/dom/#dom-document-createelementns
7. If qualifiedName or prefix is "xmlns" and namespace is not the XMLNS namespace, throw a "NamespaceError" exception.
8. If namespace is the XMLNS namespace and neither qualifiedName nor prefix is "xmlns", throw a "NamespaceError" exception.

Internet Explorer, Firefox and Chrome match the specification.

I will submit a patch that fixes this bug soon.

[Victor Costan]

Created attachment 220476 [details]
Patch

[Victor Costan]

Adam, can you please take a look?

This is ported from a Blink CL that you recently reviewed.
https://codereview.chromium.org/122083002/

Is it OK to remove the whitespace from Document.cpp, or should I use a text editor that doesn't do that?

Thank you!

[Sam Weinig]

(In reply to comment #2)
> Adam, can you please take a look?
> 
> This is ported from a Blink CL that you recently reviewed.
> https://codereview.chromium.org/122083002/
> 
> Is it OK to remove the whitespace from Document.cpp, or should I use a text editor that doesn't do that?
> 
> Thank you!

Please use a text editor that doesn't do that.

[Sam Weinig]

Comment on attachment 220476 [details]
Patch

Marking r- for the extraneous whitespace changes.

[Victor Costan]

Created attachment 220494 [details]
Patch

[Victor Costan]

Thank you for your guidance, Sam!

Can you please take another look?

[Alexey Proskuryakov]

Comment on attachment 220494 [details]
Patch

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

> LayoutTests/ChangeLog:16
> +        * fast/dom/createElementNS-namespace-errors-expected.txt: Added.
> +        * fast/dom/createElementNS-namespace-errors.html: Added. Covers all the corner cases in the DOM4 spec.
> +        * fast/dom/script-tests/setAttributeNS-prefix-and-null-namespace.js: Removed.
> +        * fast/dom/setAttributeNS-expected.txt: Updated to match cleaned up test.
> +        * fast/dom/setAttributeNS-namespace-errors-expected.txt: Added.
> +        * fast/dom/setAttributeNS-namespace-errors.html: Added. Covers all the corner cases in the DOM4 spec.
> +        * fast/dom/setAttributeNS-prefix-and-null-namespace-expected.txt: Removed.
> +        * fast/dom/setAttributeNS-prefix-and-null-namespace.html: Removed. Subsumed by setAttributeNS-namespace-errors.
> +        * fast/dom/setAttributeNS.html: Cleaned up.

Do all these tests fully pass in Firefox now?

[Victor Costan]

Alexey, the tests "almost" pass in Firefox. Some assertions check for specific error messages, and those fail in Firefox. In all the cases, Firefox also throws an error with code 14, but its message looks different.

[Alexey Proskuryakov]

Comment on attachment 220494 [details]
Patch

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

> LayoutTests/fast/dom/createElementNS-namespace-errors.html:10
> +shouldThrow("document.createElementNS(null, 'foo:bar', 'baz')", '"Error: NamespaceError: DOM Exception 14"');

Might be a stupid question... What is the third argument in all the tests?

> LayoutTests/fast/dom/createElementNS-namespace-errors.html:18
> +shouldNotThrow("document.createElementNS('http://www.w3.org/2000/xmlns/', 'xmlns', 'http://wwww.example.org')");
> +shouldThrow("document.createElementNS('http://www.w3.org/2000/not-xmlns/', 'xmlns', 'http://wwww.example.org')", '"Error: NamespaceError: DOM Exception 14"');

Might be nice to test for what elements these actually create, not just whether they raise exceptions or not. That's far from obvious, I think.

[Victor Costan]

Created attachment 220541 [details]
Patch

[Victor Costan]

(In reply to comment #9)
> (From update of attachment 220494 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220494&action=review
> 
> > LayoutTests/fast/dom/createElementNS-namespace-errors.html:10
> > +shouldThrow("document.createElementNS(null, 'foo:bar', 'baz')", '"Error: NamespaceError: DOM Exception 14"');
> 
> Might be a stupid question... What is the third argument in all the tests?

It's just a proof that I copy-pasted the tests I used in setAttributeNS. Sorry and thank you for catching that!

> 
> > LayoutTests/fast/dom/createElementNS-namespace-errors.html:18
> > +shouldNotThrow("document.createElementNS('http://www.w3.org/2000/xmlns/', 'xmlns', 'http://wwww.example.org')");
> > +shouldThrow("document.createElementNS('http://www.w3.org/2000/not-xmlns/', 'xmlns', 'http://wwww.example.org')", '"Error: NamespaceError: DOM Exception 14"');
> 
> Might be nice to test for what elements these actually create, not just whether they raise exceptions or not. That's far from obvious, I think.

Done. Can you please take a look and tell me if this is what you had in mind?

[WebKit Commit Bot]

Comment on attachment 220541 [details]
Patch

Clearing flags on attachment: 220541

Committed r161464: <http://trac.webkit.org/changeset/161464>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Victor Costan]

Thank you very much for the quick turnaround time, Alexey!