441417 - DOM exception when mixing empty and non-empty namespaces

Status: RESOLVED FIXED

(Rhino Graveyard :: E4X, defect)

Description

[Matthieu Riou]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9) Gecko/2008060309 Firefox/3.0
Build Identifier: 1.7RC1

This isn't really easy to describe so you'll have to bear with me a little (and have a look at the steps to reproduce first).

When an expression like friendInfo.name gets executed, a new name element is created as a child of friendInfo. The new element qname matches the qname of the previous element if there was one before (friendInfo already had a name child). However when the new element gets created, the default namespace lookup happens on the parent node (the friendInfo element in this example) which does have a qname. So the code ends up creating an element with an empty namespace (as was previously) but with a prefix which is assumed to match the parent. That doesn't work quite well in DOM land.


Reproducible: Always

Steps to Reproduce:
Just type the following in a Rhino interpreter:

var friendInfo = <friend xmlns="http://ode.apache.org/simpel/1.0/definition/XmlData"><name xmlns=""/><phone xmlns=""/></friend>;
var msgIn = <xd:dataOpRequest xmlns:xd="http://ode.apache.org/simpel/1.0/definition/XmlData">
                    <person>
                       <firstName>John</firstName>
                       <lastName>Doe</lastName>
                       <phone>[phone]</phone>
                    </person>
                </xd:dataOpRequest>;
friendInfo.name = msgIn.person.firstName.text() + " " + msgIn.person.lastName.text();

Actual Results:  
Exception in thread "main" org.w3c.dom.DOMException: NAMESPACE_ERR: An attempt is made to create or change an object in a way which is incorrect with regard to namespaces.
        at com.sun.org.apache.xerces.internal.dom.ElementNSImpl.setName(ElementNSImpl.java:176)
        at com.sun.org.apache.xerces.internal.dom.ElementNSImpl.<init>(ElementNSImpl.java:112)
        at com.sun.org.apache.xerces.internal.dom.CoreDocumentImpl.createElementNS(CoreDocumentImpl.java:1969)
        at com.sun.org.apache.xerces.internal.dom.CoreDocumentImpl.importNode(CoreDocumentImpl.java:1487)
        at com.sun.org.apache.xerces.internal.dom.CoreDocumentImpl.importNode(CoreDocumentImpl.java:1444)
        at org.mozilla.javascript.xmlimpl.XmlNode.insertChildAt(XmlNode.java:223)
        at org.mozilla.javascript.xmlimpl.XmlNode.insertChildrenAt(XmlNode.java:237)
        at org.mozilla.javascript.xmlimpl.XML.insertChildAfter(XML.java:531)
        at org.mozilla.javascript.xmlimpl.XML.replace(XML.java:478)
        at org.mozilla.javascript.xmlimpl.XMLName.setMyValueOn(XMLName.java:341)
        at org.mozilla.javascript.xmlimpl.XML.putXMLProperty(XML.java:195)
        at org.mozilla.javascript.xmlimpl.XMLObjectImpl.ecmaPut(XMLObjectImpl.java:299)
        at org.mozilla.javascript.ScriptRuntime.setObjectProp(ScriptRuntime.java:1515)
        at org.mozilla.javascript.ScriptRuntime.setObjectProp(ScriptRuntime.java:1507)
        at org.mozilla.javascript.Interpreter.interpretLoop(Interpreter.java:3063)
        at org.mozilla.javascript.Interpreter.interpret(Interpreter.java:2484)
        at org.mozilla.javascript.InterpretedFunction.call(InterpretedFunction.java:162)
        at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:401)
        at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3003)
        at org.mozilla.javascript.InterpretedFunction.exec(InterpretedFunction.java:173)
        at org.mozilla.javascript.tools.shell.Main.evaluateScript(Main.java:526)
        at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:386)
        at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:179)
        at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:100)
        at org.mozilla.javascript.Context.call(Context.java:499)
        at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:511)
        at org.mozilla.javascript.tools.shell.Main.exec(Main.java:162)
        at org.mozilla.javascript.tools.shell.Main.main(Main.java:140)

Comment 1

[Matthieu Riou]

Attachment: Patch to apply on xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java

Comment 2

[Norris Boyd]

I tried the patch and got this:

e4x-compile:
    [javac] Compiling 11 source files to /home/norris/rhino17/mozilla/js/rhino/build/classes
    [javac] /home/norris/rhino17/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java:92: cannot find symbol
    [javac] symbol  : method getUri()
    [javac] location: class org.mozilla.javascript.xmlimpl.XmlNode.QName
    [javac]         if (qname.getUri().length() == 0)
    [javac]                  ^
    [javac] /home/norris/rhino17/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java:94: cannot find symbol
    [javac] symbol  : method getUri()
    [javac] location: class org.mozilla.javascript.xmlimpl.XmlNode.QName
    [javac]         else e = document.createElementNS(qname.getUri(), qname.qualify(referenceDom));
    [javac]                                                ^
    [javac] 2 errors

Comment 3

[Matthieu Riou]

Attachment: Patch updated to apply on Rhino trunk

Comment 4

[Matthieu Riou]

Sorry about that, I created the patch against 1.7RC1 and didn't notice that some cleanup had been done on trunk since then. I've attached a new patch that should work on trunk.

Comment 5

[Matthieu Riou]

Can someone have a look at this? I've updated the patch last time.

Comment 6

[Norris Boyd]

I'll take a look.

Comment 7

[Norris Boyd]

I patched in the change (here's my context diff, with minor formatting changes):

RCS file: /cvsroot/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java,v
retrieving revision 1.10
diff -u -r1.10 XmlNode.java
--- xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java      2 Jul 2008 18:16:35 -0000       1.10
+++ xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java      20 Aug 2008 18:17:37 -0000
@@ -94,7 +94,15 @@
             document = processor.newDocument();
         }
         Node referenceDom = (reference != null) ? reference.dom : null;
-        Element e = document.createElementNS(qname.getNamespace().getUri(), qname.qualify(referenceDom));
+        Element e;
+        if (qname.getNamespace() == null ||
+            qname.getNamespace().getUri().length() == 0)
+        {
+            e = document.createElement(qname.getLocalName());
+        } else {
+            e = document.createElementNS(qname.getNamespace().getUri(),
+                qname.qualify(referenceDom));
+        }
         if (value != null) {
             e.appendChild(document.createTextNode(value));
         }


The following tests start failing after making the change:

e4x/Expressions/11.1.2.js
e4x/Expressions/11.2.2.js
e4x/Regress/regress-264369.js
e4x/Regress/regress-369032.js
e4x/Types/9.1.1.2.js

Comment 8

[Matthieu Riou]

Ah sorry, I didn't where the tests were before. Thanks for checking this, it looks like I'll have to do another deep dive in that code.

Comment 9

[Matthieu Riou]

I think I've found the problem and fixed my patch. Sorry about the back and forth, XML namespaces are a real pain in the neck. I'd like to try the tests on it but don't know which bug list you typically use. Do you always test with all the 2618? Seems to be taking a looong time... :)

Anyway I'll attach my patch still but if you could let me know what you usually run for the tests, that'd be great.

Comment 10

[Matthieu Riou]

Attachment: Same as previous but forces empty NS instead of no NS

Comment 11

[Norris Boyd]

I'll run them locally and let you know if there are any regressions.

Comment 12

[Norris Boyd]

That passed. I've committed the change:

Checking in xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java;
/cvsroot/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XmlNode.java,v  <--  XmlNode.java
new revision: 1.11; previous revision: 1.10
done

Checking in testsrc/doctests/441417.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/441417.doctest,v  <--  441417.doctest
initial revision: 1.1
done

Comment 13

[Matthieu Riou]

Perfect, thanks!