47852 - NAMESPACE_ERR not being thrown when setting Node.prefix

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; m18) Gecko/20000730
BuildID:    2000073020

If a Node.prefix is set when the Node.namespaceURI is null, NAMESPACE_ERR should
be thrown.  Whenever an object (Document, ELement, etc) is created with a NS
method with a null namespaceURI and a prefix in the object's name, then a
NAMESPACE_ERR should be thrown. With the exception of the CharacterData
interfaces (CDATASection, Comment, Text) NAMESPACE_ERR is not being thrown.

In addition, if the xml or xmlns prefixes are used when creating an object, then
specific namespaceURI *must* be used or else a NAMESPACE_ERR should be thrown.
This is not happening either.

Reproducible: Always

Steps to Reproduce:

1.  document.prefix = 0
    Expect NAMESPACE_ERR exception
    Actual NO_MODIFICATION_ALLOWED_ERR exception

2.  document.documentElement.prefix = 0
    Expect NAMESPACE_ERR exception
    Actual No Exception thrown

3.  document.implementation.createDocument(null, 'x:y', null);
    Expect NAMESPACE_ERR
    Actual No Exception

4.  document.implementation.createDocument('http://baduri', 'xml:stuff', null)
    Expect NAMESPACE_ERR
    Actual No Exception

5.  document.implementation.createDocument('http://baduri', 'xmlns:stuff', null)
    Expect NAMESPACE_ERR
    Actual No Exception

I believe this to be a problem in the create...NS methods as well.

See: http://www.w3.org/TR/2000/CR-DOM-Level-2-20000510/core.html#ID-NodeNSPrefix

Comment 1

[Johnny Stenback (:jst)]

This bug has been marked "future" because the original netscape engineer
working on this is over-burdened. If you feel this is an error, that you or
another known resource will be working on this bug,or if it blocks your work
in some way -- please attach your concern to the bug for reconsideration.

Comment 2

[Ian Graham]

Should NAMESPACE_ERR (and other, similar) exceptions be thrown at 
parse time?  Mozilla happily  reads in XML that violates the XML 
names specification. Here are three examples illustrating some 
error conditions:

EXAMPLE  1
==========
<?xml version="1.0" ?>
<!-- This should fail - the two prefixes 'abc' and 'foo' map 
     onto the same namespace, so the two attributes 'foo:name' and
     'abc:name' collide
     -->
<xmltest xmlns:abc="http://www.foo.org"  
         xmlns:html="http://www.w3.org/TR/REC-html40" 
         xmlns:foo="http://www.foo.org"
         foo:name="value"
	 abc:name="value2">
    <html:script src="tree-traverse.js" />
</xmltest>

EXAMPLE 2
==========
<?xml version="1.0" ?>
<!-- This should raise a NAMESPACE_ERR exception because
     the Qname foo:name uses a prefix that has no declared 
     namespace" -->
<xmltest foo:name="value"  xmlns:html="http://www.w3.org/TR/REC-html40" >
 <html:script src="tree-traverse.js" />
</xmltest>

EXAMPLE 3
=========
<?xml version="1.0" ?>
<!-- This should fail - you're not allowed to declare
     a null value for a prefixed attribute namespace declaration
     like xmlns:foo
     -->
<xmltest xmlns:foo=""
         xmlns:html="http://www.w3.org/TR/REC-html40" 
         foo:name="value" >
 <html:script src="tree-traverse.js" />
</xmltest>

Comment 3

[Johnny Stenback (:jst)]

Yes, all those should fail to parse, and there's a but for that already, this
ain't it tho.

Comment 4

[Johnny Stenback (:jst)]

Mass-reassigning bugs.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Testcases showing the problem per comment 0 would be nice here -- I can't tell
whether peterv's changes fixed this or not.

Comment 6

[Peter Van der Beken [:peterv]]

It doesn't, but once I fix bug 233907 some of those will be fixed.

Comment 7

[Bob Clary [:bc] (inactive)]

The old xb* TS shows some of these. See above URL. Click on the test results or
the method/property to see the results in the right frame. Scroll to the red
entries to see failed tests. (Green is passed and Black is skipped). To run the
full set of tests go to
<http://archive.bclary.com/dom-ts/xbtests/tc.html?txtTitle=DOM%20Core%20Level%202%20Conformance%20Tests&txtApiPath=w3c-dom-core-2/&txtTestCases=tc&txtInvariantApiTestFrame=tc_invariantTestFrame&txtMutantApiTestFrame=tc_mutantTestFrame>

Yes, they are not very friendly. 

Similar tests are available in the DOM TS although I am working on getting test
selection to work so that all tests can be run...

Attr.prefix
Document.createAttributeNS
Document.createElementNS
Document.documentElement.prefix
Document.implementation.createDocument
Document.implementation.createDocumentType
Document.prefix
Element.prefix

I Think CharacterData prefix stuff is ok.

Comment 8

[Peter Van der Beken [:peterv]]

Let's refocus this bug a bit. Of the problems listed in comment 0, #3 is fixed, #4 is fixed and #5 isn't a problem according to the spec (http://www.w3.org/TR/2000/CR-DOM-Level-2-20000510/core.html#Level-2-Core-DOM-createDocument). 

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

>Index: content/base/src/nsDOMAttribute.cpp
...
>   if (!aPrefix.IsEmpty()) {
>     prefix = do_GetAtom(aPrefix);
>+    if (!prefix) {
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    if (mNodeInfo->NamespaceID() == kNameSpaceID_None ||
>+        (prefix == nsGkAtoms::xml &&
>+         mNodeInfo->NamespaceID() != kNameSpaceID_XML) ||
>+        (prefix == nsGkAtoms::xmlns &&
>+         mNodeInfo->NamespaceID() != kNameSpaceID_XMLNS) ||
>+        (mNodeInfo->NameAtom() == nsGkAtoms::xmlns &&
>+         !mNodeInfo->GetPrefixAtom())) {

You need to check for

(prefix != nsGkAtoms::xmlns &&
 mNodeInfo->NamespaceID() != kNameSpaceID_XMLNS)

You should also make sure that the prefix isn't set to blank when the namespace is the xmlns namespace.

>Index: content/base/src/nsGenericElement.cpp
>@@ -1072,6 +1072,12 @@ nsGenericElement::SetPrefix(const nsAStr
>   if (!aPrefix.IsEmpty()) {
>     prefix = do_GetAtom(aPrefix);
>     NS_ENSURE_TRUE(prefix, NS_ERROR_OUT_OF_MEMORY);
>+
>+    if (mNodeInfo->NamespaceID() == kNameSpaceID_None ||
>+        (prefix == nsGkAtoms::xml &&
>+         mNodeInfo->NamespaceID() != kNameSpaceID_XML)) {
>+       return NS_ERROR_DOM_NAMESPACE_ERR;
>+    }

You don't want to test for xmlns here?

Comment 11

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> You need to check for
> 
> (prefix != nsGkAtoms::xmlns &&
>  mNodeInfo->NamespaceID() != kNameSpaceID_XMLNS)

Huh? Setting the prefix to anything but 'xmlns' when the namespace is not http://www.w3.org/2000/xmlns/ should throw?

> You should also make sure that the prefix isn't set to blank when the
> namespace is the xmlns namespace
...
> You don't want to test for xmlns here?

Hey, you got yourself an action item: raise these issues on the DOM Level 3 Core spec (http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-NodeNSPrefix). It talks specifically about just attributes when checking for xmlns, and it doesn't say anything about setting prefixes to something different from xml or xmlns when the node has the xml or xmlns namespace.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #11)
> (In reply to comment #10)
> > You need to check for
> > 
> > (prefix != nsGkAtoms::xmlns &&
> >  mNodeInfo->NamespaceID() != kNameSpaceID_XMLNS)
> 
> Huh? Setting the prefix to anything but 'xmlns' when the namespace is not
> http://www.w3.org/2000/xmlns/ should throw?

err, i meant

(prefix != nsGkAtoms::xmlns &&
 mNodeInfo->NamespaceID() == kNameSpaceID_XMLNS)


> 
> > You should also make sure that the prefix isn't set to blank when the
> > namespace is the xmlns namespace
> ...
> > You don't want to test for xmlns here?
> 
> Hey, you got yourself an action item: raise these issues on the DOM Level 3
> Core spec (http://www.w3.org/TR/DOM-Level-3-Core/core.html#ID-NodeNSPrefix).
> It talks specifically about just attributes when checking for xmlns, and it
> doesn't say anything about setting prefixes to something different from xml or
> xmlns when the node has the xml or xmlns namespace.

In the meantime we can be innovative :). Seriously though, it makes no sense to either use the xmlns prefix or the xmlns namespace on elements, but I can raise it with the WG if you prefer.

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: v1.1

I wasn't arguing against doing it, but we should make sure this gets noted in an errata.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

r/sr=me

Comment 15

[Peter Van der Beken [:peterv]]

Attachment: v2

Decided to do this slightly differently (merging things into nsContentUtils). I also realized I should add code to the serializer that ensures anything in the xml namespace always has an xml prefix.