Comment 1 Mark Wielaard 2008-10-15 15:56:50 UTC

So, as a workaround for now, when you want to use a non-system installed rhino, instead of using:
 -classpath /path/to/local/rhino.jar
use:
 -Xbootclasspath/p:/path/to/local/rhino.jar

I've thought about this a bit, and I think the only reasonable way to fix it (short of repackaging rhino with the jvm like Sun does) is to have the RhinoScriptEngine implementation bundled with the Rhino classes instead of the jre library. If rhino.jar is loaded though a custom classloader and the ScriptEngine sits on the jvm side, it has to go through reflection API for each interaction with Rhino, which is painful to say the least. If, OTOH, the ScriptEngine(Factory) is loaded by the same classloader as rhino.jar all that is needed are a few lines of class loading and casting.

Bundling a ScriptEngine implementation with rhino would have a number of other advantages, like allowing people to use a more cutting edge Rhino version than the one bundled with the jvm (maybe using alternative name/mimetype/extensions to avoid clashes with Sun's internal rhino engine). 

The easy way to get there would be to import the openjdk rhinoengine code into rhino, but I doubt the GPLed code is compatible with Rhino's MPL1.1/GPL2 dual license. As an alternative, implementing a ScriptEngine withing Rhino looks like a viable alternative. 

Of course, a related approach would be to split the Rhine engine implementation out of rt.jar into a separate jar file and load that with the same classloader as rhino.jar. Being a Rhino developer I just tend to think the other way around :-)

The change proposed in #3 isn't happening in Rhino after one year. And while it's a sensible change on its own from Rhino side, it's not a good fix for this bug, since it has a compatibility problem, in that applications that are using older Rhino.jar will break if IcedTea once removes its copy of rhino.jar (in favor of RhinoScriptEngine in rhino.jar in the user space.)

So I recommend the package renaming. While it's not a very smart solution, it results in the best overall user experience. I've been involved in XML technologies that are integrated into JDK, so I hope it helps add some credibility to what I'm saying.

As I explained in the bug http://bugs.sun.com/view_bug.do?bug_id=6876736 , this issue is biting a number of projects, including my Hudson, and unfortunately I have to recommend people to switch to Sun JDK.

A timely resolution of this issue is much appreciated.

Comment 5 Andrew John Hughes 2010-01-14 01:44:10 UTC

I'm afraid it's not as simple as that.  IcedTea doesn't package Rhino, it uses the system installed version.  So to have the packages renamed, you'd need the system installation of Rhino to be patched in that manner.

We don't want to be maintaining our own fork of Rhino within IcedTea.  I think we need to look at a solution whereby Rhino is loaded with a lower priority classloader that can be overridden.

An interim solution, other than the -Xbootclasspath setting suggested by Mark, would be to remove the symlink from the JDK install. 

Regarding my proposal in comment #2 of including the ScriptEngine implementation in Rhino: I've done some work on this in Rhino using the code from scripting.dev.java.net, but unfortunately ran into some erratic behaviour when selecting scripting engines (see my last comment on that bug)

https://bugzilla.mozilla.org/show_bug.cgi?id=379385#c3

(In reply to comment #5)
> We don't want to be maintaining our own fork of Rhino within IcedTea.  I think
> we need to look at a solution whereby Rhino is loaded with a lower priority
> classloader that can be overridden.
> 
> An interim solution, other than the -Xbootclasspath setting suggested by Mark,
> would be to remove the symlink from the JDK install. 

Yes, this would be the best solution - if it is feasible, which I'm not sure about. I would welcome any efforts in that direction from OpenJDK developers. I tried hacking the OpenJDK scripting engine code myself at one time, but was a bit overwhelmed by its build system (and build times).

I agree with Kohsuke that this is seriously hurting projects using their own version of Rhino.

Comment 7 Andrew John Hughes 2010-02-07 11:38:26 UTC

Instead of just symlinking the system Rhino JAR, we need to create a copy with the alternate namespace of com.sun.org.mozilla.   That means rewriting the class files so we need to look for an appropriate solution for doing this.

Comment 8 Mark Wielaard 2010-02-13 14:48:34 UTC

(In reply to comment #7)
> Instead of just symlinking the system Rhino JAR, we need to create a copy with
> the alternate namespace of com.sun.org.mozilla.   That means rewriting the
> class files so we need to look for an appropriate solution for doing this.

It would be better to load it through a separate class loader (not the extension class loader, which is used now because the symlink is in the ext lib path) so you don't need to make any copies and it never is seen on the source compiler/bootstrap path. Having the ScriptEngine added to rhino itself as suggested in comment #2 would make that easy.

The trouble with creating a copy and rejiggering all the package names is that you will have a separate copy that you will need to keep in sync with the distro supplied one some way. Double the effort on security and bug fixes.

Comment 9 Andrew John Hughes 2010-02-17 23:27:30 UTC

Mark, I agree it's not an ideal solution but I'd rather we had _a_ solution than none at all.  This bug has been open for over eighteen months.  Long term, of course it would be better to see the necessary upstream changes in Rhino and OpenJDK to provide a cleaner solution.

But for now, the rewriter is in IcedTea6 and it now uses a jar file rewritten to the sun.org.mozilla namespace.

http://icedtea.classpath.org/hg/icedtea6?cmd=changeset;node=4ac1082a1626

Given rewriting the jar file is just two fairly independent Makefile targets and a AGPL 3 licensed Java file, it could easily be used by distros in their Rhino packages so that both jars are updated together.

This will appear in IcedTea6 1.8.  It may also be worth considering backporting it to the 1.7 branch if there is sufficient demand for a fix.

Thanks a lot for fixing this, Andrew! I'm looking forward to being able to run and compile our code with OpenJDK without resorting to obscure command line switches.

For the record, I actually have started to work on a patch that includes the ScriptEngine with Rhino:

https://bugzilla.mozilla.org/show_bug.cgi?id=379385#c5

The only problem (and the reason it hasn't been committed yet) is some strange non-deterministic behaviour in which rhino engine is chosen. I haven't gotten to the bottom of this, my guess is that the script engine selection algorithm just isn't prepared to deal with multiple engines with the same name.

In case somebody wants to tackle the custom classloader approach, another solution would be to place the script engine implementation in a dedicated jar file that is not on any system class loader, e.g. jsr223-rhino.jar, and use the custom class loader to load that and the rhino jar. You definitely want the script engine implementation to be loaded with the custom class loader in order to use the javax.script interfaces to drive it. If only Rhino itself is loaded via custom classloader, you'd have to implement the script engine via reflection, which wouldn't be much fun.

(In reply to comment #9)
> This will appear in IcedTea6 1.8.  It may also be worth considering backporting
> it to the 1.7 branch if there is sufficient demand for a fix.

Assuming backporting to 1.7 would help the patch to find its way into distributions like Debian testing or Ubuntu Lucid that would be greatly appreciated.

Comment 12 Andrew John Hughes 2010-02-21 16:29:56 UTC

Targetting for 1.7.1

details: http://icedtea.classpath.org//people/andrew/icedtea8?cmd=changeset;node=8dc6e1ff8ccf
author: Andrew John Hughes <ahughes@redhat.com>
date: Thu May 20 14:37:32 2010 +0100

	Fix a number of issues and enable the Rhino rewriter in the build.

	2010-02-17 Andrew John Hughes  <ahughes@redhat.com>

		PR icedtea/179
		* ChangeLog: Remove rogue whitespace highlighted
		by emacs.
		* Makefile.am:
		(REWRITER_SRCS): Sources for class file rewriter.
		Only set RHINO_JAR when WITH_RHINO is set.  Point
		to rewritten JAR file in build directory.  Add
		rewriter and license to EXTRA_DIST.  Make icedtea,
		icedtea-debug and icedtea-boot depend on rewrite-rhino.stamp.
		(stamps/rewriter.stamp): Build the class file rewriter.
		(stamps/rewrite-rhino.stamp): Rewrite the system Rhino JAR
		file to use the sun.org.mozilla namespace to avoid conflicts.
		(rewriter): New alias for stamps/rewriter.stamp.
		(rewrite-rhino): Likewise for stamps/rewrite-rhino.stamp.
		* patches/icedtea-rhino.patch:
		Copy rather than symlink the JAR file.  Only drop the
		internal suffix on the javascript package names.
		* rewriter/com/redhat/rewriter/ClassRewriter.java:
		(executor): Use a single threaded executor when
		debugging is enabled.
		(tasks): Store the Futures returned by the executor
		for later checking.
		(main(String[])): Log what happens when processFile
		returns and handle any exceptions using Futures.
		(call()): Ensure srcDir/destDir replacement always
		matches with a trailing slash to avoid odd rewrites.
		Loop directory creation to avoid possible race issues.
		Increase logging and fix a number of issues with the rewrite:
		  * Fix off-by-one loop bug so final entry is inspected.
		  * Handle double entries which occur with 8-byte entries
		    (doubles and longs):
		http://java.sun.com/docs/books/jvms/second_edition/html/ClassFile.doc.html#16628