467396 - Rhino always prefers non-varargs java method over varargs one

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, defect)

Description

[Clovis]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9) Gecko/2008052912 Firefox/3.0
Build Identifier: Rhino 1.7R1

If i try to run follow code:

var layRegE00 =  java.lang.reflect.Array.newInstance(java.lang.Object, [17,4]);

i get the exception:

Cannot convert 17,4 to java.lang.Integer.

this code works in version 1.6R1 of Rhino.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Hannes Wallnoefer]

I guess this might have to do with the patch from bug 382340. I just tried and it is broken with 1.6R6, which is the first release containing that patch. FWIW, invoking that method varargs style works:

java.lang.reflect.Array.newInstance(java.lang.Object, 17, 4);

Comment 2

[Hannes Wallnoefer]

I found the problem. The java.lang.reflect.Array.newInstance() method taking an int array as second argument has changed to varargs signature in Java 6, and NativeJavaMethod.preferSignature() currently always prefers non-varargs methods over varargs methods. Thus, the method with int as second argument is called on Java 6, while on Java 5 the right method in chosen.

I guess we shouldn't blindly prefer non-varargs methods over varargs ones. Removing that code results in the varargs method being called with an array object, and the non-varargs one with a number argument. 

Apart from that, I wonder if it is right that NativeJavaObject.canConvert() returns true for Scriptable -> java.lang.Number conversion. Is there a scenario where this conversion actually works - and makes sense?

Comment 3

[Hannes Wallnoefer]

Attachment: patch and test case

This patch removes the code that always prefers non-varargs methods over varargs ones. From the original comment, my feeling is that the author added that code without a stringent cause such as actual code requiring it, and the testcase shows the new code works in this case. The comment is updated to point to this bug.

Comment 4

[Hannes Wallnoefer]

Committed to HEAD:

Checking in src/org/mozilla/javascript/NativeJavaMethod.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeJavaMethod.java,v  <--  NativeJavaMethod.java
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Attic/Bug467396Test.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/Bug467396Test.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Attic/Bug467396Test.java,v  <--  Bug467396Test.java
new revision: 1.1.2.1; previous revision: 1.1
done

Probably should go also to Rhino1_7R2_BRANCH, although there's a slight chance something else might break.

Comment 5

[Norris Boyd]

This change caused a regression (see bug 496585). I'm working on a fix that will preserve this case as well.

Comment 6

[Norris Boyd]

I committed my fix to bug 496585. That fix includes a new regression test for this bug: testsrc/doctests/467396.doctest.