1.0 RC2 AndroidScreenLifecycleOwner memory leak

[adrielcafe]

1.0 RC2 AndroidScreenLifecycleOwner memory leak

[Tolriq]

Modules are split by transitive deps.

Bottomsheet embed Material so should be a module.
Transition embed Animation so should be a module.

I'm using hilt.

For lifecycle https://medium.com/androiddevelopers/consuming-flows-safely-in-jetpack-compose-cde014d0d5a3 there's quite a few things that require proper handling.

With all that said, I do not really have time to test new library so, do not do all the changes for me :)

[Tolriq]

PIng @programadorthi since you introduced the atomicContext

[adrielcafe]

I'm rethinking all the Android integration. My initial idea was to implement my own LifecycleOwner, ViewModelStoreOwner and SavedStateRegistryOwner, but there's always something missing.

I'm trying to integrate directly with the platform implementation, this should finally solve the issues.

[Tolriq]

Platform is not always best :) But do not hesitate if you need things to be tested, hope you stay API compatible I'm in prod now, thanks again for the library.

[Anton111111]

@adrielcafe , do you have any thinks when it will be done?

[beyondeye]

Hi all, I have just published a fork of voyager with bug fixes and new features here.
I have changed the way ViewModel and Lifecycle are managed by voyager and I think that this specific bug has been solved in my fork
I will be happy to have your feedback.

[Tolriq]

The library is split in modules to avoid unwanted transient dependencies.
DI is kinda mandatory these days.
Lifecycle integration is also mandatory to allow composable to react to it and avoid unwanted flow collection for example.

I'm sorry but I'm not sure that your fixes are going in the proper direction for the current state of this library.

With that said, it might be a valid fork with a different purpose and targe, it's just not bug fixes and new features.

[beyondeye]

@Tolriq thanks for you feedback. So if I am understanding correctly, what you suggest is

• add back modules for DI integration (which one are you using specifically?)
• Proper lifecycle events integration: in addition to stop flow collection, you need lifecycle integration for something else?
• about splitting the library, a split like the following would make sense to you?

1. voyager-navigator
2. voyager-bottom-sheet-navigator voyager-tab-navigator voyager-transitions
3. voyager-androidx
4. voyager-livedata
5. voyager-rxjava
6. voyager-koin
7. voyager-kodein
8. voyager-hilt

[beyondeye]

@Tolriq Thank you again for your comments they were actually very useful. I have now released 0.10.0.
I have adopted all you suggestions

[Syer10]

@Tolriq Are you able to check if this happens in my fork? https://github.com/Syer10/voyager/

Not sure if this still would since I changed stuff around and the onDispose function should fire correctly now.

[Tolriq]

This is probably not fixed as a special use cases with tabs and not enabling stepdispose in that case, this require manual dispose in that case that I do in my fork (but could do with yours too with dispose public)

[Syer10]

Oh, well that is something I fixed. I added a TabDisposable for cleaning up tab resources. https://github.com/Syer10/voyager/blob/main/samples/android/src/main/java/cafe/adriel/voyager/sample/tabNavigation/TabNavigationActivity.kt#L38

[OKatrych]

That's still an issue with the latest RC version:

====================================
HEAP ANALYSIS RESULT
====================================
1 APPLICATION LEAKS

References underlined with "~~~" are likely causes.
Learn more at https://squ.re/leaks.

1336761 bytes retained by leaking objects
Signature: 889d51009c2d5ef240ef4f461f2e12cfb3c5607c
┬───
│ GC Root: Thread object
│
├─ android.net.ConnectivityThread instance
│    Leaking: NO (PathClassLoader↓ is not leaking)
│    Thread name: 'ConnectivityThread'
│    ↓ Thread.contextClassLoader
├─ dalvik.system.PathClassLoader instance
│    Leaking: NO (ScreenLifecycleStore↓ is not leaking and A ClassLoader is never leaking)
│    ↓ ClassLoader.runtimeInternalObjects
├─ java.lang.Object[] array
│    Leaking: NO (ScreenLifecycleStore↓ is not leaking)
│    ↓ Object[2065]
├─ cafe.adriel.voyager.core.lifecycle.ScreenLifecycleStore class
│    Leaking: NO (a class is never leaking)
│    ↓ static ScreenLifecycleStore.newOwners
│                                  ~~~~~~~~~
├─ cafe.adriel.voyager.core.concurrent.ThreadSafeMap instance
│    Leaking: UNKNOWN
│    Retaining 1,3 MB in 27203 objects
│    ↓ ThreadSafeMap.$$delegate_0
│                    ~~~~~~~~~~~~
├─ java.util.concurrent.ConcurrentHashMap instance
│    Leaking: UNKNOWN
│    Retaining 1,3 MB in 27202 objects
│    ↓ ConcurrentHashMap["dev.olek.p2pview.ui.rates.RatesTab"]
│                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
├─ cafe.adriel.voyager.core.concurrent.ThreadSafeMap instance
│    Leaking: UNKNOWN
│    Retaining 196 B in 6 objects
│    ↓ ThreadSafeMap.$$delegate_0
│                    ~~~~~~~~~~~~
├─ java.util.concurrent.ConcurrentHashMap instance
│    Leaking: UNKNOWN
│    Retaining 184 B in 5 objects
│    ↓ ConcurrentHashMap[instance @331514184 of kotlin.jvm.internal.TypeReference]
│                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
├─ cafe.adriel.voyager.androidx.AndroidScreenLifecycleOwner instance
│    Leaking: UNKNOWN
│    Retaining 238 B in 8 objects
│    ↓ AndroidScreenLifecycleOwner.atomicContext
│                                  ~~~~~~~~~~~~~
├─ java.util.concurrent.atomic.AtomicReference instance
│    Leaking: UNKNOWN
│    Retaining 12 B in 1 objects
│    value instance of dev.olek.p2pview.ui.MainActivity with mDestroyed = true
│    ↓ AtomicReference.value
│                      ~~~~~
╰→ dev.olek.p2pview.ui.MainActivity instance
​     Leaking: YES (ObjectWatcher was watching this because dev.olek.p2pview.ui.MainActivity received
​     Activity#onDestroy() callback and Activity#mDestroyed is true)
​     Retaining 1,3 MB in 27160 objects
​     key = 915a1a29-b59a-48a5-bfe0-6f996681f7c9
​     watchDurationMillis = 51410
​     retainedDurationMillis = 46410
​     mApplication instance of dev.olek.p2pview.app.App
​     mBase instance of androidx.appcompat.view.ContextThemeWrapper
====================================
0 LIBRARY LEAKS

A Library Leak is a leak caused by a known bug in 3rd party code that you do not have control over.
See https://square.github.io/leakcanary/fundamentals-how-leakcanary-works/#4-categorizing-leaks
====================================
0 UNREACHABLE OBJECTS

An unreachable object is still in memory but LeakCanary could not find a strong reference path
from GC roots.
====================================
METADATA

Please include this in bug reports and Stack Overflow questions.

Build.VERSION.SDK_INT: 33
Build.MANUFACTURER: Google
LeakCanary version: 2.10
App process name: dev.olek.p2pview
Class count: 34976
Instance count: 303056
Primitive array count: 164533
Object array count: 45554
Thread count: 99
Heap total bytes: 37079810
Bitmap count: 15
Bitmap total bytes: 423375
Large bitmap count: 0
Large bitmap total bytes: 0
Db 1: closed /data/user/0/dev.olek.p2pview/databases/google_app_measurement_local.db
Db 2: open /data/user/0/dev.olek.p2pview/no_backup/androidx.work.workdb
Db 3: open /data/user/0/dev.olek.p2pview/databases/ibg_diagnostics.db
Db 4: open /data/user/0/dev.olek.p2pview/databases/instabug.db
Db 5: open /data/user/0/dev.olek.p2pview/databases/instabug_encrypted.db
Stats: LruCache[maxSize=3000,hits=138386,misses=303319,hitRate=31%]
RandomAccess[bytes=15429222,reads=303319,travel=144891537106,range=44205015,size=54036495]
Heap dump reason: user request
Analysis duration: 15042 ms
Heap dump file path: /storage/emulated/0/Download/leakcanary-dev.olek.p2pview/2023-05-09_23-45-34_650.hprof
Heap dump timestamp: 1683668753827
Heap dump duration: 1783 ms
====================================