271401 - JS prototypes for superclasses with ScriptableObject.defineClass

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Daniel Gredler]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

Classes that extend ScriptableObject cannot use polymorphism to define
JavaScript-accessible properties and functions, because
FunctionObject.getMethodList(Class) uses Class.getDeclaredMethods() instead of
Class.getMethods() whenever it can, apparently as an optimization. The reason
that getDeclaredMethods() is faster is that they are fundamentally different
methods: getDeclaredMethods() excludes inherited methods, but getMethods()
includes them. So for example if I have:

public class A extends ScriptableObject {
  public String jsGet_name() {
    return "A";
  }
}

public class B extends A {
}

then as far as the JavaScript is concerned, an instance of B does not have a
property "name". Unless, of course, calling getDeclaredMethods() fails with a
SecurityException, in which case FunctionObject will use getMethods(), and
instances of B *will* have a property "name".

Reproducible: Always
Steps to Reproduce:
1. Create a Java class A that extends ScriptableObject and has a JavaScript
attribute "att".
2. Create a Java class B that extends A.
3. In JavaScript, create an instance of B and try to access inherited attribute
"att".

Actual Results:  
The attribute "att" cannot be found comes back as undefined.

Expected Results:  
The attribute "att" should have been found via polymorphism, in class A.

Comment 1

[Igor Bukanov]

Marking as invalid:

ScritableObject use getDeclaredMethods by design to allow to implement host
objects that follow prototype-based inheritance of JavaScript runtime. To get
what you need to set the prototype chain of B.prototype to A.prototype in JS.
For example, use something like:


public class A extends ScriptableObject {
  
  public getClassName() { return "A"; }
  
  public String jsGet_name() {
    return "A";
  }
}

public class B extends A {
  public getClassName() { return "B"; }
}

....

ScriptableObject.defineClass(scope, A.class);
ScriptableObject.defineClass(scope, B.class);

ScriptableObject.getClassPrototype(scope, "B").
	setPrototype(ScriptableObject.getClassPrototype(scope, "A"));
	

Now in JS the following would work:

var b = new B();
print(b.name); // should call jsGet_name

print(b instanceof B); // should print true
print(b instanceof A); // should also print true

Note also that comments in FunctionObject refer to the fact that some JVMs with
SecurityManager installed getDeclaredMaethods() may throw an exception while
getMethods() works so as a workaround code calls getMethods() and strip
super-class only methods.

Comment 2

[Daniel Gredler]

My complaint is that:
1) getDeclaredMethods() is used if possible (it should never be used)
2) if getMethods() is used, super-class methods are stripped (they should always
be included)

I don't think super-class methods should ever be excluded, thus I don't think
getDeclaredMethods should ever be used. I don't see the sense in only allowing
access to declared methods, not inherited methods. That's the whole point of
Java: polymorphism.

My use case is HttpUnit, by the way. HTMLElement extends ScriptableObject, and
Document extends HTMLElement. I don't understand why I need to override a
property in Document that is already defined in HTMLElement. It's nonsensical in
an OO world.

Comment 3

[Igor Bukanov]

> 
> My use case is HttpUnit, by the way. HTMLElement extends ScriptableObject, and
> Document extends HTMLElement. I don't understand why I need to override a
> property in Document that is already defined in HTMLElement. It's nonsensical in
> an OO world.

ScriptableObject machinery is intended to support JavaScript prototype-based
inheritance which is very different from class-based inheritance og Java. 

I also think that using ScriptableObject as a base for 
DOM objects is not a very idea. It is much better IMO to create something
similar to NativeJavaObject+dependable classes to create a special wrapper for
Java DOM. In at least one Java-based browser it allowed to emulate all MSIE-5.5
specific peculiarities and shortcuts for DOM.

Comment 4

[Daniel Gredler]

I think I understand what you're saying, but let me ask a couple of questions
just to make sure:

> ScriptableObject machinery is intended to support JavaScript
> prototype-based inheritance which is very different from class-based
> inheritance of Java.

Given that the two inheritance mechanisms are fundamentally different, would it
be ludicrous to maybe map from one to the other automatically when
ScriptableObject.defineClass() gets called? So the setting of the prototypes
would occur automatically based on the inheritance tree of the Java class
specified. Maybe even do this optionally be adding a boolean parameter to
defineClass(). It may be a stupid question, given that you think this is the
wrong way to solve our problem, regardless.

> I also think that using ScriptableObject as a base for DOM objects is
> not a very [good] idea.

Why is this not the best way to do it? What problem was ScriptableObject meant
to solve if not this one?

> It is much better IMO to create something similar to
> NativeJavaObject + dependable classes to create a special wrapper
> for Java DOM.

By "dependable classes" do you mean NativeJavaArray, NativeJavaClass,
NativeJavaMethod, etc?

> In at least one Java-based browser it allowed to emulate all
> MSIE-5.5 specific peculiarities and shortcuts for DOM.

Well, that's what we're trying to do, ultimately :) What browser was this? Is
the code open?

Thanks for clarifying all this!

Comment 5

[Igor Bukanov]

(In reply to comment #4)
> 
> Given that the two inheritance mechanisms are fundamentally different, would it
> be ludicrous to maybe map from one to the other automatically when
> ScriptableObject.defineClass() gets called? So the setting of the prototypes
> would occur automatically based on the inheritance tree of the Java class
> specified. Maybe even do this optionally be adding a boolean parameter to
> defineClass(). 

Originally ScriptableObject was heavily used to implement the standard
ECMAScript objects like Array, Data, Math etc where automatic Java inheritance
would bring more harm then explicit prototype chain manipulation due to numerous
special cases. Later it was replaced by explicit string switches to gain
significantly in performance and reduce memory consumption. 

Now Rhino does not use ScriptableObject support for jsGet_ etc internally and it
is intended as a simple way for applications to create host objects that
resemble the standard objects. Thus automated support for inheritance chain
would indeed lead to less surprises in application code and would not affect
Rhino internals but then somebody has to write the code.
> 
> > I also think that using ScriptableObject as a base for DOM objects is
> > not a very [good] idea.
> 
> Why is this not the best way to do it? What problem was ScriptableObject meant
> to solve if not this one?

You already have implementation of DOM interfaces in Java, right? In this case
you would end up with js_ wrapper methods that would simply call their
counterpart in Java DOM world.

But even if you create JS-only implementation, then JavaScript bindings would
often require (as far as I remember) direct access to JS arguments array forcing
to use generic jsFunction_xxx(Context cx, Scriptable thisObj, Object[] args,
Function funObj) form with manual type conversion. Plus the fact that Rhino does
not pass scope argument can cause problems when access to that argument is
required to get the right frame reference.

And memory consumption/initialization time is much higher since you would need
to store all those constructor/prototype objects in the scope which even with
lazy initialization is costly.

> 
> > It is much better IMO to create something similar to
> > NativeJavaObject + dependable classes to create a special wrapper
> > for Java DOM.
> 
> By "dependable classes" do you mean NativeJavaArray, NativeJavaClass,
> NativeJavaMethod, etc?

It is not that bad, you need just a copy of NativeJavaObject, JavaMemebers and
NativeJavaMethod. Then you can strip from the copies any support for static
functions and public fields simplifying code significantly. Then if you have
Java implementation of HTMLSomething interfaces it would be simple to implement,
for example, collection mapping from a[i] into a.item(i) or provide various
aliases for methods.

> 
> > In at least one Java-based browser it allowed to emulate all
> > MSIE-5.5 specific peculiarities and shortcuts for DOM.
> 
> Well, that's what we're trying to do, ultimately :) What browser was this? 

ICEbrowser, www.icesoft.com (Declaimer: I worked for them in past)

>Is the code open?
> 

Nope.

Comment 6

[Daniel Gredler]

> It is not that bad, you need just a copy of NativeJavaObject,
> JavaMemebers and NativeJavaMethod. Then you can strip from the
> copies any support for static functions and public fields
> simplifying code significantly. Then if you have Java
> implementation of HTMLSomething interfaces it would be simple to
> implement, for example, collection mapping from a[i] into a.item(i)
> or provide various aliases for methods.

Do you have any examples of doing this? All the examples on the website use
either the Scriptable interface, or the default implementation,
ScriptableObject. I can't find any examples of doing it the way you mention.

Also, has it been absolutely ruled out to provide a DOM implementation in Rhino
itself, since most JavaScript is used to manipulate HTML documents? It just
seems that many projects that use Rhino have to roll their own DOM host objects,
and it would be much easier to pool resources and effort and have just one
implementation.

Comment 7

[Igor Bukanov]

Sorry, I can not provide examples. And you are right that this not documented.
And certanly DOM implementation in Rhino would be extremely welcome. But it
would remain like that until somebody writes it. 

Unfortunately I can not help with this much since besides the lack of time a
somewhat traumatic experience of trying to support all MSIE 5 quirks few years
ago lead to inevitable desire to avoid writing anything related to Java
implementation of MSIE DOM ;)

Comment 8

[Daniel Gredler]

Attachment: map Java inheritance to JS prototype-based inheritance

> Now Rhino does not use ScriptableObject support for jsGet_ etc
> internally and it is intended as a simple way for applications
> to create host objects that resemble the standard objects. Thus
> automated support for inheritance chain would indeed lead to
> less surprises in application code and would not affect Rhino
> internals but then somebody has to write the code.

I'm attaching a patch that does just this. This is my first time tinkering with
Rhino internals, so please advise as to possible improvements.

Comment 9

[Igor Bukanov]

Comment on attachment 168021 [details] [diff] [review]
map Java inheritance to JS prototype-based inheritance

> 
>+        // Map Java inheritance to JS prototype-based inheritance.
>+        Class superclass = clazz.getSuperclass();
>+        if (superclass != null) {
>+            String name = superclass.getName();
>+            Scriptable superProto = ScriptableObject.getClassPrototype(scope, name);
>+            if (superProto != null) {
>+                proto.setPrototype(superProto);
>+            }
>+        }
>+

That does not work since the name of Java class and the name of constructor
object also known as JS "class" object (like String, Date, Array etc.) are
unrelated and you need the later here. But to get that you need an instance of
the superclass to be able to call getClassName on it. 

One possibility would be to have a version of defineClass that loops through
the parent chain defining the necessary classes if that is not already done.

Comment 10

[Daniel Gredler]

Attachment: map Java inheritance to JS prototype-based inheritance

I didn't realize the the class name was not the Java class name in that
context. Here's another patch, let me know what you think. It changes the
defineClass API so that it returns the class name of the prototype defined,
which wouldn't break existing code but might still be a no-no. Let me know.

Comment 11

[Igor Bukanov]

Reopening as enhancment and changing the title

Comment 12

[Igor Bukanov]

Comment on attachment 168034 [details] [diff] [review]
map Java inheritance to JS prototype-based inheritance

>+        Class superClass = clazz.getSuperclass();
>+        if (superClass != null && !superClass.isAssignableFrom(Object.class)) {

That should be superClass != ScriptableObject.class or since x.class leads to
significant class file bloat:
superClass != ScriptRuntime.ScriptableObjectClass
since the recursion should stop on ScriptableObject

Comment 13

[Igor Bukanov]

(In reply to comment #10)
> Created an attachment (id=168034)
> map Java inheritance to JS prototype-based inheritance
> 
> I didn't realize the the class name was not the Java class name in that
> context. Here's another patch, let me know what you think. It changes the
> defineClass API so that it returns the class name of the prototype defined,
> which wouldn't break existing code but might still be a no-no. 

It would break binary compatibility plus it changes the method semantic
significantly which is no go. I suggest to add one more signature of defineClass
with int flag field with explicit flag constants which would also take care
about sealed.

Regards, Igor

Comment 14

[Daniel Gredler]

Attachment: map Java inheritance to JS prototype-based inheritance

Hmm, I wasn't aware of the x.class overhead... This project seems to be pretty
performance-conscious, from this and other things I have seen in the code :) I
changed the statement in question to the following:

+	 if (superClass != null &&
+	     superClass != ScriptRuntime.ObjectClass &&
+	     superClass != ScriptRuntime.ScriptableObjectClass) {

I'm leaving the check for null and ObjectClass in because of the
Class.getSuperclass() contract (from Javadoc):

> Returns the <code>Class</code> representing the superclass of the
> entity (class, interface, primitive type or void) represented by
> this <code>Class</code>.  If this <code>Class</code> represents
> either the <code>Object</code> class, an interface, a primitive
> type, or void, then null is returned.  If this object represents
> an array class then the <code>Class</code> object representing the
> <code>Object</code> class is returned.

Comment 15

[Daniel Gredler]

> It would break binary compatibility plus it changes the method
> semantic significantly which is no go. I suggest to add one more
> signature of defineClass with int flag field with explicit flag
> constants which would also take care about sealed.

I tend to dislike numeric flag fields, since using simple booleans
is usually much clearer. Do you have anything against just adding a
defineClass(Scriptable,Class,boolean,boolean) method?

Comment 16

[Igor Bukanov]

(In reply to comment #15)
> > It would break binary compatibility plus it changes the method
> > semantic significantly which is no go. I suggest to add one more
> > signature of defineClass with int flag field with explicit flag
> > constants which would also take care about sealed.
> 
> I tend to dislike numeric flag fields, since using simple booleans
> is usually much clearer. 

Well, it is more clear at the definition side but at the caller side trying to
figure out what defineClass(..., false, true) for example would mean is harder.

> Do you have anything against just adding a
> defineClass(Scriptable,Class,boolean,boolean) method?

I do not have anything against that, the idea of using int was just a suggestion
in case a third boolean would be necessary. But then perhaps a whole new
defineClass should be defined.

Comment 17

[Igor Bukanov]

(In reply to comment #14)
> Created an attachment (id=168056)
> map Java inheritance to JS prototype-based inheritance
> 
> Hmm, I wasn't aware of the x.class overhead... 

Try to compare the size of compiled
class Foo
{
    Class bar() { return "".getClass(); }
}

and 

class Foo
{
    Class bar() { return String.class; }
}

Then multiply that by 50 or so cases where Rhino would need to access Class objects.

...
> I
> changed the statement in question to the following:
> 
> +	 if (superClass != null &&
> +	     superClass != ScriptRuntime.ObjectClass &&
> +	     superClass != ScriptRuntime.ScriptableObjectClass) {
> 
> I'm leaving the check for null and ObjectClass in because of the
> Class.getSuperclass() contract (from Javadoc):
> 
> > Returns the <code>Class</code> representing the superclass of the
> > entity (class, interface, primitive type or void) represented by
> > this <code>Class</code>.  If this <code>Class</code> represents
> > either the <code>Object</code> class, an interface, a primitive
> > type, or void, then null is returned.  If this object represents
> > an array class then the <code>Class</code> object representing the
> > <code>Object</code> class is returned.
> 

Note that the code before already created an instance of your class and casted
it to Scriptable so the check for null is not necessary since the class is not
Object. 

But you are right that lone superClass != ScriptRuntime.ScriptableObjectClass is
not enough since defineClass should work with any class as long as it implements
Scriptable. Which tells me that the check should be something like:

> +	 if (ScriptRuntime.ScriptableClass.isAssignableFrom(superClas)) {

Comment 18

[Daniel Gredler]

What is the next version of Rhino going to be? 1.6R2? For the @since javadoc tag...

Comment 19

[Igor Bukanov]

The next version would be 1.6R1.1 but that would include strictly bug fixes so
it is OK to use @since 1.6R2

Comment 20

[Daniel Gredler]

Attachment: map Java inheritance to JS prototype-based inheritance

Alright, here's a patch that maintains backward compatibility in the API by
adding a new method that allows inheritance mapping from Java-world to
JavaScript-world. It also makes use of the
ScriptRuntime.ScriptableClass.isAssignableFrom(superClass) check. Let me know
what you think.

Comment 21

[Igor Bukanov]

The patch is OK, I will commit it after regression testing.

Comment 22

[Igor Bukanov]

I committed the patch after replacing tabs by spaces. 

Comment 23

[Daniel Gredler]

> I committed the patch after replacing tabs by spaces.

Sounds good! I guess I can claim to have contributed to the Mozilla project now? ;)