[mobile] mobile/bind: make LOG_FATAL abort() on Android

[Elias Naur (Gerrit)]

Elias Naur uploaded a change:
https://go-review.googlesource.com/20257

mobile/bind: make LOG_FATAL abort() on Android

LOG_FATAL already throws an exception on iOS. Make it abort() on
Android, so that any fatal error will hopefully end up with a useful
log instead of an easily missed message in logcat.

Also, remove return statements after LOG_FATAL on both platforms.
They're unnecessary and confusing and they weren't used consistently
anyway.

Change-Id: I2a8e2e0ac064e95f52ca130de17265c9741cefe4
---
M bind/java/seq.h
M bind/java/seq_android.c.support
M bind/objc/seq_darwin.m.support
3 files changed, 5 insertions(+), 25 deletions(-)



diff --git a/bind/java/seq.h b/bind/java/seq.h
index c6e16d3..f859b82 100644
--- a/bind/java/seq.h
+++ b/bind/java/seq.h
@@ -10,7 +10,11 @@
#include <jni.h>

#define LOG_INFO(...) __android_log_print(ANDROID_LOG_INFO, "go/Seq",
__VA_ARGS__)
-#define LOG_FATAL(...) __android_log_print(ANDROID_LOG_FATAL, "go/Seq",
__VA_ARGS__)
+#define LOG_FATAL(...) \
+ { \
+ __android_log_print(ANDROID_LOG_FATAL, "go/Seq", __VA_ARGS__); \
+ abort(); \
+ }

// Platform specific types
typedef struct nstring {
diff --git a/bind/java/seq_android.c.support
b/bind/java/seq_android.c.support
index b1a095a..0233eb2 100644
--- a/bind/java/seq_android.c.support
+++ b/bind/java/seq_android.c.support
@@ -48,11 +48,9 @@
if (ret != JNI_OK) {
if (ret != JNI_EDETACHED) {
LOG_FATAL("failed to get thread env");
- return;
}
if ((*jvm)->AttachCurrentThread(jvm, &env, NULL) != JNI_OK) {
LOG_FATAL("failed to attach current thread");
- return;
}
pthread_setspecific(jnienvs, env);
}
@@ -81,7 +79,6 @@
jbyteArray res = (*env)->NewByteArray(env, s.len);
if (res == NULL) {
LOG_FATAL("NewByteArray failed");
- return res;
}
(*env)->SetByteArrayRegion(env, res, 0, s.len, s.ptr);
if (copy) {
@@ -187,7 +184,6 @@
jchar *chars = (jchar *)(*env)->GetStringChars(env, str, NULL);
if (chars == NULL) {
LOG_FATAL("GetStringChars failed");
- return res;
}
nstring nstr = utf16_decode(chars, nchars);
(*env)->ReleaseStringChars(env, str, chars);
@@ -207,13 +203,11 @@
jbyte *ptr = (*env)->GetByteArrayElements(env, arr, NULL);
if (ptr == NULL) {
LOG_FATAL("GetByteArrayElements failed");
- return res;
}
if (copy) {
void *ptr_copy = (void *)malloc(len);
if (ptr_copy == NULL) {
LOG_FATAL("malloc failed");
- return res;
}
memcpy(ptr_copy, ptr, len);
(*env)->ReleaseByteArrayElements(env, arr, ptr, JNI_ABORT);
@@ -239,7 +233,6 @@
jobject ref = (*env)->CallStaticObjectMethod(env, seq_class, seq_getRef,
(jint)refnum);
if (ref == NULL) {
LOG_FATAL("Unknown reference: %d", refnum);
- return NULL;
}
if (refnum > 0) { // Java object
// return ref.obj
@@ -283,54 +276,44 @@
Java_go_Seq_init(JNIEnv *env, jclass clazz) {
if ((*env)->GetJavaVM(env, &jvm) != 0) {
LOG_FATAL("failed to get JVM");
- return;
}
if (pthread_key_create(&jnienvs, env_destructor) != 0) {
LOG_FATAL("failed to initialize jnienvs thread local storage");
- return;
}

seq_class = (*env)->NewGlobalRef(env, clazz);
seq_throw_exc = (*env)->GetStaticMethodID(env,
seq_class, "throwException", "(Ljava/lang/String;)V");
if (seq_throw_exc == NULL) {
LOG_FATAL("failed to find method Seq.throwException");
- return;
}

seq_getRef = (*env)->GetStaticMethodID(env,
seq_class, "getRef", "(I)Lgo/Seq$Ref;");
if (seq_getRef == NULL) {
LOG_FATAL("failed to find method Seq.getRef");
- return;
}
seq_decRef = (*env)->GetStaticMethodID(env, seq_class, "decRef", "(I)V");
if (seq_decRef == NULL) {
LOG_FATAL("failed to find method Seq.decRef");
- return;
}
seq_incRef = (*env)->GetStaticMethodID(env,
seq_class, "incRef", "(Lgo/Seq$Object;)I");
if (seq_incRef == NULL) {
LOG_FATAL("failed to find method Seq.incRef");
- return;
}
jclass throwable_class = (*env)->FindClass(env, "java/lang/Throwable");
if (throwable_class == NULL) {
LOG_FATAL("failed to find Throwable class");
- return;
}
throwable_getMessage = (*env)->GetMethodID(env,
throwable_class, "getMessage", "()Ljava/lang/String;");
if (throwable_getMessage == NULL) {
LOG_FATAL("failed to find method Throwable.getMessage");
- return;
}
jclass ref_class = (*env)->FindClass(env, "go/Seq$Ref");
if (ref_class == NULL) {
LOG_FATAL("failed to find the Seq.Ref class");
- return;
}
ref_objField = (*env)->GetFieldID(env,
ref_class, "obj", "Lgo/Seq$Object;");
if (ref_objField == NULL) {
LOG_FATAL("failed to find the Seq.Ref.obj field");
- return;
}
}

diff --git a/bind/objc/seq_darwin.m.support b/bind/objc/seq_darwin.m.support
index 1e6482c..44d8f46 100644
--- a/bind/objc/seq_darwin.m.support
+++ b/bind/objc/seq_darwin.m.support
@@ -168,7 +168,6 @@
void *arr_copy = malloc(sz);
if (arr_copy == NULL) {
LOG_FATAL(@"malloc failed");
- return res;
}
memcpy(arr_copy, [data bytes], sz);
ptr = arr_copy;
@@ -194,7 +193,6 @@
char *buf = (char *)malloc(len);
if (buf == NULL) {
LOG_FATAL(@"malloc failed");
- return res;
}
NSUInteger used;
[s getBytes:buf
@@ -214,7 +212,6 @@

- (id)init {
LOG_FATAL(@"GoSeqRef init is disallowed");
- return nil;
}

// called when an object from Go is passed in.
@@ -279,14 +276,12 @@
- (void)dec:(int32_t)refnum { // called whenever a go proxy object is
finalized.
if (IS_FROM_GO(refnum)) {
LOG_FATAL(@"dec:invalid refnum for Objective-C objects");
- return;
}
@synchronized(self) {
id key = @(refnum);
RefCounter *counter = [_objs objectForKey:key];
if (counter == NULL) {
LOG_FATAL(@"unknown refnum");
- return;
}
int n = counter.cnt;
if (n <= 0) {
@@ -305,13 +300,11 @@
- (id)get:(int32_t)refnum {
if (IS_FROM_GO(refnum)) {
LOG_FATAL(@"get:invalid refnum for Objective-C objects");
- return NULL;
}
@synchronized(self) {
RefCounter *counter = _objs[@(refnum)];
if (counter == NULL) {
LOG_FATAL(@"unidentified object refnum: %d", refnum);
- return NULL;
}
return counter.obj;
}

--
https://go-review.googlesource.com/20257

[Hyang-Ah Hana Kim (Gerrit)]

Hyang-Ah Hana Kim has posted comments on this change.

mobile/bind: make LOG_FATAL abort() on Android

Patch Set 1: Code-Review+2

I didn't abort because it's not clear to me how android behaves - is it
okay to abort the app before the logger prints and reacts to the fatal
message?

--
https://go-review.googlesource.com/20257
Gerrit-Reviewer: Hyang-Ah Hana Kim <hya...@gmail.com>
Gerrit-HasComments: No

[Elias Naur (Gerrit)]

Elias Naur has posted comments on this change.

mobile/bind: make LOG_FATAL abort() on Android

Patch Set 1:

> I didn't abort because it's not clear to me how android behaves -
> is it okay to abort the app before the logger prints and reacts to
> the fatal message?

I believe so. Forcing a LOG_FATAL gives me the dump below in the logcat.
Not only is the message printed ("GetStringChars failed"), it is also
included in the abort message ("Abort message: 'GetStringChars failed'").

And even if the message doesn't appear, the backtrace probably will. Most
importantly, the app is not allowed to continue in an undefined and
undertested
state.




GetStringChars failed
Fatal signal 6 (SIGABRT), code -6 in tid 4524 (ationTestRunner)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build
fingerprint: 'google/hammerhead/hammerhead:6.0.1/MMB29Q/2480792:user/release-keys'
Revision: '0'
ABI: 'arm'
pid: 4499, tid: 4524, name: ationTestRunner >>> com.example.BindTest.test
<<<
signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
Abort message: 'GetStringChars failed'
r0 00000000 r1 000011ac r2 00000006 r3 af01f978
r4 af01f980 r5 af01f930 r6 00000002 r7 0000010c
r8 af01f010 r9 ab415de0 sl af01f25c fp af01f134
ip 00000006 sp af01ef88 lr b6cc8b61 pc b6ccaf50 cpsr 400f0010

backtrace:
#00 pc 00041f50 /system/lib/libc.so (tgkill+12)
#01 pc 0003fb5d /system/lib/libc.so (pthread_kill+32)
#02 pc 0001c30f /system/lib/libc.so (raise+10)
#03 pc 000194c1 /system/lib/libc.so (__libc_android_abort+34)
#04 pc 000174ac /system/lib/libc.so (abort+4)
#05 pc 00127700
/data/app/com.example.BindTest.test-1/lib/arm/libgojni.so
(go_seq_from_java_string+580)
#06 pc 0010204c [stack:4524]

--
https://go-review.googlesource.com/20257
Gerrit-Reviewer: Elias Naur <elias...@gmail.com>

[Elias Naur (Gerrit)]

Elias Naur has submitted this change and it was merged.

mobile/bind: make LOG_FATAL abort() on Android

LOG_FATAL already throws an exception on iOS. Make it abort() on
Android, so that any fatal error will hopefully end up with a useful
log instead of an easily missed message in logcat.

Also, remove return statements after LOG_FATAL on both platforms.
They're unnecessary and confusing and they weren't used consistently
anyway.

Change-Id: I2a8e2e0ac064e95f52ca130de17265c9741cefe4

Reviewed-on: https://go-review.googlesource.com/20257
Reviewed-by: Hyang-Ah Hana Kim <hya...@gmail.com>

---
M bind/java/seq.h
M bind/java/seq_android.c.support
M bind/objc/seq_darwin.m.support
3 files changed, 5 insertions(+), 25 deletions(-)

Approvals:
Hyang-Ah Hana Kim: Looks good to me, approved