233907 - document.createElementNS() doesn't throw NAMESPACE_ERR when given a namespace-invalid name.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Now that bug 16603 is fixed, we're closer to being compliant wrt
document.createElementNS() et al, but we're not quite there yet. We now properly
throw INVALID_CHARACTER_ERR in most cases, but we don't throw NAMESPACE_ERR if
the provided qname is not namespace-wellformed. I.e. the following should IMO
throw INVALID_CHARACTER_ERR

  javascript:document.createElementNS('http://foo', 'a<')
  javascript:document.createElementNS('http://foo', 'a:<')
  javascript:document.createElementNS('http://foo', 'a:b:c<')

But the following should throw NAMESPACE_ERR

  javascript:document.createElementNS('http://foo', 'a:')
  javascript:document.createElementNS('http://foo', 'a:5b')
  javascript:document.createElementNS('http://foo', 'a:b:')

and so on...

Comment 1

[Peter Van der Beken [:peterv]]

I'd argue that javascript:document.createElementNS('http://foo', 'a:b:c<')
should throw NAMESPACE_ERR, since the first error encountered is the second
colon. I could change the code to continue checking after it encounters the
invalid extra colons but I'd rather not, so let me know if you feel strongly
about this.

Comment 2

[Peter Van der Beken [:peterv]]

Er, replace NAMESPACE_ERR with INVALID_CHARACTER_ERR in comment 1.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 142846 [details] [diff] [review]
v1

So this reports the first error encountered, a:b:c< will report a
NAMESPACE_ERR, not an INVALID_CHARACTER_ERR.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 142918 [details] [diff] [review]
v1.1

- In nsDOMImplementation::CreateDocument():

   if (!aQualifiedName.IsEmpty()) {
...
+  }
+  else if (DOMStringIsNull(aQualifiedName) &&
+	    !DOMStringIsNull(aNamespaceURI)) {
+    return NS_ERROR_DOM_NAMESPACE_ERR;

Shouldn't that be:

+  else if (DOMStringIsNull(aQualifiedName) &&
+	    !aNamespaceURI.IsEmpty()) {
+    return NS_ERROR_DOM_NAMESPACE_ERR;

A null string is always empty, but an empty string is not always null.

- In moz_extensions.c:

+#define MOZ_EXPAT_VALID_QNAME	     0
+#define MOZ_EXPAT_EMPTY_QNAME	     1
+#define MOZ_EXPAT_INVALID_CHARACTER 2
+#define MOZ_EXPAT_MALFORMED	     4

Make that:

+#define MOZ_EXPAT_VALID_QNAME	     (0)
+#define MOZ_EXPAT_EMPTY_QNAME	     (1 << 0)
+#define MOZ_EXPAT_INVALID_CHARACTER (1 << 1)
+#define MOZ_EXPAT_MALFORMED	     (1 << 2)

- In MOZ_XMLCheckQName():

+  if (ptr == end) {
+    return MOZ_EXPAT_EMPTY_QNAME;
   }
   while (ptr != end) {

This could be flipped to a do { } while () loop to avoid checking ptr != end on
the first iteration since you know it's not equal.

- In nsParserService::CheckQName():

Hmm, this method is inline, does it need to be?

+    // MOZ_EXPAT_INVALID_CHARACTER
+    if (result & 2) {

There's no good shared header file we could put MOZ_EXPAT_INVALID_CHARACTER in
so that we wouldn't need to hardcode '2' (or 1 << 1) here?

r+sr=jst

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to comment #7)
> (From update of attachment 142918 [details] [diff] [review])
> - In nsDOMImplementation::CreateDocument():
> 
>    if (!aQualifiedName.IsEmpty()) {
> ...
> +  }
> +  else if (DOMStringIsNull(aQualifiedName) &&
> +	    !DOMStringIsNull(aNamespaceURI)) {
> +    return NS_ERROR_DOM_NAMESPACE_ERR;
> 
> Shouldn't that be:
> 
> +  else if (DOMStringIsNull(aQualifiedName) &&
> +	    !aNamespaceURI.IsEmpty()) {
> +    return NS_ERROR_DOM_NAMESPACE_ERR;
> 
> A null string is always empty, but an empty string is not always null.

Nope, see
http://www.w3.org/TR/2004/PR-DOM-Level-3-Core-20040205/core.html#Level-2-Core-DOM-createDocument
"NAMESPACE_ERR: Raised if ..., or if the qualifiedName is null and the
namespaceURI is different from null..."

> - In nsParserService::CheckQName():
> 
> Hmm, this method is inline, does it need to be?

No, not really. It used to be simple but I'll uninline it.

> +    // MOZ_EXPAT_INVALID_CHARACTER
> +    if (result & 2) {
> 
> There's no good shared header file we could put MOZ_EXPAT_INVALID_CHARACTER in
> so that we wouldn't need to hardcode '2' (or 1 << 1) here?

No easy one unfortunately. Importing Mozilla headers into expat is hell, and I
prefer to keep changes to other expat files to a minimum. I could maybe add it
to xmlparse.h but I'd rather do that once we moved all those expat files into
one directory (expat/lib).