Ensure Hints do not cause memory leaks

[markushi]

📜 Description

Clear any extra hint attributes before passing them to the transport layer.

💡 Motivation and Context

Hint objects are only freed after network transfer is completed. Thus this may causes memory leaks whenever a hint contains references to an activity/fragment/view.

Options considered:

• Use WeakReference<Object> for hint values: probably a bad idea because calling hint.set("key", new Value()) gives you no guarantee that the value will be present at a later stage / could be gc'd at any time
• Use existing API, pass all relevant values as a WeakReference directly (e.g. hint.set("activity", new WeakReference(activity))) and unwrap the WeakReference in get() automatically: Would work nicely but breaks the API for anyone already using WeakReference as we now would suddenly auto-unwrap it.

💚 How did you test it?

Added unit test

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• No breaking changes

🔮 Next steps

[markushi]

@romtsn I'm not very happy with the solution, as the presence of the activity/fragment/view hint value is not guaranteed anymore. Would it be feasible to delete the whole hint earlier and not wait until network request is done?

[adinauer]

breaks the API for anyone already using WeakReference as we now would suddenly auto-unwrap it.

Isn't this also true for the current implementation? It doesn't matter whether it was us who set the WeakReference or the user. In get() it'll be unwrapped either way.

If we want to avoid this, I see three ways atm:

• Store a wrapper object that tells us whether it was us who used the weak ref
• Store weak refs set by us in a different map
• Tell users to use getAs for this use case and then we don't use get() internally but try to keep WeakReference if that's what the type passed to getAs asks for

presence of the activity/fragment/view hint value is not guaranteed anymore

API always said @Nullable but users may have tested and found they never receive null where they now will if we go ahead with this change. I guess not bloating memory is more important here.

Would it be feasible to delete the whole hint earlier and not wait until network request is done?

Maybe we could only remove stuff we would mark as weak early?

[romtsn]

@romtsn I'm not very happy with the solution, as the presence of the activity/fragment/view hint value is not guaranteed anymore. Would it be feasible to delete the whole hint earlier and not wait until network request is done?

I think we can't delete the hint entirely, because we check if it's Retryable, etc. But I think we could drop the specific keys (like activity or view) after beforeSend is executed

[adinauer]

How about we wrap stuff in the internal storage with some object that allows us to mark things as weak without actually using WeakReference and then have a method on Hint to remove weak stuff after beforeSend was executed?

How does this work for things that are not errors (e.g. transactions)? Do we also attach things that should be weakly referenced there? Then we would have to make sure everything is cleaned up which is more risky than using WeakReference

[romtsn]

How does this work for things that are not errors (e.g. transactions)? Do we also attach things that should be weakly referenced there? Then we would have to make sure everything is cleaned up which is more risky than using WeakReference

Yes, we also send breadcrumbs with transactions, so they can leak stuff. I guess we can just drop the keys inside transport.send, because that's called always, regardless of the envelope type

[adinauer]

I assume hints for beforeBreadcrumb are fine because we don't store them. Seems like the only problematic hints are those passed to the transport so clearing in transport sounds like a good approach to me.

[markushi]

After having a closer look with @romtsn the issue is less relevant as initially thought:

• Affected Hints for breadcrumbs are processed right after adding the breadcrumb and are not passed to the transport layer
• For events: the transport layer does only use the hint class/sentry-internal attributes, thus any extra attributes could be safely cleared to avoid any leak

Let me adopt the PR accordingly

[github-actions]

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	328.04 ms	435.34 ms	107.30 ms
Size	1.73 MiB	2.32 MiB	611.97 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
507f924	342.51 ms	402.65 ms	60.14 ms
b85d8aa	289.35 ms	335.92 ms	46.56 ms
4a9c176	320.62 ms	334.68 ms	14.06 ms
7597ded	289.60 ms	339.69 ms	50.09 ms
3695453	299.25 ms	360.04 ms	60.79 ms
f809aac	301.51 ms	346.60 ms	45.09 ms
3695453	314.63 ms	353.10 ms	38.47 ms
3695453	301.78 ms	371.14 ms	69.36 ms
4a9c176	319.77 ms	363.20 ms	43.43 ms
90e9745	314.68 ms	357.28 ms	42.60 ms

App size

Revision	Plain	With Sentry	Diff
507f924	1.73 MiB	2.32 MiB	609.95 KiB
b85d8aa	1.73 MiB	2.32 MiB	611.62 KiB
4a9c176	1.73 MiB	2.33 MiB	612.69 KiB
7597ded	1.73 MiB	2.32 MiB	609.88 KiB
3695453	1.73 MiB	2.32 MiB	611.62 KiB
f809aac	1.73 MiB	2.32 MiB	608.63 KiB
3695453	1.73 MiB	2.32 MiB	611.62 KiB
3695453	1.73 MiB	2.32 MiB	611.62 KiB
4a9c176	1.73 MiB	2.33 MiB	612.69 KiB
90e9745	1.73 MiB	2.32 MiB	608.63 KiB

Previous results on branch: fix/hint-potential-memory-leaks

Startup times

Revision	Plain	With Sentry	Diff
ece9431	307.92 ms	341.24 ms	33.32 ms
c365eee	360.24 ms	386.17 ms	25.93 ms
51f3af9	311.71 ms	335.52 ms	23.81 ms
1b771a8	346.04 ms	373.74 ms	27.70 ms
9553ba1	272.67 ms	332.36 ms	59.70 ms

App size

Revision	Plain	With Sentry	Diff
ece9431	1.73 MiB	2.32 MiB	611.97 KiB
c365eee	1.73 MiB	2.32 MiB	611.64 KiB
51f3af9	1.73 MiB	2.32 MiB	611.74 KiB
1b771a8	1.73 MiB	2.32 MiB	611.65 KiB
9553ba1	1.73 MiB	2.32 MiB	611.64 KiB

[codecov-commenter]

Codecov Report

Base: 80.28% // Head: 80.29% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (f4c8de1) compared to base (b39d6a0).
Patch coverage: 90.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #2387   +/-   ##
=========================================
  Coverage     80.28%   80.29%           
- Complexity     3695     3698    +3     
=========================================
  Files           292      292           
  Lines         13890    13900   +10     
  Branches       1839     1841    +2     
=========================================
+ Hits          11152    11161    +9     
  Misses         2021     2021           
- Partials        717      718    +1     

Impacted Files	Coverage Δ
sentry/src/main/java/io/sentry/Hint.java	92.98% <85.71%> (-1.02%)	⬇️
sentry/src/main/java/io/sentry/SentryClient.java	86.12% <100.00%> (+0.10%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.