49350 - JavaAdapter changes prototype of delegee

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect, P3)

Description

[Matthias Radestock]

For some bizarre reason there is some code in JavaAdapter that changes the 
prototype of the delegee object, e.g.
 new JavaAdapter(Packages.Foo, foo)
will actually change the prototype of foo to be the newly created object.
This is bad because
a) it is not very intuitive
b) one cannot use Javascript inheritance (via prototypes) to implement methods 
of Java interfaces

Comment 1

[Matthias Radestock]

changing ScriptRuntime::setAdapterProto as follows fixes things: 

    public static void setAdapterProto(Scriptable obj, Object adapter) {
        Scriptable scope = ScriptableObject.getTopLevelScope(obj);
        Scriptable wrapped = (Scriptable) Context.toObject(adapter, scope);
        wrapped.setPrototype(obj);
    }

However, I'm not sure what the rationale was behind the original design. It is 
therefore possible that the above completely misses the point :)

Comment 2

[Norris Boyd]

This code is in to support the use of JavaAdapters as in NervousText.js. See 
examples/NervousText.js in the distribution. I verified that the proposed change 
breaks this example:

[rhino] java sun.applet.AppletViewer NervousText.html
ReferenceError: "resize" is not defined.
        at org.mozilla.javascript.NativeGlobal.constructError(NativeGlobal.java:
496)
        at org.mozilla.javascript.ScriptRuntime.getBase(ScriptRuntime.java, Comp
iled Code)
        at NervousText$init.call(Unknown Source)
        at org.mozilla.javascript.ScriptRuntime.call(ScriptRuntime.java:1217)
        at org.mozilla.javascript.JavaAdapter.callMethod(JavaAdapter.java:301)
        at NervousText.init(<adapter>)
        at sun.applet.AppletPanel.run(AppletPanel.java:333)
        at java.lang.Thread.run(Thread.java:479)

In the init function, resize is called. In the Java implementation, the resize 
method lives in the base class, so we make the prototype of the JavaAdapter be 
the Java reflection of the class so that base methods are "inherited". 

It'd be great if we could figure out a way to make both work. Maybe the 
prototype should be changed only if it is set to Object.prototype (the default)?  
It's a bit of a weird case because of the collision of prototypes and 
class-baseed inheritance.

Comment 3

[Matthias Radestock]

Here is what I think should be done.

1) Instead of setting the proto of the delegee to the wrapper, the proto of the 
wrapper is set to the delegee. This is *a lot* cleaner since it leaves the 
delegee intact. It also means that any properties added later to the delegee (or 
one of the objects in the prototype chain) will be accessible from the wrapper 
(on the Javascript side only though).

2) When creating the wrapper class we not only generate methods for properties 
on the delegee but also traverse the delegee's prototype chain. As a result, 
Java code can invoke methods on the Javascript object that are defined anywhere 
within the Javascript prototype chain.

3) Instead of calling methods on the wrapper with "this" bound to the delegee, 
we call them with "this" bound to the wrapper. This means that the "this" can be 
used to call methods on Java base classes. This *will* break existing code which 
 calls the methods unqualified, but doing so was never the right thing to do in 
the first place, IMHO.

I have implemented 1)+3) and it works great. What do you think ?

Comment 4

[Matthias Radestock]

Here's a nice little example of inheritance in Java and Javascript working in 
perfect harmony. This all works fine with the changes I proposed.

A.java:
public class A extends B {
    public String m1() { return "m1"+m2(); }
}

B.java:
public class B {
    public String m2() { return "m2"; }
    public String m4() { return "m4"; }
}

Javascript:
function P() {}
P.prototype.m2 = function() { return "j2"+this.m3(); }
function J() {}
J.prototype = new P;
J.prototype.m3 = function() { return "j3"+this.m4(); }
var j = new J;
var jj = new JavaAdapter(Packages.A,j);
jj.m1(); //=> "m1j2j3m4"

Comment 5

[Norris Boyd]

Do you have a patch that implements the changes you suggest?

Thanks for looking at this.

Comment 6

[Matthias Radestock]

Attachment: patch (part 1/3)

Comment 7

[Matthias Radestock]

Attachment: patch (part 2/3)

Comment 8

[Matthias Radestock]

Attachment: patch (part 3/3)

Comment 9

[Norris Boyd]

I'm looking at this. Thanks for the patch files. Perhaps I'm doing something 
wrong, but I have trouble with the format of your patch files. I generally use a 
unified diff for patch files.

Comment 10

[Norris Boyd]

In the JavaAdapter patch you have

+    public static Scriptable setAdapterProto(Scriptable obj, Object adapter) {
+        Scriptable res = ScriptRuntime.toObject(ScriptableObject.getTopLevelSco
pe(obj),adapter);
+        res.setPrototype(obj);
+        return res;
+    }

The parameters for Context.toObject are (value, scope). Did you mean 
    ScriptRuntime.toObject(adapter,ScriptableObject.getTopLevelScope(obj));
instead?

Comment 11

[Matthias Radestock]

The parameters for Context.toObject are "value,scope". However, I'm using
*ScriptRuntime* and there the parameters are "scope,value" :)

Comment 12

[Norris Boyd]

Finally got around to merging this patch. The only change I made was to keep
Rhino working on JDK 1.1.x by removing the use of HashSet. It's unfortunate 
because that looked like the best solution.

Checking in examples/NervousText.js;
/cvsroot/mozilla/js/rhino/examples/NervousText.js,v  <--  NervousText.js
new revision: 1.4; previous revision: 1.3
done
Checking in org/mozilla/javascript/JavaAdapter.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/JavaAdapter.java,v  <--  JavaAd
apter.java
new revision: 1.29; previous revision: 1.28
done
Checking in org/mozilla/javascript/ScriptRuntime.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/ScriptRuntime.java,v  <--  Scri
ptRuntime.java
new revision: 1.47; previous revision: 1.46
done
Checking in org/mozilla/javascript/ScriptableObject.java;
/cvsroot/mozilla/js/rhino/org/mozilla/javascript/ScriptableObject.java,v  <--  S
criptableObject.java
new revision: 1.20; previous revision: 1.19
done

Comment 13

[Matthias Radestock]

Norris,

in your attempt to eliminate HashSets you introduced an infinite loop in 
ScriptableObject::getPropertyIds :

       while (obj != null) {
            Object[] ids = obj.getIds();
            for (int i=0; i < ids.length; i++) {
                h.put(ids[i], ids[i]);
            }
        }

You've missed out
       obj = (Scriptable)obj.getPrototype();
from the loop.

Comment 14

[Norris Boyd]

I did check in the fix to this, but didn't mark this fixed.