413838 - Incorrect ScriptRuntime.eq(Object, Object) implementation

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Vyacheslav Soldatov]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; ru; rv:1.8.1.10) Gecko/20071128 Fedora/2.0.0.10-2.fc7 Firefox/2.0.0.10
Build Identifier: 1_6R7

In case of arguments (Wrapper, Wrapper) this method invokes following code:
                if (x instanceof Wrapper && y instanceof Wrapper) {
                    return ((Wrapper)x).unwrap() == ((Wrapper)y).unwrap();
                }

but should invoke:
                if (x instanceof Wrapper && y instanceof Wrapper) {
                    return eq(((Wrapper)x).unwrap(), ((Wrapper)y).unwrap());
                }
This bug may appearances, by example, if we have in current scope two equals wrapped string objects. It may be if the object is returned from another object. In this case the operator a == b returns false instead of true, but the operation a.equals(b) returns true. It not appears if we will use code
("" + a) == ("" + b) which casts the objects to real strings.

Reproducible: Always

Steps to Reproduce:
1. Create two equals wrapped strings (x = "aaa", y = "aaa") and put them into top level scriptable as indexed properties.
2. execute script "x == y"
Actual Results:  
false

Expected Results:  
true

The unit test by example of reproduce:
/**
 * TestEq.java (24.01.2008)
 */

package com.uupdate.script.js;

import java.io.IOException;
import java.io.StringReader;
import java.util.HashMap;
import java.util.Map;

import org.mozilla.javascript.Context;
import org.mozilla.javascript.NativeJavaObject;
import org.mozilla.javascript.Script;
import org.mozilla.javascript.Scriptable;
import org.mozilla.javascript.tools.shell.Global;

import junit.framework.TestCase;

/**
 * @author Vyacheslav Soldatov
 *
 */
public class TestEq extends TestCase {
    /**
     * The scripting context.
     */
    private Context context;

    /**
     * The constructor.
     */
    public TestEq() {
        super();
    }

    /**
     * @param name the test case name.
     */
    public TestEq(final String name) {
        super(name);
    }

    /**
     * @see junit.framework.TestCase#setUp()
     */
    @Override
    protected void setUp() throws Exception {
        context = Context.enter();
    }

    /**
     * Tests '==' operator.
     * @throws IOException comilation exception.
     */
    public void testEq() throws IOException {
        Map<String, String> map = new HashMap<String, String>();
        map.put("x", new String("aaa"));
        map.put("y", new String("aaa"));

        Scriptable scriptable =  new NativeJavaObject(new Global(), map, Map.class);
        Object obj = compileScript("get('x') == get('y');").exec(context, scriptable);
        assertEquals(Boolean.TRUE, obj);
    }

    /**
     * Tests 'equals' work.
     * @throws IOException comilation exception.
     */
    public void testEquals() throws IOException {
        Map<String, String> map = new HashMap<String, String>();
        map.put("x", new String("aaa"));
        map.put("y", new String("aaa"));

        Scriptable scriptable =  new NativeJavaObject(new Global(), map, Map.class);
        Object obj = compileScript("get('x').equals(get('y'));").exec(context, scriptable);
        assertEquals(Boolean.TRUE, obj);
    }

    /**
     * @param script the script body.
     * @return compiled script. 
     * @throws IOException 
     */
    private Script compileScript(final String script) throws IOException {
        return context.compileReader(new StringReader(script), "Unit test body", 0, null);
    }
    /**
     * @see junit.framework.TestCase#tearDown()
     */
    @Override
    protected void tearDown() throws Exception {
        Context.exit();
    }
}

Comment 1

[Norris Boyd]

The proposed change won't work:

js> var a = new java.util.HashMap();
js> var b = new java.util.HashMap();
js> a == b
RHINO USAGE WARNING: Missed Context.javaToJS() conversion:
Rhino runtime detected object {} of class java.util.HashMap where it expected String, Number, Boolean or Scriptable instance. Please check your code for missing Context.javaToJS() call.
false

The problem is that a Wrapper doesn't necessarily wrap a value that can be handled by the Rhino runtime (which is why we have Wrappers in the first place).  So passing the unwrapped values to eq() can cause the problem above. 

What if we just make this change instead:
                if (x instanceof Wrapper && y instanceof Wrapper) {
                    return ((Wrapper)x).unwrap().equals(((Wrapper)y).unwrap());
                }
?

Comment 2

[Vyacheslav Soldatov]

IMHO, eq(obj1, obj2) should be invoked only for primitive types and string, but should be invoked eq(obj1, obj2) certainly.
Because the method eq(obj1, obj2) correctly and accurately handles any situations when arguments have different types, by example:
new java.lang.String("1") == new java.lang.Integer(1); //should be true
and any other.

By example, following fix can be used:
.......
                if (x instanceof Wrapper && y instanceof Wrapper) {
                	Object obj1 = ((Wrapper)x).unwrap();
                	Object obj2 = ((Wrapper)y).unwrap();

                	if (isPrimitiveOrString(obj1) && isPrimitiveOrString(obj2)) {
                		return eq(obj1, obj2);
                	}

                    return obj1 == obj2;
                }
.......
when
    private static boolean isPrimitiveOrString(final Object obj) {
    	return
    		(obj instanceof Number)
    		|| (obj instanceof String)
    		|| (obj instanceof Boolean);
    }

Comment 3

[Norris Boyd]

Looking at this more, I checked out the relevant ECMA section:

11.9.3 The Abstract Equality Comparison Algorithm
The comparison x == y, where x and y are values, produces true or false. Such a comparison is
performed as follows:
1. If Type(x) is different from Type(y), go to step 14.
2. If Type(x) is Undefined, return true.
3. If Type(x) is Null, return true.
4. If Type(x) is not Number, go to step 11.
5. If x is NaN, return false.
6. If y is NaN, return false.
7. If x is the same number value as y, return true.
8. If x is +0 and y is −0, return true.
9. If x is −0 and y is +0, return true.
10. Return false.
11.If Type(x) is String, then return true if x and y are exactly the same sequence of characters (same
length and same characters in corresponding positions). Otherwise, return false.
12. If Type(x) is Boolean, return true if x and y are both true or both false. Otherwise, return false.
13.Return true if x and y refer to the same object or if they refer to objects joined to each other (see
13.1.2). Otherwise, return false.
14. If x is null and y is undefined, return true.
15. If x is undefined and y is null, return true.
- 56 -
16.If Type(x) is Number and Type(y) is String,
return the result of the comparison x == ToNumber(y).
17.If Type(x) is String and Type(y) is Number,
return the result of the comparison ToNumber(x) == y.
18. If Type(x) is Boolean, return the result of the comparison ToNumber(x) == y.
19. If Type(y) is Boolean, return the result of the comparison x == ToNumber(y).
20.If Type(x) is either String or Number and Type(y) is Object,
return the result of the comparison x == ToPrimitive(y).
21.If Type(x) is Object and Type(y) is either String or Number,
return the result of the comparison ToPrimitive(x) == y.
22. Return false.

Since "joined" objects in step 13 refers only to function objects, we don't have the normal "host object" escape clause I would have expected. That means we can't implement something more than object identity in the case of two objects. I think the current implementation is correct as the same Java object could have two different wrappers.

Comment 4

[Vyacheslav Soldatov]

Yes, formally it is right. The java.lang.String is the java object and should be handled as punkt 13.
But see please example:
js> var a = "";
js> var b = "";
js> a == b; //good result
true
js> var map1 = new java.util.HashMap();
js> var map2 = new java.util.HashMap();
js> map1.put("key", a);
null
js> map2.put("key", b);
null
js> map1.get("key") == map2.get("key"); //not good result because this is equals with a==b
false
js> 

In last string the result of map1.get("key") is the variable a, and result of map2.get("key") is the variable b. If we directly compare them then result will 'true' and if we compare them as results of operation get from corresponding map, the result will 'false'. This is result of conversation rhino objects to java objects and back.
This is a cause. Two ways there are for resolving of this situation.
1. Changing of implementation of work with java objects and methods. All returned values if they are primitive should be not wrapped.
2. Wrapped java primitives should have a bihaviour like as corresponding native rhino primitive.

IMHO second approach is more simple and natural.

Comment 5

[Norris Boyd]

Thanks, that's a much more motivating example. The machinery of wrapping and unwrapping JavaScript primitives interferes with the normal comparison semantics.

I've submitted your proposed change with minor changes.