Since the SentryFlutterPlugin does not have unit test code

We are currently adding tests #1587

@linkaipeng thanks for the PR.

I'm not sure the suggested changes will have any effect, the leak seems to be in https://github.com/getsentry/sentry-java/blob/549cbb4657b9dea1048c4882e79504e87d6acc4f/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L26 (based on the SS)

The changes with the beforeSend callback are not holding any reference, just modifying the metadata.

Can you elaborate on what exactly is causing this, which line, etc?

@romtsn or @markushi can chime in as well since they own this part of the code.

What is LocalizationPlugin? Is this something from Flutter/Dart or custom?

LocalizationPlugin

Probably https://github.com/flutter/engine/blob/146c4c9487fc68ff97dbf900bf3c760c60ed251a/shell/platform/android/io/flutter/plugin/localization/LocalizationPlugin.java

Hm, found this, looks very similar ryanheise/audio_session#12. Should we nullify the channel in onDetachedFromEngine? This was the fix from that repo ryanheise/audio_session@f9f114a

@linkaipeng thanks for the PR.

I'm not sure the suggested changes will have any effect, the leak seems to be in https://github.com/getsentry/sentry-java/blob/549cbb4657b9dea1048c4882e79504e87d6acc4f/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java#L26 (based on the SS)

The changes with the beforeSend callback are not holding any reference, just modifying the metadata.

Can you elaborate on what exactly is causing this, which line, etc?

@romtsn or @markushi can chime in as well since they own this part of the code.

@marandaneto Thank you for your reply. The reason is that the BeforeSendCallback instance passed to beforeSend is an anonymous inner class, and through the bytecode, it is clear that it holds a reference to the SentryFlutterPlugin object, which is the root cause of the leak.

The approach I took was to completely decouple the BeforeSendCallback instance from SentryFlutterPlugin and remove its reference to the SentryFlutterPlugin object.

Before modification:

After modification:

Another way to modify is to make the channel nullable and set it to null in the onDetachedFromEngine method. However, in my understanding, the purpose of BeforeSendCallback is to modify the metadata of the SentryEvent Methods like setEventOriginTag and addPackages seem more like utility methods and should not belong to the SentryFlutterPlugin. Therefore, changing them to static methods would be more appropriate.

Oh I see, that makes sense @linkaipeng thanks for the explanation.
There are probably some linting issues, I will run CI, also please add a new entry to the changelog.

@krystofwoldrich this is probably an issue on RN as well https://github.com/getsentry/sentry-react-native/blob/4232fae5aa22cb7e1bd01576ef9b154f00864ed5/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java#L189

Codecov Report

Patch coverage has no change and project coverage change: +0.23% 🎉

Comparison is base (07d9b4d) 88.44% compared to head (896dad3) 88.68%.

@@            Coverage Diff             @@
##             main    #1588      +/-   ##
==========================================
+ Coverage   88.44%   88.68%   +0.23%     
==========================================
  Files         132      124       -8     
  Lines        4274     3940     -334     
==========================================
- Hits         3780     3494     -286     
+ Misses        494      446      -48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Oh I see, that makes sense @linkaipeng thanks for the explanation. There are probably some linting issues, I will run CI, also please add a new entry to the changelog.

Thank you. The changelog has been committed.

Oh I see, that makes sense @linkaipeng thanks for the explanation. There are probably some linting issues, I will run CI, also please add a new entry to the changelog.

Thank you. The changelog has been committed.

the lining issues:

ktlint version: 0.50.0
reviewdog: This GitHub token doesn't have write permission of Review API [1],
so reviewdog will report results via logging command [2] and create annotations similar to
github-pr-check reporter as a fallback.
[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target,
[2]: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/development-tools-for-github-actions#logging-commands
reviewdog: input data has violations
Error: [ktlint] reported by reviewdog 🐶
Unnecessary trailing comma before ")"

Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:211:37: error: Unnecessary trailing comma before ")" (standard:trailing-comma-on-call-site)
Error: [ktlint] reported by reviewdog 🐶
Unnecessary trailing comma before ")"

Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:252:33: error: Unnecessary trailing comma before ")" (standard:trailing-comma-on-call-site)
Error: [ktlint] reported by reviewdog 🐶
Exceeded max line length (100)

Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:403:1: error: Exceeded max line length (100) (standard:max-line-length)
Error: [ktlint] reported by reviewdog 🐶
Exceeded max line length (100)

Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:427:1: error: Exceeded max line length (100) (standard:max-line-length)

Oh I see, that makes sense @linkaipeng thanks for the explanation. There are probably some linting issues, I will run CI, also please add a new entry to the changelog.

Thank you. The changelog has been committed.

the lining issues:

ktlint version: 0.50.0
reviewdog: This GitHub token doesn't have write permission of Review API [1],
so reviewdog will report results via logging command [2] and create annotations similar to
github-pr-check reporter as a fallback.
[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target,
[2]: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/development-tools-for-github-actions#logging-commands
reviewdog: input data has violations
Error: [ktlint] reported by reviewdog 🐶
Unnecessary trailing comma before ")"
Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:211:37: error: Unnecessary trailing comma before ")" (standard:trailing-comma-on-call-site)
Error: [ktlint] reported by reviewdog 🐶
Unnecessary trailing comma before ")"
Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:252:33: error: Unnecessary trailing comma before ")" (standard:trailing-comma-on-call-site)
Error: [ktlint] reported by reviewdog 🐶
Exceeded max line length (100)
Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:403:1: error: Exceeded max line length (100) (standard:max-line-length)
Error: [ktlint] reported by reviewdog 🐶
Exceeded max line length (100)
Raw Output:
flutter/android/src/main/kotlin/io/sentry/flutter/SentryFlutterPlugin.kt:427:1: error: Exceeded max line length (100) (standard:max-line-length)

Fixed.