Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

switch to a vmbridge for jdk_11 and above #1070

Closed
wants to merge 1 commit into from
Closed

switch to a vmbridge for jdk_11 and above #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 3 additions & 57 deletions src/org/mozilla/javascript/JavaMembers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@
import java.security.AllPermission;
import java.security.Permission;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import javax.lang.model.SourceVersion;
import org.mozilla.javascript.VMBridge.MethodSignature;

/**
* @author Mike Shaver
Expand All @@ -33,9 +32,6 @@
*/
class JavaMembers {

private static final boolean STRICT_REFLECTIVE_ACCESS =
SourceVersion.latestSupported().ordinal() > 8;

private static final Permission allPermission = new AllPermission();

JavaMembers(Scriptable scope, Class<?> cl) {
Expand Down Expand Up @@ -345,7 +341,7 @@ private void discoverAccessibleMethods(
}
}
} else {
discoverPublicMethods(clazz, map);
VMBridge.instance.discoverPublicMethods(clazz, map);
}
return;
} catch (SecurityException e) {
Expand All @@ -369,47 +365,6 @@ private void discoverAccessibleMethods(
}
}

void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) {
Method[] methods = clazz.getMethods();
for (Method method : methods) {
registerMethod(map, method);
}
}

static void registerMethod(Map<MethodSignature, Method> map, Method method) {
MethodSignature sig = new MethodSignature(method);
// Array may contain methods with same signature but different return value!
map.putIfAbsent(sig, method);
}

static final class MethodSignature {
private final String name;
private final Class<?>[] args;

private MethodSignature(String name, Class<?>[] args) {
this.name = name;
this.args = args;
}

MethodSignature(Method method) {
this(method.getName(), method.getParameterTypes());
}

@Override
public boolean equals(Object o) {
if (o instanceof MethodSignature) {
MethodSignature ms = (MethodSignature) o;
return ms.name.equals(name) && Arrays.equals(args, ms.args);
}
return false;
}

@Override
public int hashCode() {
return name.hashCode() ^ args.length;
}
}

private void reflect(Scriptable scope, boolean includeProtected, boolean includePrivate) {
// We reflect methods first, because we want overloaded field/method
// names to be allocated to the NativeJavaMethod before the field
Expand Down Expand Up @@ -782,7 +737,7 @@ static JavaMembers lookupClass(
return members;
}
try {
members = createJavaMembers(cache.getAssociatedScope(), cl, includeProtected);
members = new JavaMembers(cache.getAssociatedScope(), cl, includeProtected);
break;
} catch (SecurityException e) {
// Reflection may fail for objects that are in a restricted
Expand Down Expand Up @@ -818,15 +773,6 @@ static JavaMembers lookupClass(
return members;
}

private static JavaMembers createJavaMembers(
Scriptable associatedScope, Class<?> cl, boolean includeProtected) {
if (STRICT_REFLECTIVE_ACCESS) {
return new JavaMembers_jdk11(associatedScope, cl, includeProtected);
} else {
return new JavaMembers(associatedScope, cl, includeProtected);
}
}

private static Object getSecurityContext() {
Object sec = null;
SecurityManager sm = System.getSecurityManager();
Expand Down
111 changes: 68 additions & 43 deletions src/org/mozilla/javascript/VMBridge.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,24 @@

import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Method;
import java.util.Arrays;
import java.util.Map;

public abstract class VMBridge
{
public abstract class VMBridge {

static final VMBridge instance = makeInstance();

private static VMBridge makeInstance()
{
private static VMBridge makeInstance() {
String[] classNames = {
"org.mozilla.javascript.VMBridge_custom",
"org.mozilla.javascript.jdk11.VMBridge_jdk11",
"org.mozilla.javascript.jdk18.VMBridge_jdk18",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@rbri rbri Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@carlosame carlosame Oct 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Obtain the module
Method getmodule;
Class<?> cl = clazz.getClass();
try {
getmodule = cl.getMethod("getModule");
} catch (NoSuchMethodException e) {
// We are on non-modular Java
return true;
}

if (bridge != null) {
return bridge;
}
Expand All @@ -37,49 +38,45 @@ private static VMBridge makeInstance()

/**
* Return a helper object to optimize {@link Context} access.
* <p>
* The runtime will pass the resulting helper object to the subsequent
* calls to {@link #getContext(Object contextHelper)} and
* {@link #setContext(Object contextHelper, Context cx)} methods.
* In this way the implementation can use the helper to cache
* information about current thread to make {@link Context} access faster.
*
* <p>The runtime will pass the resulting helper object to the subsequent calls to {@link
* #getContext(Object contextHelper)} and {@link #setContext(Object contextHelper, Context cx)}
* methods. In this way the implementation can use the helper to cache information about current
* thread to make {@link Context} access faster.
*/
protected abstract Object getThreadContextHelper();

/**
* Get {@link Context} instance associated with the current thread
* or null if none.
* Get {@link Context} instance associated with the current thread or null if none.
*
* @param contextHelper The result of {@link #getThreadContextHelper()}
* called from the current thread.
* @param contextHelper The result of {@link #getThreadContextHelper()} called from the current
* thread.
*/
protected abstract Context getContext(Object contextHelper);

/**
* Associate {@link Context} instance with the current thread or remove
* the current association if <code>cx</code> is null.
* Associate {@link Context} instance with the current thread or remove the current association
* if <code>cx</code> is null.
*
* @param contextHelper The result of {@link #getThreadContextHelper()}
* called from the current thread.
* @param contextHelper The result of {@link #getThreadContextHelper()} called from the current
* thread.
*/
protected abstract void setContext(Object contextHelper, Context cx);

/**
* In many JVMSs, public methods in private
* classes are not accessible by default (Sun Bug #4071593).
* VMBridge instance should try to workaround that via, for example,
* calling method.setAccessible(true) when it is available.
* The implementation is responsible to catch all possible exceptions
* like SecurityException if the workaround is not available.
* In many JVMSs, public methods in private classes are not accessible by default (Sun Bug
* #4071593). VMBridge instance should try to workaround that via, for example, calling
* method.setAccessible(true) when it is available. The implementation is responsible to catch
* all possible exceptions like SecurityException if the workaround is not available.
*
* @return true if it was possible to make method accessible
* or false otherwise.
* @return true if it was possible to make method accessible or false otherwise.
*/
protected abstract boolean tryToMakeAccessible(AccessibleObject accessible);

/**
* Create helper object to create later proxies implementing the specified
* interfaces later. Under JDK 1.3 the implementation can look like:
* Create helper object to create later proxies implementing the specified interfaces later.
* Under JDK 1.3 the implementation can look like:
*
* <pre>
* return java.lang.reflect.Proxy.getProxyClass(..., interfaces).
* getConstructor(new Class[] {
Expand All @@ -88,22 +85,50 @@ private static VMBridge makeInstance()
*
* @param interfaces Array with one or more interface class objects.
*/
protected abstract Object getInterfaceProxyHelper(ContextFactory cf,
Class<?>[] interfaces);
protected abstract Object getInterfaceProxyHelper(ContextFactory cf, Class<?>[] interfaces);

/**
* Create proxy object for {@link InterfaceAdapter}. The proxy should call
* {@link InterfaceAdapter#invoke(ContextFactory, Object, Scriptable,
* Object, Method, Object[])}
* as implementation of interface methods associated with
* <code>proxyHelper</code>. {@link Method}
* Create proxy object for {@link InterfaceAdapter}. The proxy should call {@link
* InterfaceAdapter#invoke(ContextFactory, Object, Scriptable, Object, Method, Object[])} as
* implementation of interface methods associated with <code>proxyHelper</code>. {@link Method}
*
* @param proxyHelper The result of the previous call to
* {@link #getInterfaceProxyHelper(ContextFactory, Class[])}.
* @param proxyHelper The result of the previous call to {@link
* #getInterfaceProxyHelper(ContextFactory, Class[])}.
*/
protected abstract Object newInterfaceProxy(Object proxyHelper,
ContextFactory cf,
InterfaceAdapter adapter,
Object target,
Scriptable topScope);
protected abstract Object newInterfaceProxy(
Object proxyHelper,
ContextFactory cf,
InterfaceAdapter adapter,
Object target,
Scriptable topScope);

public abstract void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be an API change. If someone has implemented VMBridge_custom - it will break the API


public static final class MethodSignature {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

private final String name;
private final Class<?>[] args;

private MethodSignature(String name, Class<?>[] args) {
this.name = name;
this.args = args;
}

public MethodSignature(Method method) {
this(method.getName(), method.getParameterTypes());
}

@Override
public boolean equals(Object o) {
if (o instanceof MethodSignature) {
MethodSignature ms = (MethodSignature) o;
return ms.name.equals(name) && Arrays.equals(args, ms.args);
}
return false;
}

@Override
public int hashCode() {
return name.hashCode() ^ args.length;
}
}
}
13 changes: 4 additions & 9 deletions ...mozilla/javascript/JavaMembers_jdk11.java → ...illa/javascript/jdk11/VMBridge_jdk11.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,17 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */

package org.mozilla.javascript;
package org.mozilla.javascript.jdk11;

import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Proxy;
import java.util.Map;
import org.mozilla.javascript.jdk18.VMBridge_jdk18;

/** Version of {@link JavaMembers} for modular JDKs. */
class JavaMembers_jdk11 extends JavaMembers {

JavaMembers_jdk11(Scriptable scope, Class<?> cl, boolean includeProtected) {
super(scope, cl, includeProtected);
}

public class VMBridge_jdk11 extends VMBridge_jdk18 {
@Override
void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) {
public void discoverPublicMethods(Class<?> clazz, Map<MethodSignature, Method> map) {
if (isExportedClass(clazz)) {
super.discoverPublicMethods(clazz, map);
} else {
Expand Down
Loading