531374 - JavaAdapter broken for similar derivations (JavaAdapter.JavaAdapterSignature.equals is broken)

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[John Spackman]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3
Build Identifier: rhino1_7R2

Summary:	JavaAdapter broken for similar derivations (JavaAdapter.JavaAdapterSignature.equals is broken)

JavaAdapter creates classes on the fly and caches the results; it uses JavaAdapter.JavaAdapterSignature to uniquely
identify the cached class (as a key for a Map) BUT JavaAdapter.JavaAdapterSignature.equals() is broken.

The JavaAdapter.JavaAdapterSignature.equals checks "this" against another signature - it checks the superclass and
inheritance are the same, but checks the overridden method names of "this" against "this" - not the other instance.

The result is that if you use JavaAdapter to create two instances of a class and in both instances you override the 
same method but with different implementation, the second class is seen as identical to the first.

EG given this Java class:
	public class JsTest4Extra {
		public String testFirst() {
			return "first-not-implemented";
		}
		public String testSecond() {
			return "second-not-implemented";
		}
	}

...and then writing this code in JS:

	var myCompA = {
		testFirst: function() {
			return "alpha: first-IS-implemented";
		}
	};
	var myA = new JavaAdapter(com.zenesis.test.script.JsTest4Extra, myCompA);

	var myCompB = {
		testSecond: function() {
			return "bravo: second-IS-implemented";
		}
	};

	var myB = new JavaAdapter(com.zenesis.test.script.JsTest4Extra, myCompB);

in this example, myA and myB are supposed to be different instances overriding either testFirst() or testSecond() but
instead you get this:

		a.testFirst()=alpha: first-IS-implemented
		a.testSecond()=second-not-implemented
		b.testFirst()=undefined
		b.testSecond()=second-not-implemented


Reproducible: Always

Steps to Reproduce:
package com.zenesis.test.script;

import java.io.StringReader;

import org.mozilla.javascript.Context;
import org.mozilla.javascript.ContextFactory;
import org.mozilla.javascript.NativeJavaObject;
import org.mozilla.javascript.Scriptable;
import org.mozilla.javascript.Undefined;

/**
 * Demonstrates a bug in JavaAdapter.equals(); if you have a Java class and use JavaAdapter to override
 * a method in a derived instance, everything is fine.  If you seperately use JavaAdapter to override
 * a _different_ method of the same class (but in a new instance), Rhino will use the the class
 * it generated the first time around and _not_ generate a new class.  This is because JavaAdapter.equals()
 * is broken so Rhino thinks that it already has a suitable class.
 * 
 * When running an unmodified Rhino1_7R2 this code produces:
 * 
		a.testFirst()=alpha: first-IS-implemented
		a.testSecond()=second-not-implemented
		b.testFirst()=undefined
		b.testSecond()=second-not-implemented
 * 
 * after patching org/mozilla/javascript/JavaAdapter.java line 91, it shows:
 * 
		a.testFirst()=alpha: first-IS-implemented
		a.testSecond()=second-not-implemented
		b.testFirst()=first-not-implemented
		b.testSecond()=bravo: second-IS-implemented
 *
 * @author <a href="mailto:[e-mail]">John Spackman</a>
 */
public class JsTest5 {
	
	private static final String ALPHA = "" +
			"var myComp = {\r\n" + 
			"	testFirst: function() {\r\n" + 
			"		return \"alpha: first-IS-implemented\";\r\n" + 
			"	}\r\n" + 
			"};\r\n" + 
			"\r\n" + 
			"new JavaAdapter(com.zenesis.test.script.JsTest4Extra, myComp);";

	private static final String BRAVO = "" +
			"var myCompB = {\r\n" + 
			"	testSecond: function() {\r\n" + 
			"		return \"bravo: second-IS-implemented\";\r\n" + 
			"	}\r\n" + 
			"};\r\n" + 
			"\r\n" + 
			"new JavaAdapter(com.zenesis.test.script.JsTest4Extra, myCompB);";
	
	public JsTest5() throws Exception {
		super();
		Context context = ContextFactory.getGlobal().enterContext();
		Scriptable scope = context.initStandardObjects();
		
		JsTest4Extra a = (JsTest4Extra)run(scope, ALPHA);
		JsTest4Extra b = (JsTest4Extra)run(scope, BRAVO);
		
		System.out.println("a.testFirst()=" + a.testFirst());
		System.out.println("a.testSecond()=" + a.testSecond());
		System.out.println("b.testFirst()=" + b.testFirst());
		System.out.println("b.testSecond()=" + b.testSecond());
	}
	
	private Object run(Scriptable scope, String code) throws Exception {
		Context context = ContextFactory.getGlobal().enterContext();
		try {
		    Object result = context.evaluateReader(scope, new StringReader(code), "inline", 1, null);
			if (result instanceof NativeJavaObject) {
				NativeJavaObject njo = (NativeJavaObject)result;
				result = njo.unwrap();
			}
			if (result == Undefined.instance)
				result = null;
			return result;
		}finally {
			Context.exit();
		}
	}

	public static void main(String[] args) {
		try {
			new JsTest5();
		} catch(Exception e) {
			e.printStackTrace();
		}
	}
}

Actual Results:  
		a.testFirst()=alpha: first-IS-implemented
		a.testSecond()=second-not-implemented
		b.testFirst()=undefined
		b.testSecond()=second-not-implemented


Expected Results:  
		a.testFirst()=alpha: first-IS-implemented
		a.testSecond()=second-not-implemented
		b.testFirst()=first-not-implemented
		b.testSecond()=bravo: second-IS-implemented


The fix for this is on line 91 of org/mozilla/javascript/JavaAdapter.java; change this:
   ObjToIntMap.Iterator iter = new ObjToIntMap.Iterator(names);
to this:
   ObjToIntMap.Iterator iter = new ObjToIntMap.Iterator(sig.names);

Comment 1

[Hannes Wallnoefer]

This was fixed in CVS some time ago. I'm sorry for finding this bug only now and thanks for reporting!

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/js/rhino/src/org/mozilla/javascript&command=DIFF_FRAMESET&root=/cvsroot&file=JavaAdapter.java&rev1=1.119&rev2=1.120