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.

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

fix: bytecode version unspecified & NPE and trySetAccessible method does not exist on Android #1636

Closed
wants to merge 2 commits into from

Conversation

naijun0403
Copy link
Contributor

@naijun0403 naijun0403 commented Sep 20, 2024

Fixes #1634

@gbrail
Copy link
Collaborator

gbrail commented Sep 21, 2024

Thanks -- I am going to open a PR soon that uses the "Errorprone" plugin to help detect these kinds of things proactively, but TBH nothing is going to be really permanently better until we have some way to do some sort of Android smoke test as part of the build, or somewhere in GitHub. Do you have any idea how other projects might handle this?

In the meantime, I'd love to understand better why the change to the class file writing is necessary. I'd be even happier if we could find some way to write a test for it -- any ideas?

@naijun0403
Copy link
Contributor Author

Thank you for your feedback!

Regarding testing:
I'm aware that the nowinandroid repository uses Android emulators for testing. However, my knowledge of testing methodologies is limited, so I can't provide comprehensive suggestions for alternative testing methods.

Reason for modifying ClassFileWriter.java:
The modification was necessary due to Android's unique runtime environment. Android doesn't use the JVM, so .class files are not included in the app. Consequently, in the source code, inputStream was always null, leading to a NPE that prevented the entire engine from functioning.

Changes made:

  1. I added a null check for inputStream.
  2. If inputStream is null, I substitute default minor and major version values (retrieved from a previous commit) to ensure null safety in subsequent operations.

This approach aims to prevent NPE and maintain the engine's functionality in the Android environment.

@@ -328,7 +328,8 @@ private void discoverAccessibleMethods(
if (isPublic(mods) || isProtected(mods) || includePrivate) {
MethodSignature sig = new MethodSignature(method);
if (!map.containsKey(sig)) {
if (includePrivate) method.trySetAccessible();
if (includePrivate && !method.isAccessible())
method.setAccessible(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I stumbled upon this place in #1575 (which probably will cause merge conflicts)
I'm wondering, if we should replace this with VMBridge.instance.tryToMakeAccessible(method)
which will effectively call this code: https://github.com/mozilla/rhino/blob/master/rhino/src/main/java/org/mozilla/javascript/jdk18/VMBridge_jdk18.java#L57

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that's a good idea. I guess it will work on Android too.

@gbrail
Copy link
Collaborator

gbrail commented Sep 26, 2024

Thanks! Resolved the conflict and merged manually.

@gbrail gbrail closed this Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaAdapter unavailable in Android environment on upstream rhino
3 participants