switch to a vmbridge for jdk_11 and above #1070
Conversation
|
Can/should we use the VMBridge also to handle jvm relevant things like SecurityManager #1068 |
At least there is also the concept of providing a custom implementation. I can think about some good reasons for having only the JVM Bridge for customize various aspects and handle all the JVM version stuff. |
There was a problem hiding this comment.
If we do some refactoring here I would think about the following ideas:
VMBridgeis not longer abstract, but contains all default implementations (prevents us from API changes)VMBridge.makeInstance()tries to instantiate the bridge by trying out the following classes:- org.mozilla.javascript.VMBridge_custom`
- org.mozilla.javascript.VMBridge_jdkXXX` (XXX = current-java version) down to
- org.mozilla.javascript.VMBridge_jdk8`
- org.mozilla.javascript.VMBridge` as last option.
| String[] classNames = { | ||
| "org.mozilla.javascript.VMBridge_custom", | ||
| "org.mozilla.javascript.jdk11.VMBridge_jdk11", | ||
| "org.mozilla.javascript.jdk18.VMBridge_jdk18", |
There was a problem hiding this comment.
I'm not happy with that naming scheme, because it can be confusing
- jdk11 could be 1.1 or 11 (here 11)
- jdk18 could be 1.8 or 18 (here 1.8)
- jdk19 ...
So jdk18 should be renamed to jdk1_8 or just jdk8
| Object target, | ||
| Scriptable topScope); | ||
|
|
||
| public abstract void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map); |
There was a problem hiding this comment.
This would be an API change. If someone has implemented VMBridge_custom - it will break the API
| "org.mozilla.javascript.jdk18.VMBridge_jdk18", | ||
| }; | ||
| for (int i = 0; i != classNames.length; ++i) { | ||
| String className = classNames[i]; | ||
| Class<?> cl = Kit.classOrNull(className); | ||
| if (cl != null) { | ||
| VMBridge bridge = (VMBridge)Kit.newInstanceOrNull(cl); | ||
| VMBridge bridge = (VMBridge) Kit.newInstanceOrNull(cl); |
There was a problem hiding this comment.
I see no code that distinguish between the different JVM versions.
So also for a jdk8 the first instance that is used
(or do we get a NoClassDefFound-error when VMBridge_jdk11 when instanttiated on jdk8)
There was a problem hiding this comment.
Kit.newInstanceOrNull return null in the case of a ClassNotFoundException so this works based on class loading; the code tries first the jdk11 version an if this fails (because we are on jdk8 it falls back to the jdk8 impl)
There was a problem hiding this comment.
because we are on jdk8 it falls back to the jdk8 impl
JavaMembers_jdk11 is perfectly valid Java 8 code, can even be run from Java 8 thanks to this:
rhino/src/org/mozilla/javascript/JavaMembers_jdk11.java
Lines 57 to 65 in c538737
|
|
||
| public abstract void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map); | ||
|
|
||
| public static final class MethodSignature { |
There was a problem hiding this comment.
MethodSignature is being used only to satisfy generics in a Map<MethodSignature, Method> but otherwise is used just as an Object. Here, it becomes public and hence part of the public API. That's one of the reasons why I kept this stuff as a package-visible subclass to JavaMembers (and @gbrail mentioned something along those lines when merging).
The current master code does not change the public API of Rhino, and would leave it as it is for 1.7.14. Once that release is out, the VMBridge API could be changed if there was an obvious gain from it, like trying to address non-public reflective accesses. I have in mind something along those lines although my approach would keep JavaMembers_jdk11 as a class.
|
#1072 is the better choice |
There is this concept of the VMBride to handle JDK specifics - let us use this for the jdk11 specific stuff to follow a unique path.