ANDROID: additional error checking on JNI an more #386

0-8-15 · 2020-11-02T11:06:36Z

Preface: I have not been able to break this in meaningful smaller patches. All changes where required to pass a meaningful test case.

1.) Android 9/10 restricts access to JNI and Jave reflection API's.

This adds additional checks.

Commented out an possible optimization, which could be assumed to
works, but does not for me so far. Please leave this comment in, so
we recall before we wonder why and try over and over again. Once in a
while we should ponder if it still does not work or why.

2.) Clear Java exceptions.

This enables to continue to run when an exception ocured during a JNI
call. (Future versions shall forward this as an exception in Gambit.)

3.) Avoid some JNI calls and provide more information about the
Java/Android environment to Scheme.

This is required for (upcoming) tricks to still call embedded dynamic
libraries as subprocesses.

It also enables to figure out a sane path to store app-private data
instead of the deprecated hard coding of the publicly accessible path
/sdcard.

4.) Add support to switch the apps content view to Java and back.

An upcoming module webview will need this.

1.) Android 9/10 restricts access to JNI and Jave reflection API's. This adds additional checks. Commented out an possible optimization, which could be assumed to works, but does not for me so far. Please leave this comment in, so we recall before we wonder why and try over and over again. Once in a while we should ponder if it still does not work or why. 2.) Clear Java exceptions. This enables to continue to run when an exception occured during a JNI call. (Future versions shall forward this as an exception in Gambit.) 3.) Avoid some JNI calls and provide more information about the Java/Android environment to Scheme. This is required for (upcoming) tricks to still call embedded dynamic libraries as subprocesses. It also enables to figure out a sane path to store app-private data instead of the deprecated hard coding of the publicly accessible path `/sdcard`. 4.) Add support to switch the apps content view to Java and back. An upcoming module `webview` will need this.

mgorges · 2020-11-05T19:28:19Z

I might break this into three commit when I push it into master, just to clearly identify what does what. Not sure yet though.

mgorges · 2020-11-06T09:59:46Z

libraries/liblambdanative/system.c

+#if 1
+  sprintf(path,"/sdcard/%s", SYS_APPNAME);
+  sys_dir=strdup(path);
+  sys_appdir=android_getFilesDir();


A potential concern to confirm during testing is what we do with apps that get upgraded and that have files in the previous location, which is no longer accessible if we do this? The approach in #380 might be an interesting (or complementary) change.

mgorges · 2020-11-06T10:00:41Z

loaders/android/bootstrap.java.in

@@ -126,42 +126,65 @@ public class @SYS_APPNAME@ extends Activity implements @ANDROID_JAVA_IMPLEMENTS@
      }
  }

+  private android.view.View current_ContentView = null;
+  @Override
+  public void setContentView(android.view.View view) {


Note to self - move to separate commit - unrelated to everything else

mgorges · 2020-11-06T10:03:56Z

loaders/android/bootstrap.java.in

 }

 class  xGLSurfaceView extends GLSurfaceView {
  public xGLSurfaceView(Context context) {
    super(context);
-    setFocusable(true); 
+    setFocusable(true);


All changes from here on out in this file are cosmetic (space at end of line). I guess the vimrc from #135 was never included anywhere?

mgorges · 2020-11-06T10:05:57Z

modules/config/config.scm

+extern char* android_getPackageCodePath();
+EOF
+)
+  (define (android-PackageCodePath) ((c-lambda () char-string "android_getPackageCodePath"))))


This seems to be a strange naming conversion for a scheme function, more like Java with CamelCase?

Also this function is not defined on non-android platforms, which may be a problem as how do we prevent users from including this in their code? Wouldn't it be safer to always declare it and simply not do anything on non-android platforms?

0-8-15 · 2020-11-06T17:30:12Z

Am Thu, 05 Nov 2020 11:28:33 -0800 schrieb Matthias Görges <[email protected]>:

I might break this into three commit when I push it into master, just to clearly identify what does what. Not sure yet though.

I don't mind. I just did not know how to test one of the changes without the other being avail as well.

mgorges · 2020-11-07T08:52:48Z

loaders/android/bootstrap.c.in

+char* android_getFilesDir() { return (char*) app_directory_files; }
+char* android_getPackageCodePath() { return (char*) app_code_path; }
+
+char* android_getFilesDir_info_get() { return android_getFilesDir(); }


Sorry why is this needed - it does the exact same thing as android_getFilesDir(), which is defined 2 lines above?

mgorges · 2020-11-07T08:54:03Z

modules/config/config.scm

+ (android
+  (c-declare #<<EOF
+extern char* android_getFilesDir_info_get();
+char* android_getFilesDir_info()


You are now making a third way of calling android_getFilesDir() - am I missing something that makes this one different? as otherwise only the extern char* android_getPackageCodePath() needs to stay here?

mgorges · 2020-11-07T09:28:03Z

loaders/android/bootstrap.c.in

+int JNI_forward_exception_to_gambit(JNIEnv*env) {
+  // TBD: actually forward, not only clear!
+  if((*env)->ExceptionCheck(env)) {
+    (*env)->ExceptionClear(env);


So a call into log_c with content as described in https://stackoverflow.com/questions/10408972/how-to-obtain-a-description-of-a-java-exception-in-c-when-using-jni ?

0-8-15 · 2020-11-07T10:08:28Z

Am Sat, 07 Nov 2020 01:28:15 -0800 schrieb Matthias Görges <[email protected]>:

@mgorges commented on this pull request. > return (error?NULL:env); } +int JNI_forward_exception_to_gambit(JNIEnv*env) { + // TBD: actually forward, not only clear! + if((*env)->ExceptionCheck(env)) { + (*env)->ExceptionClear(env); So a call into `log_c` with content as described in https://stackoverflow.com/questions/10408972/how-to-obtain-a-description-of-a-java-exception-in-c-when-using-jni ?

Either this. Though I've been more thinking of extracting info about the exception and raise an exception in the Scheme world.

0-8-15 · 2020-11-07T10:11:20Z

Am Sat, 07 Nov 2020 00:59:47 -0800 schrieb Matthias Görges <[email protected]>:

@mgorges commented on this pull request. > @@ -94,4 +94,18 @@ end-of-c-declare (gambit-c (if (string=? (system-platform) "android") (##heartbeat-interval-set! -1.))) (else (if (string=? (system-platform) "android") (##set-heartbeat-interval! -1.)))) +(cond-expand + (android + (c-declare #<<EOF +extern char* android_getFilesDir_info_get(); +char* android_getFilesDir_info() +{ + return android_getFilesDir_info_get(); +} +extern char* android_getPackageCodePath(); +EOF +) + (define (android-PackageCodePath) ((c-lambda () char-string "android_getPackageCodePath")))) Also this function is not defined on non-android platforms, which may be a problem as how do we prevent users from including this in their code? Wouldn't it be safer to always declare it and simply not do anything on non-android platforms?

No real preference about that here. Though I'd tend to not pollute the toplevel namespace with things likely to be changed again as soon as goggle think it's time to change. Also: something it will have to return. Which value? Why? Eventually useful it's going to be on Android only anyway.

0-8-15 · 2020-11-07T10:13:04Z

Am Sat, 07 Nov 2020 00:54:15 -0800 schrieb Matthias Görges <[email protected]>:

@mgorges commented on this pull request. > @@ -94,4 +94,18 @@ end-of-c-declare (gambit-c (if (string=? (system-platform) "android") (##heartbeat-interval-set! -1.))) (else (if (string=? (system-platform) "android") (##set-heartbeat-interval! -1.)))) +(cond-expand + (android + (c-declare #<<EOF +extern char* android_getFilesDir_info_get(); +char* android_getFilesDir_info() You are now making a third way of calling `android_getFilesDir()` - am I missing something that makes this one different? as otherwise only the `extern char* android_getPackageCodePath()` needs to stay here?

That's true. This getFileDir_info_get() is a leftover from my experiments. Overlooked. Should have been deleted before submission.

An upcoming module webview will need this.

Avoid some JNI calls and provide more information about the Java/Android environment to Scheme. Co-authored-by: Matthias Görges <[email protected]>

1) Android 9/10 restricts access to JNI and Jave reflection API's. This adds additional checks. Commented out an possible optimization, which could be assumed to works, but does not for me so far. Please leave this comment in, so we recall before we wonder why and try over and over again. Once in a while we should ponder if it still does not work or why. 2) Clear Java exceptions. This enables to continue to run when an exception occured during a JNI call. (Future versions shall forward this as an exception in Gambit.)

mgorges · 2020-11-09T12:04:19Z

Alright, I hope I have faithfully committed this as three separate commits. I then had to fix (*env)->ReleaseStringUTFChars(env, codePath, NULL); as I get a JNI exception with NULL not being allowed? Will need to do a bit more testing but the simple apps I tried in the simulator seem to run.

mgorges · 2020-11-20T17:47:52Z

Short of the dealing of JNI exceptions, has this been merged in functionally equivalent three changes so we can close this one?

0-8-15 · 2020-11-20T20:17:08Z

let's see.

give me some to reconcile my differences here

0-8-15 mentioned this pull request Nov 2, 2020

New module lnjscheme #387

Open

ANDROID: actually use the new file information

c1b681d

mgorges reviewed Nov 6, 2020

View reviewed changes

mgorges reviewed Nov 7, 2020

View reviewed changes

mgorges pushed a commit that referenced this pull request Nov 9, 2020

ANDROID: Add support to switch content view to Java and back #386

fc59681

An upcoming module webview will need this.

mgorges added a commit that referenced this pull request Nov 9, 2020

ANDROID: change sys_appdir to use getFilesDir() #386

7312d92

Avoid some JNI calls and provide more information about the Java/Android environment to Scheme. Co-authored-by: Matthias Görges <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ANDROID: additional error checking on JNI an more #386

ANDROID: additional error checking on JNI an more #386

0-8-15 commented Nov 2, 2020

mgorges commented Nov 5, 2020

mgorges Nov 6, 2020

mgorges Nov 6, 2020

mgorges Nov 6, 2020

mgorges Nov 6, 2020

mgorges Nov 7, 2020

0-8-15 commented Nov 6, 2020 via email

mgorges Nov 7, 2020

mgorges Nov 7, 2020

mgorges Nov 7, 2020

0-8-15 commented Nov 7, 2020 via email

0-8-15 commented Nov 7, 2020 via email

0-8-15 commented Nov 7, 2020 via email

mgorges commented Nov 9, 2020

mgorges commented Nov 20, 2020

0-8-15 commented Nov 20, 2020

ANDROID: additional error checking on JNI an more #386

Are you sure you want to change the base?

ANDROID: additional error checking on JNI an more #386

Conversation

0-8-15 commented Nov 2, 2020

mgorges commented Nov 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0-8-15 commented Nov 6, 2020 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0-8-15 commented Nov 7, 2020 via email

0-8-15 commented Nov 7, 2020 via email

0-8-15 commented Nov 7, 2020 via email

mgorges commented Nov 9, 2020

mgorges commented Nov 20, 2020

0-8-15 commented Nov 20, 2020