Skip to content
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

Fix LeakCanary startup in debug builds and fix a memory leak #10232

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Jul 14, 2023

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • According to the stacktrace provided in Update LeakCanary library #10085 (comment), LeakCanary now takes care to install the AppWatcher by itself, so we don't need to set it up manually, so I just removed the incriminating line. Also see Fix LeakCanary crash #10231 and this changelog.
  • I have tested by enabling LeakCanary in the settings and capturing a few leaks. Among those leaks I then found a really suspicious PlayerService-related leak and I decided to fix it, for more info see https://stackoverflow.com/q/63787707 and the leak report below. You can verify that this was fixed by opening the player, closing it and then taking a heap dump from LeakCanary. Note that these steps did not always lead to the leak before, idk why. Remember to close the app after you enable LeakCanary in debug settings!
  • I also set all LeakCanary dependencies as debugImplementation only, as it does not make sense to include those in release builds. Previously they were included just because BaseFragment.onDestroy() called AppWatcher.expectWeaklyReachable(this), but I don't think we need to do it manually, since LeakCanary already takes care of detecting fragments being destroyed. After removing this manual call, I still saw some leaks being detected because "[...] MainFragment received onDestroyView", so I guess the automatic detection works. The release builds fine, here is a release APK signed by me:
The leak report I fixed

┬───
│ GC Root: Global variable in native code

├─ org.schabi.newpipe.player.PlayerService$LocalBinder instance
│ Leaking: UNKNOWN
│ Retaining 6.8 kB in 103 objects
│ this$0 instance of org.schabi.newpipe.player.PlayerService
│ ↓ PlayerService$LocalBinder.this$0
│ ~~~~~~
╰→ org.schabi.newpipe.player.PlayerService instance
​ Leaking: YES (ObjectWatcher was watching this because org.schabi.newpipe.
​ player.PlayerService received Service#onDestroy() callback and Service
​ not held by ActivityThread)
​ Retaining 6.3 kB in 102 objects
​ key = cd33af62-1bd6-4297-bc44-2f05fab9fa16
​ watchDurationMillis = 19136
​ retainedDurationMillis = 14120
​ mApplication instance of org.schabi.newpipe.DebugApp
​ mBase instance of org.schabi.newpipe.player.AudioServiceLeakFix

METADATA

Build.VERSION.SDK_INT: 33
Build.MANUFACTURER: Google
LeakCanary version: 2.12
App process name: org.schabi.newpipe.debug
Class count: 30345
Instance count: 297963
Primitive array count: 185241
Object array count: 45771
Thread count: 74
Heap total bytes: 40216348
Bitmap count: 55
Bitmap total bytes: 10275621
Large bitmap count: 0
Large bitmap total bytes: 0
Db 1: open /data/user/0/org.schabi.newpipe.debug/databases/newpipe.db
Db 2: open /data/user/0/org.schabi.newpipe.debug/databases/exoplayer_internal.db
Db 3: open /data/user/0/org.schabi.newpipe.debug/no_backup/androidx.work.workdb
Stats: LruCache[maxSize=3000,hits=136480,misses=286718,hitRate=32%]
RandomAccess[bytes=15166205,reads=286718,travel=153801922092,range=44549734,size
=57029441]
Analysis duration: 24471 ms

Fixes the following issue(s)

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.

Due diligence

@Stypox Stypox mentioned this pull request Jul 14, 2023
5 tasks
@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@AudricV AudricV added bug Issue is related to a bug player Issues related to any player (main, popup and background) codequality Improvements to the codebase to improve the code quality labels Jul 14, 2023
@TobiGr TobiGr added the dependency Issues and PRs related to dependencies label Jul 16, 2023
@TacoTheDank
Copy link
Member

@TobiGr Oh yes, thanks for creating a label for dependency stuff. That was something I had in mind a couple of times, but never got around to.

@TobiGr TobiGr merged commit 2f0ed7f into TeamNewPipe:dev Jul 17, 2023
4 of 5 checks passed
@AudricV AudricV mentioned this pull request Jul 30, 2023
5 tasks
This was referenced Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality dependency Issues and PRs related to dependencies player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants