Last Comment Bug 467396 - Rhino always prefers non-varargs java method over varargs one
: Rhino always prefers non-varargs java method over varargs one
Status: RESOLVED FIXED
:
Product: Rhino
Classification: Components
Component: Core (show other bugs)
: other
: x86 Linux
: -- major (vote)
: ---
Assigned To: Hannes Wallnoefer
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-12-01 12:11 PST by Clovis
Modified: 2009-06-09 10:45 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch and test case (3.99 KB, patch)
2008-12-05 04:03 PST, Hannes Wallnoefer
no flags Details | Diff | Splinter Review

Description Clovis 2008-12-01 12:11:35 PST
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 2008-12-01 12:39:39 PST
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 2008-12-05 03:01:15 PST
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 2008-12-05 04:03:15 PST
Created attachment 351536 [details] [diff] [review]
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 2008-12-05 04:19:16 PST
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 2009-06-09 10:41:12 PDT
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 2009-06-09 10:45:16 PDT
I committed my fix to bug 496585. That fix includes a new regression test for this bug: testsrc/doctests/467396.doctest.

Note You need to log in before you can comment on or make changes to this bug.