524931 - Setting the value of an uninitialized child of an E4X object by using square bracket syntax causes Null Pointer Exception

Status: RESOLVED FIXED

(Rhino Graveyard :: E4X, defect)

Description

[junk]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b1) Gecko/20091014 Firefox/3.6b1
Build Identifier: Rhino 1.7 release 2 2009 03 22

Attempting to set the value of an uninitialized child of an E4X object by using `top.child[0] = "value"` causes Rhino to crash with a Null Pointer Exception.

However, if top.child[0] already exists, the value is changed successfully.

Reproducible: Always

Steps to Reproduce:
var x = <x></x>;
x.foo[0] = "bar";
Actual Results:  
js> var x = <x><foo>bar</foo></x>;
js> x.foo[0];
bar
js> x.foo[0] = "baz";
baz
js> x.foo[1];
js>
js> x.foo[1] = "barf";
Exception in thread "main" java.lang.NullPointerException
	at org.mozilla.javascript.xmlimpl.XMLList.put(XMLList.java:242)
	at org.mozilla.javascript.xmlimpl.XMLObjectImpl.ecmaPut(XMLObjectImpl.java:312)
	at org.mozilla.javascript.ScriptRuntime.setObjectIndex(ScriptRuntime.java:1588)
	at org.mozilla.javascript.ScriptRuntime.setObjectIndex(ScriptRuntime.java:1576)
	at org.mozilla.javascript.Interpreter.interpretLoop(Interpreter.java:3081)
	at org.mozilla.javascript.Interpreter.interpret(Interpreter.java:2487)
	at org.mozilla.javascript.InterpretedFunction.call(InterpretedFunction.java:164)
	at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:398)
	at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3065)
	at org.mozilla.javascript.InterpretedFunction.exec(InterpretedFunction.java:175)
	at org.mozilla.javascript.tools.shell.Main.evaluateScript(Main.java:564)
	at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:424)
	at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:196)
	at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:117)
	at org.mozilla.javascript.Context.call(Context.java:515)
	at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:507)
	at org.mozilla.javascript.tools.shell.Main.exec(Main.java:179)
	at org.mozilla.javascript.tools.shell.Main.main(Main.java:157)

Expected Results:  
<x>
 <foo>baz</foo>
 <foo>barf</foo>
</x>

Comment 1

[Alberto Saez Torres]

This happens in other parts of the code.

This errors could be solved by catching the exceptions in 

org.mozilla.javascript.xmlimpl.XMLObjectImpl.ecmaPut:

  /**
     * Implementation of ECMAScript [[Put]]
     */
    @Override
    public final void ecmaPut(Context cx, Object id, Object value) {
        try {
            if (cx == null) cx = Context.getCurrentContext();
            XMLName xmlName = lib.toXMLNameOrIndex(cx, id);
            if (xmlName == null) {
                long index = ScriptRuntime.lastUint32Result(cx);
                // XXX Fix this cast
                put((int)index, this, value);
                return;
            }
            putXMLProperty(xmlName, value);
        } catch (Exception e) {
        	e.printStackTrace();
            throw ScriptRuntime.constructError("TypeError", "Can't assign " + value + " to " + id + ": " + e.getMessage());
        }
    }

Comment 2

[Norris Boyd]

Fixed: 

Checking in xmlimplsrc/org/mozilla/javascript/xmlimpl/XMLList.java;
/cvsroot/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XMLList.java,v  <--  XMLList.java
new revision: 1.32; previous revision: 1.31
done

Turns out this case isn't treated as an error by SpiderMonkey. I've emulated the behavior where the script

var x = <x><foo>bar</foo></x>;
x.foo[0] = "baz";
x.foo[1] = "barf";
print(x);

produces the output

<x>
  <foo>baz</foo>
  <foo>barf</foo>
</x>

Comment 3

[Alberto Saez Torres]

This change solves some issues, but not all related to this issue.

Another test case:

1) CASE 1

This code:

var tmp=new XML('<message xmlns="http://something"><data></data></message>');
default xml namespace = "http://something";
var otherData = new XML(<otherData></otherData>); 
otherData['@anAtt'] = "anATTValue"; 
tmp.data.otherData=otherData;

works fine in SpiderMonkey, but launch a DOMException (NAMEPSACE_ERR) in Rhino.

Curiously, changing the "otherData['@anAtt'] = "anATTValue"; " line to 

otherData.@anAtt = "anATTValue"; 

it works fine.


2) CASE 2

The next code:

default xml namespace = "http://something";
var tmp = new XML(<data></data>); 
var otherData=new XML(<otherData></otherData>); 
otherData['@anAtt2'] = "anATTValue2";
tmp.appendChild(otherData);

Works fine in SpiderMonkey, but launch a non-catched DomException NAMESPACE_ERR.

Similar as the previous case, changing 

otherData['@anAtt2'] = "anATTValue2";

to 

otherData.@anAtt2 = "anATTValue2";

It works.


I don't know if it's the right solution, but all this cases works by appling the patch related in 535964, what is changing create method at  :
XmlNode.Namespace class 

This is my suggested code:

static Namespace create(String uri) {
            Namespace rv = new Namespace();
            rv.uri = uri;
            //Avoid null prefix for "" namespaces
            if (uri=="") rv.prefix="";
            return rv;
        }

Comment 4

[Norris Boyd]

Thanks, I've committed your patch:

Checking in testsrc/jstests/524931.jstest;
/cvsroot/mozilla/js/rhino/testsrc/jstests/524931.jstest,v  <--  524931.jstest
initial revision: 1.1
done
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.13; previous revision: 1.12
done

Comment 5

[Alberto Saez Torres]

Unfortunately, We've found another problems.

CASE 3:

The next code works in spiderMonkey, but not in Rhino:

var xmlTester=<message><data></data></message>;
xmlTester['data']['test'][0] = "test0";
xmlTester['data']['test'][1] = "test1";
alert(xmlTester);


CASE 4:

The next code works in spiderMonkey, but not in Rhino:
var xmlTester=<message><data></data></message>;
xmlTester['data']['test'][0] = <subTest>subtest1</subTest>;
xmlTester['data']['test'][1] = "test1";
alert(xmlTester);


I've tested the next solution:

In XMLList, at  put(int index, Scriptable start, Object value) method, it's necessary to change:

if (xmlValue == null) {
    xmlValue=item(0).copy(); 
}

To:

if (xmlValue == null) {
    xmlValue=(item(0)==null)? newTextElementXML(null,targetProperty,null):item(0).copy(); 
}

And

if (index < length()) {
   parent = item(index).parent();
}else {
   // Appending
   parent = parent();
}

To:


if (index < length()) {
   parent = item(index).parent();
}else if (length()==0){
    parent= targetObject.getXML();
}else {
   // Appending
   parent = parent();
}


With this changes applied, it seems to work in a similar way to SpiderMonkey

Comment 6

[Norris Boyd]

Attachment: Proposed fix from earlier comments

I tried this patch and it worked for the test case but failed some regression tests. I'll need to investigate.

Comment 7

[jacobb]

Any update to this issue?  The patch Alberto supplied seemed to work for me, but what regression tests did it fail for?  Looking for a good solution to this bug in the latest versions.

Comment 8

[Alberto Saez Torres]

I've run all the Mozilla Suite tests, and I've found the problem.

Instead of

  parent= targetObject.getXML();

..It should be:

  parent= (targetObject!=null)? targetObject.getXML():parent();

The test case is:

var e = <employees><employee id="0"><name>John</name><age>20</age></employee><employee id="1"><name>Sue</name><age>30</age></employee></employees>;

var i = 0;
var twoEmployees = new XMLList();
for each (var p in e..employee)
{
    if (p.@id == 0 || p.@id == 1)
    {
        twoEmployees[i++] = p;
    }
}

Comment 9

[Norris Boyd]

Great, thanks. I hadn't yet had time to track down the problem. Thanks for doing so. 

I'll run regression tests tonight and submit if everything looks good.

Comment 10

[jacobb]

Will there be another 1.7R3pre release including this fix?  I'm currently using 1.7R3pre and just patched XMLList with this fix.  1.7R3pre was snagged from ftp://ftp.mozilla.org/pub/mozilla.org/js/

Comment 11

[Norris Boyd]

Committed patch with update from comment 8:

Checking in testsrc/doctests/524931.doctest;
/cvsroot/mozilla/js/rhino/testsrc/doctests/524931.doctest,v  <--  524931.doctest
new revision: 1.2; previous revision: 1.1
done
Checking in xmlimplsrc/org/mozilla/javascript/xmlimpl/XMLList.java;
/cvsroot/mozilla/js/rhino/xmlimplsrc/org/mozilla/javascript/xmlimpl/XMLList.java,v  <--  XMLList.java
new revision: 1.33; previous revision: 1.32

Thanks for the contribution!

Comment 12

[Norris Boyd]

(In reply to comment #10)
> Will there be another 1.7R3pre release including this fix?  I'm currently using
> 1.7R3pre and just patched XMLList with this fix.  1.7R3pre was snagged from
> ftp://ftp.mozilla.org/pub/mozilla.org/js/

I just pushed another rhino1_7R3pre.zip to staging that includes this fix. It should appear shortly.

Comment 13

[Hannes Wallnoefer]

Fabulous - this must have been the most reported E4X bug in Rhino - see bug 380173 and all its duplicates. A quick run through all bugs' test cases suggests they're all running as excpected now. Thanks!

Comment 16

[Alberto Saez Torres]

Humb 

I'm afraid but The 
"parent= (targetObject!=null)? targetObject.getXML():parent();" 

solution is at the CVS, but not in 7R3pre release (at least not in the src of the release)

Comment 17

[Alberto Saez Torres]

Sorry for the previous post. I've confirmed line is in the 7R3pre release.