feat: expose a module loader for Android thru sentry-native #1043

marandaneto · 2020-11-13T09:05:42Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

feat: expose a module loader for Android thru sentry-native

💡 Motivation and Context

Flutter apps when has split symbols enabled need the debug images for proper server symbolization and the Dart/Flutter API doesn't offer it natively.

💚 How did you test it?

📝 Checklist

I reviewed submitted code
I added tests to verify changes
All tests passing
No breaking changes

🔮 Next steps

codecov-io · 2020-11-13T09:52:08Z

Codecov Report

Merging #1043 (af6f4d7) into main (ae8c568) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #1043      +/-   ##
============================================
- Coverage     72.06%   72.03%   -0.03%     
  Complexity     1338     1338              
============================================
  Files           138      138              
  Lines          4893     4895       +2     
  Branches        499      499              
============================================
  Hits           3526     3526              
- Misses         1106     1108       +2     
  Partials        261      261

Impacted Files	Coverage Δ	Complexity Δ
...y/src/main/java/io/sentry/protocol/DebugImage.java	`3.12% <0.00%> (-0.21%)`	`1.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae8c568...af6f4d7. Read the comment docs.

Swatinem

lgtm!

You should add the type, debug_id, code_id and debug_file props as well.

philipphofmann

The overall PR looks great. I have added a few important comments about naming and in which package to put the classes. Can we maybe also add an integration test for this?

sentry-android-core/src/main/java/io/sentry/android/core/IDebugImagesLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/IModuleListLoader.java

sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugImagesLoaderTest.kt

sentry-android-ndk/src/main/java/io/sentry/android/ndk/IModuleListLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java

sentry-android-core/src/main/java/io/sentry/android/core/IDebugImagesLoader.java

sentry/src/main/java/io/sentry/protocol/DebugImage.java

…agesLoaderTest.kt Co-authored-by: Philipp Hofmann <[email protected]>

… into feat/module_loader

philipphofmann

One important comment and a few nitpicks.

sentry-android-core/src/main/java/io/sentry/android/core/NdkIntegration.java

sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/DebugImagesLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/IModuleListLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NativeModuleListLoader.java

sentry-android-ndk/src/main/java/io/sentry/android/ndk/IModuleListLoader.java

philipphofmann

The only thing stopping me to approve this is to add some integration tests to validate that the JNI calls are working properly. Can we add some?

marandaneto · 2020-11-16T18:18:13Z

The only thing stopping me to approve this is to add some integration tests to validate that the JNI calls are working properly. Can we add some?

all good then, we can't run Android native code on the JVM, so we can't really do it.
Ideally, this is gonna be tested along with our E2E tests which are not there yet.

philipphofmann

As said integration tests would make sense, but we had some trouble in the past adding them and we don't want this to block this PR.

bruno-garcia

I worry that we do all this work to read a single entry (libapp.so) for Flutter only. Wouldn't it make sense to add a single: getByImageName or by id or something?

bruno-garcia · 2020-11-17T18:40:08Z

sentry-android-ndk/src/main/jni/sentry.c

+
+JNIEXPORT jobjectArray JNICALL
+Java_io_sentry_android_ndk_NativeModuleListLoader_nativeLoadModuleList(JNIEnv *env, jclass cls) {
+    sentry_value_t image_list_t = sentry_get_modules_list();


Maybe we drop the _t? or is this really a type

Suggested change

sentry_value_t image_list_t = sentry_get_modules_list();

sentry_value_t image_list = sentry_get_modules_list();

well unless we rename jobjectArray but either of them needs a suffix or prefix, as they are the same thing but using a different data type

bruno-garcia · 2020-11-17T18:40:19Z

sentry-android-ndk/src/main/jni/sentry.c

+    jobjectArray image_list = NULL;
+
+    if (sentry_value_get_type(image_list_t) == SENTRY_VALUE_TYPE_LIST) {
+        size_t len_t = sentry_value_get_length(image_list_t);


Suggested change

size_t len_t = sentry_value_get_length(image_list_t);

size_t len = sentry_value_get_length(image_list_t);

https://github.com/getsentry/sentry-java/pull/1043/files#r525421746

bruno-garcia · 2020-11-17T18:40:41Z

sentry-android-ndk/src/main/jni/sentry.c

+    if (sentry_value_get_type(image_list_t) == SENTRY_VALUE_TYPE_LIST) {
+        size_t len_t = sentry_value_get_length(image_list_t);
+
+        jclass image_class = (*env)->FindClass(env, "io/sentry/protocol/DebugImage");


Do we expect this method to be called multiple times? I wonder if would be worth caching these FindClass results

unless somebody call clearDebugImages its used only once

bruno-garcia · 2020-11-17T18:45:53Z

sentry-android-ndk/src/main/jni/sentry.c

+            sentry_value_t image_t = sentry_value_get_by_index(image_list_t, i);
+
+            if (!sentry_value_is_null(image_t)) {
+                jobject image = (*env)->NewObject(env, image_class, image_addr_ctor);


when/where is this freed?

after talking to @Swatinem this was not necessary, maybe after #1050 this is worth another review?
@Swatinem mind reviewing this part again?

It looks fine from my side. TBH, I have no idea how the ownership transfer works for these JNI object, and if the GC will ever run concurrently with this call? If we would need to manually root these things on the C stack, or if we need to unref them rather.

marandaneto · 2020-11-17T19:18:15Z

I worry that we do all this work to read a single entry (libapp.so) for Flutter only. Wouldn't it make sense to add a single: getByImageName or by id or something?

doing a string.contains('libapp') is suboptimal IMO and not worth the savings

Swatinem · 2020-11-18T13:31:09Z

sentry-android-ndk/src/main/jni/sentry.c

+            sentry_value_t image_t = sentry_value_get_by_index(image_list_t, i);
+
+            if (!sentry_value_is_null(image_t)) {
+                jobject image = (*env)->NewObject(env, image_class, image_addr_ctor);


It looks fine from my side. TBH, I have no idea how the ownership transfer works for these JNI object, and if the GC will ever run concurrently with this call? If we would need to manually root these things on the C stack, or if we need to unref them rather.

feat: expose a module loader for Android thru sentry-native

6f373ea

marandaneto requested review from bruno-garcia and maciejwalkowiak as code owners November 13, 2020 09:05

marandaneto marked this pull request as draft November 13, 2020 09:05

marandaneto added 2 commits November 13, 2020 10:08

fix identation

25e8903

identation

ee0ae43

marandaneto added 7 commits November 13, 2020 11:59

fix

08bbf3a

fix

dc1680b

add image size as primitive type

58affbd

fix

7345845

remove useless comments

3b311b7

write tests

849b088

add javadocs

a757838

marandaneto marked this pull request as ready for review November 13, 2020 12:59

changelog

0737503

marandaneto requested review from Swatinem and a team November 13, 2020 13:00

Swatinem approved these changes Nov 13, 2020

View reviewed changes

marandaneto added 2 commits November 13, 2020 14:46

added missing fields

aea36a3

identiation

48bf90d

philipphofmann reviewed Nov 16, 2020

View reviewed changes

marandaneto and others added 5 commits November 16, 2020 11:32

Merge branch 'main' into feat/module_loader

af36362

Update sentry-android-ndk/src/test/java/io/sentry/android/ndk/DebugIm…

4d8729a

…agesLoaderTest.kt Co-authored-by: Philipp Hofmann <[email protected]>

review

f36de25

Merge branch 'feat/module_loader' of github.com:getsentry/sentry-java…

dcbe2ca

… into feat/module_loader

review

b923131

marandaneto requested a review from philipphofmann November 16, 2020 11:24

marandaneto added 2 commits November 16, 2020 12:25

fix changelog

c9ae3d3

fix changelog

99d6346

philipphofmann reviewed Nov 16, 2020

View reviewed changes

review

a1709ce

philipphofmann reviewed Nov 16, 2020

View reviewed changes

sentry-android-ndk/src/main/java/io/sentry/android/ndk/NativeModuleListLoader.java Show resolved Hide resolved

sentry-android-ndk/src/main/java/io/sentry/android/ndk/IModuleListLoader.java Outdated Show resolved Hide resolved

philipphofmann reviewed Nov 16, 2020

View reviewed changes

set ndk module testable by mockito inline

af6f4d7

marandaneto requested a review from philipphofmann November 17, 2020 07:51

philipphofmann approved these changes Nov 17, 2020

View reviewed changes

bruno-garcia reviewed Nov 17, 2020

View reviewed changes

Swatinem approved these changes Nov 18, 2020

View reviewed changes

fix conflict

3ad3523

marandaneto merged commit b98169a into main Nov 18, 2020

marandaneto deleted the feat/module_loader branch November 18, 2020 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose a module loader for Android thru sentry-native #1043

feat: expose a module loader for Android thru sentry-native #1043

marandaneto commented Nov 13, 2020 •

edited

Loading

codecov-io commented Nov 13, 2020 •

edited

Loading

Swatinem left a comment

philipphofmann left a comment

philipphofmann left a comment

philipphofmann left a comment

marandaneto commented Nov 16, 2020

philipphofmann left a comment

bruno-garcia left a comment

bruno-garcia Nov 17, 2020

marandaneto Nov 17, 2020

bruno-garcia Nov 17, 2020

marandaneto Nov 17, 2020

bruno-garcia Nov 17, 2020

marandaneto Nov 17, 2020

bruno-garcia Nov 17, 2020

marandaneto Nov 17, 2020

Swatinem Nov 18, 2020

marandaneto commented Nov 17, 2020

Swatinem Nov 18, 2020

	sentry_value_t image_list_t = sentry_get_modules_list();
	sentry_value_t image_list = sentry_get_modules_list();

	size_t len_t = sentry_value_get_length(image_list_t);
	size_t len = sentry_value_get_length(image_list_t);

feat: expose a module loader for Android thru sentry-native #1043

feat: expose a module loader for Android thru sentry-native #1043

Conversation

marandaneto commented Nov 13, 2020 • edited Loading

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

codecov-io commented Nov 13, 2020 • edited Loading

Codecov Report

Swatinem left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

marandaneto commented Nov 16, 2020

philipphofmann left a comment

Choose a reason for hiding this comment

bruno-garcia left a 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

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

Choose a reason for hiding this comment

marandaneto commented Nov 17, 2020

Choose a reason for hiding this comment

marandaneto commented Nov 13, 2020 •

edited

Loading

codecov-io commented Nov 13, 2020 •

edited

Loading