Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Catch C++ exceptions before returning to JNI #2825

Closed
1ec5 opened this issue Oct 27, 2015 · 5 comments
Closed

Catch C++ exceptions before returning to JNI #2825

1ec5 opened this issue Oct 27, 2015 · 5 comments
Assignees
Labels
Android Mapbox Maps SDK for Android bug crash

Comments

@1ec5
Copy link
Contributor

1ec5 commented Oct 27, 2015

Per #2804, the Android SDK suffers from a native crash any time there’s an uncaught C++ exception at the mbgl level. An uncaught exception there usually indicates developer error, so the Android SDK needs to present that error in a way that’s actionable without spelunking into mbgl code. The Android bindings cannot rely on an assumption that the C++ code won’t throw exceptions.

platform/android/jni.cpp needs to have C-style exception handling. For example, nativeSetStyleURL() should be wrapped in a try-catch statement. When an exception is caught, this function should return some kind of error code. For functions that already need to return a value, this would imply some kind of out parameter or a tuple-typed return value.

Though this could be a fair amount of work, it pays off in the form of Android bindings that can actually be debugged.

/cc @ljbade @bleege @jfirebaugh

@1ec5 1ec5 added bug Android Mapbox Maps SDK for Android crash labels Oct 27, 2015
@ljbade
Copy link
Contributor

ljbade commented Oct 27, 2015

As discussed in slack a wrinkle with nativeSetStyleURL is that the invalid style URL error will be thrown from the map thread, not the UI thread, so adding a catch to nativeSetStyleURL will not work.

Instead we can take advantage of how Java checking JNI thrown Java exceptions. If we convert exception to Java exception in the map thread instead of killing the thread. Then when we next call into Java land (View::invalidate) the exception will be picked up and crash the app Java side with a useful message.

We also need to log the C++ exception stack trace to logcat first so that in the log we have both the original stack trace, and the new Java one which will point to invalidate()

@ljbade
Copy link
Contributor

ljbade commented Oct 27, 2015

@1ec5 Historic context on my previous attempt at C++ exception handling:
#562

@ljbade
Copy link
Contributor

ljbade commented Oct 28, 2015

@1ec5 Should we also open a iOS ticket for iOS exception handling? Will help in #2775 (comment)

I don't believe iOS has any direct way of knowing if a style has failed to load.

@1ec5
Copy link
Contributor Author

1ec5 commented Oct 28, 2015

@ljbade, once #2762 is fixed, iOS will get such notifications for free.

@bleege
Copy link
Contributor

bleege commented Oct 28, 2015

once #2762 is fixed, iOS will get such notifications for free.

The joys of living in an all C++ world. 😃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android bug crash
Projects
None yet
Development

No branches or pull requests

4 participants