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

Handle remaining JNI calls error handling #1092

Merged
merged 1 commit into from
Feb 1, 2021

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Jan 26, 2021

Goal

Checks additional JNI calls in the NDK module for pending exceptions and no-ops, building on #1088. This is achieved by calling ExceptionClear to remove any pending JVM exceptions that could lead to app termination.

Changeset

Added safe versions of the following methods and updated code to check the return value where appropriate:

GetStaticFieldID
GetStaticObjectField
NewObject
CallObjectMethod
CallStaticVoidMethod
CallStaticObjectMethod

Testing

Relied on existing test coverage.

@fractalwrench fractalwrench force-pushed the PLAT-5746/remaining-jni-handling branch from 2cdeb94 to b5b16d3 Compare January 26, 2021 17:26
@fractalwrench fractalwrench force-pushed the PLAT-5746/jni-handling branch 2 times, most recently from 72c392d to b661294 Compare January 28, 2021 10:29
Base automatically changed from PLAT-5746/jni-handling to next January 28, 2021 11:35
@fractalwrench fractalwrench force-pushed the PLAT-5746/remaining-jni-handling branch 5 times, most recently from bc7213f to 0738da9 Compare January 28, 2021 15:14
@fractalwrench fractalwrench changed the title fix: handle remaining JNI calls error handling Handle remaining JNI calls error handling Jan 28, 2021
@fractalwrench fractalwrench marked this pull request as ready for review January 28, 2021 16:25
@bugsnagbot
Copy link
Collaborator

bugsnagbot commented Jan 28, 2021

Android notifier sizes

Format Size impact of Bugsnag (kB) Size impact of Bugsnag when Minified (kB)
APK 1468.72 1383.39
arm64_v8a 381.61 291.5
armeabi 357.03 271.01
armeabi_v7a 340.65 254.64
x86 418.45 332.44
x86_64 402.08 311.97

Generated by 🚫 Danger

Copy link
Contributor

@kstenerud kstenerud left a comment

Choose a reason for hiding this comment

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

There are many functions that have existing/new leak issues that can be fixed with c-style RAII. We should probably just convert everything over once and for all...

// get the breadcrumb type
jobject jtype = bsg_safe_get_static_object_field(env, type_class, crumb_type);
if (jtype == NULL) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will crumb_type leak here?

bsg_safe_new_object(env, trace_class, trace_constructor, class, method,
filename, frame.line_number);
if (jframe == NULL) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

trace will leak here

jobject jseverity =
bsg_safe_get_static_object_field(env, severity_class, severity_field);
if (jseverity == NULL) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

These will leak things as well. This should probably be a c-style RAII

env, jni_cache->arraylist, jni_cache->arraylist_init_with_obj, keyset);
if (keylist == NULL) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

These will leak things. Should be c-style RAII

@fractalwrench fractalwrench force-pushed the PLAT-5746/remaining-jni-handling branch 2 times, most recently from 1c30044 to 0c872c3 Compare January 29, 2021 14:00
@fractalwrench fractalwrench force-pushed the PLAT-5746/remaining-jni-handling branch from 0c872c3 to 8bb86a9 Compare January 29, 2021 14:07
@fractalwrench
Copy link
Contributor Author

Thanks @kstenerud - I've updated the changeset so that the functions use RAII where it seemed appropriate.

Copy link
Contributor

@kstenerud kstenerud left a comment

Choose a reason for hiding this comment

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

LGTM!

@fractalwrench fractalwrench merged commit 2c3900b into next Feb 1, 2021
@fractalwrench fractalwrench deleted the PLAT-5746/remaining-jni-handling branch February 1, 2021 09:10
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.

3 participants