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.

Sign up for GitHub

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

[SES-1936] Fix memory leaks #1493

Merged
merged 8 commits into from
May 28, 2024
Merged

[SES-1936] Fix memory leaks #1493

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
75e53c8
Fixes SES-1936
May 21, 2024
c7c0519
Feedback
May 22, 2024
90f0cae
Tidy up
May 23, 2024
c1d82cc
Naming
May 23, 2024
31f4de2
More changes
May 23, 2024
c0128b8
OOM feedback
May 27, 2024
2d7f23a
More work on animation views
May 27, 2024
3533548
Merge remote-tracking branch 'upstream/dev' into ses-1936-oom
May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/main/java/org/thoughtcrime/securesms/attachments/AttachmentServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public AttachmentServer(Context context, Attachment attachment)
throws IOException
{
try {
this.context = context;
this.context = context.getApplicationContext();
this.attachment = attachment;
this.socket = new ServerSocket(0, 0, InetAddress.getByAddress(new byte[]{127, 0, 0, 1}));
this.port = socket.getLocalPort();
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/org/thoughtcrime/securesms/components/ProfilePictureView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ class ProfilePictureView @JvmOverloads constructor(

glide.clear(imageView)

val placeholder = PlaceholderAvatarPhoto(context, publicKey, displayName ?: "${publicKey.take(4)}...${publicKey.takeLast(4)}")
val placeholder = PlaceholderAvatarPhoto(publicKey, displayName ?: "${publicKey.take(4)}...${publicKey.takeLast(4)}")

if (signalProfilePicture != null && avatar != "0" && avatar != "") {
glide.load(signalProfilePicture)
Expand Down
1 change: 1 addition & 0 deletions app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,7 @@ class ConversationActivityV2 : PassphraseRequiredActionBarActivity(), InputBarDe

override fun onDestroy() {
viewModel.saveDraft(binding?.inputBar?.text?.trim() ?: "")
cancelVoiceMessage()
tearDownRecipientObserver()
super.onDestroy()
binding = null
Expand Down
7 changes: 6 additions & 1 deletion ...c/main/java/org/thoughtcrime/securesms/conversation/v2/input_bar/InputBarRecordingView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,12 @@ class InputBarRecordingView : RelativeLayout {
private fun updateTimer() {
val duration = (Date().time - startTimestamp) / 1000L
binding.recordingViewDurationTextView.text = DateUtils.formatElapsedTime(duration)
snHandler.postDelayed({ updateTimer() }, 500)

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to restart if it's reattached?

Copy link
Author

@simophin simophin May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateTimer posts a delayed call to updateTimer

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

fun lock() {
Expand Down
9 changes: 5 additions & 4 deletions app/src/main/java/org/thoughtcrime/securesms/glide/PlaceholderAvatarLoader.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.thoughtcrime.securesms.glide

import android.content.Context
import android.graphics.drawable.BitmapDrawable
import com.bumptech.glide.load.Options
import com.bumptech.glide.load.model.ModelLoader
Expand All @@ -8,22 +9,22 @@ import com.bumptech.glide.load.model.ModelLoaderFactory
import com.bumptech.glide.load.model.MultiModelLoaderFactory
import org.session.libsession.avatars.PlaceholderAvatarPhoto

class PlaceholderAvatarLoader(): ModelLoader<PlaceholderAvatarPhoto, BitmapDrawable> {
class PlaceholderAvatarLoader(private val appContext: Context): ModelLoader<PlaceholderAvatarPhoto, BitmapDrawable> {

override fun buildLoadData(
model: PlaceholderAvatarPhoto,
width: Int,
height: Int,
options: Options
): LoadData<BitmapDrawable> {
return LoadData(model, PlaceholderAvatarFetcher(model.context, model))
return LoadData(model, PlaceholderAvatarFetcher(appContext, model))
}

override fun handles(model: PlaceholderAvatarPhoto): Boolean = true

class Factory() : ModelLoaderFactory<PlaceholderAvatarPhoto, BitmapDrawable> {
class Factory(private val appContext: Context) : ModelLoaderFactory<PlaceholderAvatarPhoto, BitmapDrawable> {
override fun build(multiFactory: MultiModelLoaderFactory): ModelLoader<PlaceholderAvatarPhoto, BitmapDrawable> {
return PlaceholderAvatarLoader()
return PlaceholderAvatarLoader(appContext)
}
override fun teardown() {}
}
Expand Down
11 changes: 10 additions & 1 deletion app/src/main/java/org/thoughtcrime/securesms/home/HomeViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class HomeViewModel @Inject constructor(private val threadDb: ThreadDatabase): V

private val executor = viewModelScope + SupervisorJob()
private var lastContext: WeakReference<Context>? = null
private var updateJobs: MutableList<Job> = mutableListOf()
private val updateJobs: MutableList<Job> = mutableListOf()

private val _conversations = MutableLiveData<List<ThreadRecord>>()
val conversations: LiveData<List<ThreadRecord>> = _conversations
Expand All @@ -31,6 +31,15 @@ class HomeViewModel @Inject constructor(private val threadDb: ThreadDatabase): V

fun tryUpdateChannel() = listUpdateChannel.trySend(Unit)

override fun onCleared() {
super.onCleared()

for (job in updateJobs) {
job.cancel()
}
updateJobs.clear()
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me come up with something


fun getObservable(context: Context): LiveData<List<ThreadRecord>> {
// If the context has changed (eg. the activity gets recreated) then
// we need to cancel the old executors and recreate them to prevent
Expand Down
19 changes: 11 additions & 8 deletions app/src/main/java/org/thoughtcrime/securesms/home/PathActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,19 +240,22 @@ class PathActivity : PassphraseRequiredActionBarActivity() {
dotViewLayoutParams.addRule(CENTER_IN_PARENT)
dotView.layoutParams = dotViewLayoutParams
addView(dotView)
Handler().postDelayed({
postDelayed({
performAnimation()
}, dotAnimationStartDelay)
}

private fun performAnimation() {
expand()
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Handler().postDelayed({
collapse()
Handler().postDelayed({
performAnimation()
}, dotAnimationRepeatInterval)
}, 1000)
if (isAttachedToWindow) {
expand()

postDelayed({
collapse()
postDelayed({
performAnimation()
}, dotAnimationRepeatInterval)
}, 1000)
}
}

private fun expand() {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/org/thoughtcrime/securesms/mms/SignalGlideModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void registerComponents(@NonNull Context context, @NonNull Glide glide, @
registry.append(DecryptableUri.class, InputStream.class, new DecryptableStreamUriLoader.Factory(context));
registry.append(AttachmentModel.class, InputStream.class, new AttachmentStreamUriLoader.Factory());
registry.append(ChunkedImageUrl.class, InputStream.class, new ChunkedImageUrlLoader.Factory());
registry.append(PlaceholderAvatarPhoto.class, BitmapDrawable.class, new PlaceholderAvatarLoader.Factory());
registry.append(PlaceholderAvatarPhoto.class, BitmapDrawable.class, new PlaceholderAvatarLoader.Factory(context));
registry.replace(GlideUrl.class, InputStream.class, new OkHttpUrlLoader.Factory());
}

Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/org/thoughtcrime/securesms/util/IP2Country.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class IP2Country private constructor(private val context: Context) {

public fun configureIfNeeded(context: Context) {
if (isInitialized) { return; }
shared = IP2Country(context)
shared = IP2Country(context.applicationContext)
}
}

Expand Down
5 changes: 1 addition & 4 deletions libsession/src/main/java/org/session/libsession/avatars/PlaceholderAvatarPhoto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
package org.session.libsession.avatars

import android.content.Context
import com.bumptech.glide.load.Key
import java.security.MessageDigest

class PlaceholderAvatarPhoto(val context: Context,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val hashString: String,
class PlaceholderAvatarPhoto(val hashString: String,
val displayName: String): Key {

override fun updateDiskCacheKey(messageDigest: MessageDigest) {
messageDigest.update(hashString.encodeToByteArray())
messageDigest.update(displayName.encodeToByteArray())
Expand Down
6 changes: 3 additions & 3 deletions libsession/src/main/java/org/session/libsession/utilities/recipients/Recipient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public class Recipient implements RecipientModifiedListener {
private final @NonNull Address address;
private final @NonNull List<Recipient> participants = new LinkedList<>();

private Context context;
private final Context context;
private @Nullable String name;
private @Nullable String customLabel;
private boolean resolving;
Expand Down Expand Up @@ -132,7 +132,7 @@ public static boolean removeCached(@NonNull Address address) {
@NonNull Optional<RecipientDetails> details,
@NonNull ListenableFutureTask<RecipientDetails> future)
{
this.context = context;
this.context = context.getApplicationContext();
this.address = address;
this.color = null;
this.resolving = true;
Expand Down Expand Up @@ -259,7 +259,7 @@ public void onFailure(ExecutionException error) {
}

Recipient(@NonNull Context context, @NonNull Address address, @NonNull RecipientDetails details) {
this.context = context;
this.context = context.getApplicationContext();
this.address = address;
this.contactUri = details.contactUri;
this.name = details.name;
Expand Down