85776 - Incorrect exception thrown for setNamedItem(newchild)

Status: RESOLVED INVALID

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

Description

[Sivakiran Tummala]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.1)
Gecko/20010605 Netscape6/6.1b1
BuildID:    2001060509

Incorrect exception thrown for setNamedItem(newchild).
i was expecting "NO_MODIFICATION_ALLOWED_ERR" exception to be thrown but an
exception was thrown which was not listed in DOM 2 specs.
i am attaching a simple testcase to demonstrate this.

Reproducible: Always
Steps to Reproduce:
1.load the testcase 



Actual Results:  NO_MODIFICATION_ALLOWED_ERR expected

Expected Results:  unknown exception was thrown

Comment 1

[Sivakiran Tummala]

the testcase is located at http://h-208-12-38-201.mcom.com/cdex027.html

Comment 2

[Sivakiran Tummala]

Attachment: testcase

Comment 3

[Johnny Stenback (:jst)]

Future, not a show stopper

Comment 4

[Fabian Guisset]

To make it clear what the problem is:
The reduced testcase is:
******************************************************
<button id="btn" type="submit"/>

var btn = document.getElementById("btn");
var attrs = btn.attributes;
var newAttr = document.createAttribute("name");
attrs.setNameItem(newAttr);

The DOM2 Core spec states that if the NamedNodeMap is readonly (as is the case
with the attributes properties of Node), then setNamedItem(), setNamedItemNS(),
removeNamedItem(), and removeNamedItemNS() should throw a
NO_MODIFICATION_ALLOWED_ERR.

Since the |attributes| properties of Elements is implemented by
nsDOMAttributeMap.cpp, I assume that we should modify
nsDOMAttributeMap::SetNamedItem() and the 3 other methods to return
NO_MODIFICATION_ALLOWED_ERR in all cases.

However perhaps jst and sicking had other plans for this (with the attribute
revamp). If the reasoning above is correct, I'll come up with a patch.
Otherwise, we'll just wait till the attribute revamp lands.

On a side note, the class implementing NamedNodeMap for things other than
attributes, nsXMLNamedNodeMap, doesn't have a check for this either. I don't
know how we could figure out that the NamedNodeMap we are implementing is
readonly or not. Will that require to change the constructor to take an extra
argument?

Comment 5

[Johnny Stenback (:jst)]

Fabian, I doubt the attr rewamp (whenever that happens) will affect how we deal
with attribute nodes, so it'd be great if you could come up with a patch :-)

As for how a named node map can know if it's readonly or not, either the named
node map needs internal state (which could be set at construction time, for
instance), or we need separate classes for the readonly and readwrite named node
maps (this means more code, but potentially smaller instances of these classes).

Comment 6

[Fabian Guisset]

Attachment: Useless fix.

Here comes another injection of daily patch.

Comment 7

[Johnny Stenback (:jst)]

Fabian, seems like I missed this paragraph last time I read your comment above:

> The DOM2 Core spec states that if the NamedNodeMap is readonly (as is the case
> with the attributes properties of Node), then setNamedItem(), setNamedItemNS(),
> removeNamedItem(), and removeNamedItemNS() should throw a
> NO_MODIFICATION_ALLOWED_ERR.

This is not true, the DOM Core spec does say that the attributes property of a
Node is readonly, but that doesn't mean that the NamedNodeMap that the property
references is readonly. It only means that you can't set Node.attributes to an
other NamedNodeMap, but you can still modify the NamedNodeMap that
Node.attributes references.

Comment 8

[Fabian Guisset]

Comment on attachment 67725 [details] [diff] [review]
Useless fix.

Ah ok... but then in which case is a readonly NamedNodeMap created? This patch
is useless, marking obsolete.

Comment 9

[Johnny Stenback (:jst)]

The document.doctype.entities and .notations NamedNodeMaps are readonly, but we
don't support those yet.

Comment 10

[Fabian Guisset]

Ok then I don't understand this bug...
Sivakiran could you explain? Why did you expect NO_MODIFICATION_ALLOWED_ERR to
be thrown when using SetNamedItem() on the attributes NamedNodeMap? The DOM spec
states NO_MODIFICATION_ALLOWED_ERR should be thrown only for readonly
NamedNodeMaps, but the attributes properties isn't readonly according to jst.
(sorry if this is obvious, i'm just trying to understand :-)

By the way Siva, the testcase you attached has something bogus:
var sNewChild = document.createAttribute("HR").nodeName;

Comment 11

[Sivakiran Tummala]

Fabian,
var sNewChild = document.createAttribute("HR").nodeName returns the nodeName.
setNamedItem method expects Node as arg. 

Why should not we throw an exception when an attribute properties of Node is 
readony?
I am confused too :)

Comment 12

[Fabian Guisset]

Siva,
first thing first, there is no such thing as a "hr" attribute, although I guess
that's not a problem.
Second thing second, setNamedItem() expects a "Node" attribute (as you said it
yourself). However .nodeName returns a _DOMString_ with the name of the Node.
You have to pass the node itself to setNamedItem.
Even then, if you were indeed passing the attribute Node itself as argument to
setNamedItem(), it would still be wrong to expect a NO_MODIFICATION_ALLOWED_ERR,
because NO_MODIFICATION_ALLOWED_ERR should only be thrown for readonly
NamedNodeMap's (according to the spec), and the .attributes NamedNodeMap is
_not_ readonly (see comment #7).

Thoughts?

Comment 13

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

the difference is:

The attribute is readonly, but the attribute_value_ is a read/write 
NamedNodeMap. So you can't do

myNode.attributes = myNamedNodeMap;

but you can do:

myNode.attributes.setNamedItem(myItem);

Comment 14

[basic]

remove self

Comment 15

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 16

[Peter Van der Beken [:peterv]]

The testcase is invalid, we throw a NS_ERROR_XPC_BAD_CONVERT_JS exception (which
is not a DOM exception) because the passed in argument is a string (.nodeName)
instead of a Attr. Furthermore, we do throw an JS exception when you try to set
.attributes (setting a property that has only a getter).