Fix memory leak issue in SentryFlutterPlugin

[linkaipeng]

I fixed the memory leak issue in SentryFlutterPlugin by removing the anonymous inner class that was holding a reference to the external SentryFlutterPlugin object, which was causing a memory leak problem with the MethodChannel object.

📜 Description

💡 Motivation and Context

To reproduce the steps:

• Integrate sentry_flutter into your project.
• Open the application's MainActivity and press the back button to return to the Launcher. Repeat this process multiple times.
• Use Android Studio Profiler to dump the memory for analysis.

💚 How did you test it?

Since the SentryFlutterPlugin does not have unit test code, I repeated the steps mentioned above after making the modifications and confirmed that the issue has been fixed.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
• I updated the docs if needed
• All tests passing
• No breaking changes

🔮 Next steps

[marandaneto]

Since the SentryFlutterPlugin does not have unit test code

We are currently adding tests #1587

[marandaneto]

@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.

[romtsn]

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

[marandaneto]

LocalizationPlugin

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

[romtsn]

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]

@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.

[marandaneto]

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.

[marandaneto]

@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]

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%.

Additional details and impacted files

@@            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     

see 8 files with indirect coverage changes

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

[linkaipeng]

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.

[marandaneto]

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)

[linkaipeng]

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.