[SES-1936] Fix memory leaks #1493

simophin · 2024-05-21T01:46:04Z

Description

There are a lot of memory leaks in the app, primarily around ConversationActivityV2, basically every conversation page you open it never gets GCed. And soon enough you'll run out of memory.

This is due to a lot of misuse of context.

This PR tries to address these misuses.

simophin · 2024-05-21T01:47:33Z

libsession/src/main/java/org/session/libsession/avatars/PlaceholderAvatarPhoto.kt

 import com.bumptech.glide.load.Key
 import java.security.MessageDigest

-class PlaceholderAvatarPhoto(val context: Context,


Key is a used to cache an image for Glide, context should not be part of an image Key (it doesn't make sense anyway), and the cache key is stored in a global object.

simophin · 2024-05-21T01:49:11Z

app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt

                performAnimation()
            }, dotAnimationStartDelay)
        }

        private fun performAnimation() {
-            expand()


performAnimation is a "recursive" call, basically keep running in the main thread. This "loop" is stored globally so it never gets cleaned up when the view disappears. Here a circuit breaker is introduced so that when the view goes out of windows the animation stops.

simophin · 2024-05-21T01:50:18Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt

+        for (job in updateJobs) {
+            job.cancel()
+        }
+        updateJobs.clear()
+    }


The updateJobs keep activity context in a globally referenced memory so they have to be cleaned up when viewModel destroys. There's code smells here in terms of how the LiveData is used but that's another story.

Is this necessary?

Jobs are run in this scope: executor = viewModelScope + SupervisorJob()

so I would've thought they'd be canceled when the VM scope is over.

You are right, I didn't see executor comes from a viewModelScope, I sort of assumed it was one of those Java ExecutorService I saw somewhere else.

It is interesting it shows up in the memory reference graph though so I took a closer look at the implementation of ContentResolver.observeQuery, which comes from a third party lib: app.cash.copper.flow: The snippet of that piece of code is:

@CheckResult fun ContentResolver.observeQuery( uri: Uri, projection: Array<String>? = null, selection: String? = null, selectionArgs: Array<String>? = null, sortOrder: String? = null, notifyForDescendants: Boolean = false ): Flow<Query> { val query = ContentResolverQuery(this, uri, projection, selection, selectionArgs, sortOrder) return flow { emit(query) val channel = Channel<Unit>(CONFLATED) val observer = object : ContentObserver(mainThread) { override fun onChange(selfChange: Boolean) { channel.offer(Unit) } } registerContentObserver(uri, notifyForDescendants, observer) try { for (item in channel) { emit(query) } } finally { unregisterContentObserver(observer) } } }

I believe the clean up code is not set up correctly, as unregisterContentObserver is never called when debugging the code. It surprises me that it's not called but if I have to DIY I'd use callbackFlow so that I can register a cancel hook to proper clean up the observer.

Let me come up with something

simophin · 2024-05-21T01:53:19Z

There's one instance of leak left that is AudioSlidePlayer, who keeps the context in a global reference. It will leak up to one ConversationActivity I haven't got to check if changing to application context will break the player, will probably include it in a separate PR.

bemusementpark

Looks pretty reasonable, maybe double check the animation stuff?

bemusementpark · 2024-05-21T02:58:45Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/input_bar/InputBarRecordingView.kt

+        if (isAttachedToWindow) {
+            // Should only update the timer if the view is still attached to the window.
+            // Otherwise, the timer will keep running even after the view is detached.
+            snHandler.postDelayed({ updateTimer() }, 500)


do we need to restart if it's reattached?

No we don't, I did check it still works. Originally I had the same thought as I didn't know whether putting a view into background will count as a detach but it seems like the window still exists in the background.

Just for a fun reference, this is an answer I got from chatgpt:

No, putting an Android activity into the background does not destroy its window. The activity's window is only destroyed when the activity is finished (i.e., its finish() method is called) or the system needs to reclaim memory and decides to kill the activity's process. When an activity goes into the background, it is in the stopped state but its state is preserved. This means that the system maintains all view hierarchy and data associated with the activity. The activity's window is hidden, but all of its state such as member variables is preserved. However, it's important to note that when the system is low on memory, it may decide to kill background activities to reclaim resources. In this case, the activity's window and state would be destroyed. The system would later recreate the activity if it becomes visible again, calling onCreate() with a non-null Bundle that has the saved state.

ok, still a little brittle to stop on remove, and not start again if a dev was

There's another issue:

ConActivityV2#showVoiceMessageUI calls InputBarRecordingView#show calls updateTimer()

So every time we start a new voice recording we add another one of these async-recursive calls.

Maybe we can address that too?

Hmm ok I can try to address the "what if the dev wants to re-use the view but keep animation running" scenario, I was been lazy.

I'm not sure about your second question, updateTimer doesn't seem to "add" anything?

updateTimer posts a delayed call to updateTimer

Ah I see what you mean: a duplicated call into show will have started another timer, which will do nothing but increase the frequency of timer update?

Yeah, every time you record a message, we'll create a new "async-recursive" loop.

AL-Session

All looking good to me! Approved.

simophin · 2024-05-22T00:55:49Z

@bemusementpark Please have a look again with the latest change. I've also added the missing animation resume on PathActivity

bemusementpark · 2024-05-22T01:01:04Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/input_bar/InputBarRecordingView.kt

+
+        if (isAttachedToWindow) {
+            // Make sure there's only one runnable in the handler at a time.
+            removeCallbacks(updateTimerRunnable)


nice, though maybe we should stop the callback when we stop recording, as right now the callbacks will not stop when you finish a recording.

You could make it fun updateTimer(scope) and pass in lifecycleScope and stop when the activity is destroyed for free.

Make some stop() that cancels the job.

Then we can just remove the onAttach stuff.

bemusementpark · 2024-05-23T00:19:47Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt

+        for (job in updateJobs) {
+            job.cancel()
+        }
+        updateJobs.clear()
+    }


Is this necessary?

Jobs are run in this scope: executor = viewModelScope + SupervisorJob()

so I would've thought they'd be canceled when the VM scope is over.

simophin · 2024-05-23T03:52:00Z

Hey @bemusementpark @AL-Session , apologies I needed to make additional changes to this PR:

Tidy up the usage of Kotlin Flow in HomeViewModel, now it should be much cleaner on what it's trying to do.
Adds a ContentResolver.observeChanges which unlike app.cash.copper.flow.observeQuery, it cleans up properly
Another leaking place comes from MnemonicCodec.Language where it stores a lambda into a global cache :-(

simophin · 2024-05-23T03:56:24Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeActivity.kt

-        val broadcastReceiver = object : BroadcastReceiver() {
-            override fun onReceive(context: Context, intent: Intent) {
-                binding.recyclerView.adapter!!.notifyDataSetChanged()
-            }
-        }
-        this.broadcastReceiver = broadcastReceiver
-        LocalBroadcastManager.getInstance(this).registerReceiver(broadcastReceiver, IntentFilter("blockedContactsChanged"))


This piece of code is useless: it doesn't update any data but asking RecyclerView to refresh itself completely on the same data. The only two data source for the home adapter is List<ThreadRecord> and Set<TypingID>, which are totally immutable: trying to reload the immutable data will always result in the same UI representation.

The "blocked contact" will actually be updated through the change to the thread itself. I have tested and it still works without this code here.

yeah, it looked like there's no other usage of "blockedContactsChanged"

simophin · 2024-05-23T04:18:30Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeActivity.kt

-        ApplicationContext.getInstance(this@HomeActivity).typingStatusRepository.typingThreads.observe(this) { threadIds ->
-            homeAdapter.typingThreadIDs = (threadIds ?: setOf())
-        }


I have moved this change detection into HomeViewModel so we have a uniform data coming from the VM instead.

simophin · 2024-05-23T04:19:06Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeDiffUtil.kt

This diff util now takes the "typing status" into account so we don't need to refresh the whole list when one of the thread status changes.

bemusementpark · 2024-05-23T04:18:25Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt

@@ -287,8 +287,10 @@ class ConversationActivityV2 : PassphraseRequiredActionBarActivity(), InputBarDe
        if (hexEncodedSeed == null) {
            hexEncodedSeed = IdentityKeyUtil.getIdentityKeyPair(this).hexEncodedPrivateKey // Legacy account
        }
+
+        val appContext = applicationContext


remove and inline?

This actually can't be inlined. As the syntax val appContext = applicationContext is equivalent to val appContext = this.getApplicationContext(), if you move applicationContext into the lambda, you move this.getApplicationContext() there hence moving this into the lambda, where this is where the memory leak comes from

looks like loadFileContents is used once, synchronously in the next line, so Activity Context was fine.

If you look at MnemonicCodec.encode, it creates a new Language and calls Language.loadWordSet, where Language instance is stored in static cache, in whichloadFileContents is a field of.

One of those dodgy "Putting everything you can think of into cache" scenarios

app/src/main/java/org/thoughtcrime/securesms/database/ThreadDatabase.java

app/src/main/java/org/thoughtcrime/securesms/home/HomeAdapter.kt

app/src/main/java/org/thoughtcrime/securesms/util/ContentResolverUtils.kt

app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt

bemusementpark · 2024-05-24T08:30:11Z

app/src/main/java/org/thoughtcrime/securesms/conversation/v2/input_bar/InputBarRecordingView.kt

+    override fun onAttachedToWindow() {
+        super.onAttachedToWindow()
+
+        if (isVisible) {


isVisible won't take parent visibility into account, so this will probably start the timer onAttach.

My intention is to fix the memory leak and potential bug that animation won't run on reattach. What you are saying is a performance future proof (i.e. prevent excessive timer run) which I think doesn't need to happen in this PR.

To be able to control the timer correctly I think you'd need to do the timer thing in dispatchOnDraw, which I think it's too big a rabbit hole to get into.

Or rewrite in compose haha..

I'm saying that now this animation will start when the activity starts, because when this view is inflated and attached, it is visible, even though it's parent layout is not, so the manual call to updateTimer is basically a no-op, because updateTimer is already running.

It'd be nice for this animation to not run while the Activity is in the background.

bemusementpark · 2024-05-24T08:52:03Z

app/src/main/java/org/thoughtcrime/securesms/home/HomeActivity.kt

-        val broadcastReceiver = object : BroadcastReceiver() {
-            override fun onReceive(context: Context, intent: Intent) {
-                binding.recyclerView.adapter!!.notifyDataSetChanged()
-            }
-        }
-        this.broadcastReceiver = broadcastReceiver
-        LocalBroadcastManager.getInstance(this).registerReceiver(broadcastReceiver, IntentFilter("blockedContactsChanged"))


yeah, it looked like there's no other usage of "blockedContactsChanged"

app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt

bemusementpark

looking good!

Fixes SES-1936

75e53c8

simophin changed the title ~~Fixes SES-1936~~ [SES-1936] Fixes memory leaks May 21, 2024

simophin commented May 21, 2024

View reviewed changes

simophin requested review from bemusementpark and AL-Session May 21, 2024 01:51

simophin changed the title ~~[SES-1936] Fixes memory leaks~~ [SES-1936] Fix memory leaks May 21, 2024

bemusementpark suggested changes May 21, 2024

View reviewed changes

AL-Session approved these changes May 21, 2024

View reviewed changes

Feedback

c7c0519

bemusementpark approved these changes May 23, 2024

View reviewed changes

fanchao added 2 commits May 23, 2024 13:48

Tidy up

90f0cae

Naming

c1d82cc

More changes

31f4de2

simophin commented May 23, 2024

View reviewed changes

bemusementpark approved these changes May 24, 2024

View reviewed changes

fanchao added 3 commits May 27, 2024 10:29

OOM feedback

c0128b8

More work on animation views

2d7f23a

Merge remote-tracking branch 'upstream/dev' into ses-1936-oom

3533548

bemusementpark approved these changes May 27, 2024

View reviewed changes

bemusementpark merged commit b757691 into oxen-io:dev May 28, 2024
1 check failed

simophin deleted the ses-1936-oom branch May 28, 2024 04:55

[SES-1936] Fix memory leaks #1493

[SES-1936] Fix memory leaks #1493

Conversation

simophin commented May 21, 2024

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simophin commented May 21, 2024

bemusementpark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simophin May 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AL-Session left a comment

Choose a reason for hiding this comment

simophin commented May 22, 2024

bemusementpark May 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simophin commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bemusementpark left a comment

Choose a reason for hiding this comment

simophin May 21, 2024 •

edited

Loading

bemusementpark May 22, 2024 •

edited

Loading