-
Notifications
You must be signed in to change notification settings - Fork 9
[Android] Fix JankStats memory leak due to picking Dialog window #705
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
Conversation
67f464d to
7a69e4e
Compare
Size Comparison Report (x86_64)
|
…m/bitdriftlabs/capture-sdk into franjam/prevent-dialog-window-fix
0f16f96 to
44160e2
Compare
| setJankStatsForCurrentWindow(it.window) | ||
| } | ||
| // We are done detecting initial Application ON_CREATE, we don't need to listen anymore | ||
| processLifecycleOwner.lifecycle.removeObserver(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also minor change to make sure we always remove the observer upon initial ON_CREATE signal
| <?xml version="1.0" encoding="utf-8"?> | ||
| <resources> | ||
| <bool name="leak_canary_watcher_watch_dismissed_dialogs">true</bool> | ||
| </resources> No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this leftover from testing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally yes, but given gradle-test-app is small it would be nice to have more signal for these type of leaks moving forward. If becomes too noisy for future use case in test app we can disable the flag
| override fun findFirstValidActivity(): Activity? = | ||
| getAllRootViews().firstNotNullOfOrNull { view -> | ||
| val activity = view.unwrapToActivity() | ||
| //noinspection NewApi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because the min API level for activity.isDestroyed is API 17, but the common module doesn't specify a min SDK in its build.gradle and it infers as API level 1 so the warning is shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like the better fix wouldl be to make our modules all match in terms if min API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup I agree, updated here 5364cf012e6b7064fad6d634b399292f058ee010 following other modules
| minSdk = 23 |
Resolves BIT-6673
Big shout out to @pyricau for the help with the investigation, repro steps and suggestions for fix!
What
Fixes a memory leak in JankStats monitoring that can occur when the SDK is initialized after UI is already displayed and there are multiple root views present (including dialogs). The SDK could attach to a dialog window instead of an activity window, causing the JankStats instance to leak when the dialog is dismissed.
The solution for now will be to pick the first valid activity window instead (as a follow up will explore into updating the product to track multiple windows)
Below the original leaktrace report
Verification
Test steps
e.g. logs
Follow-up
BIT-6709. We'll explore updating the product to track multiple windows