Memory leaks #9221
Memory leaks #9221
Conversation
|
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
|
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Thanks stale bot. But it's still not fixed. I won't let this get closed :-) This is just ridiculous. |
| this.currentIsDefaultSms = currentIsDefaultSms; | ||
| this.future = future; | ||
| this.contextWeakReference = contextWeakReference; | ||
| this.activityWeakReference = activityWeakReference; |
There was a problem hiding this comment.
Activity and Context are going to be the same object here so no need to capture both, we can simply capture the activity.
| **/ | ||
| public class InitializeSecurityAsyncTask extends AsyncTask<Recipient, Void, boolean[]> { | ||
|
|
||
|
|
|
|
||
|
|
||
| private long threadId; | ||
|
|
There was a problem hiding this comment.
These spaces are unnecessary, all of these should be final, and they should be aligned as per our style:
private final boolean currentIsDefaultSms
private final SettableFuture<Boolean future;
| private String tag; | ||
|
|
||
| public InitializeSecurityAsyncTask(long threadId, boolean currentSecureText, boolean currentIsDefaultSms, SettableFuture<Boolean> future, WeakReference<Context> contextWeakReference, WeakReference<ConversationActivity> activityWeakReference, String tag) { | ||
| this.threadId = threadId; |
|
|
||
| @Override | ||
| protected boolean[] doInBackground(Recipient... params) { | ||
| Context context = contextWeakReference.get(); |
There was a problem hiding this comment.
alignment:
Context context = contextWeakReference.get();
Recipient recipient = params[0].resolve()
| private SettableFuture<Long> future; | ||
|
|
||
| public SaveDraftAsyncTask(WeakReference<Context> contextWeakReference, WeakReference<ConversationActivity> activityWeakReference, Recipient recipient, int thisDistributionType, DraftDatabase.Drafts drafts, SettableFuture<Long> future) { | ||
| this.contextWeakReference = contextWeakReference; |
There was a problem hiding this comment.
same comment about context, if it's the application context, we can just pass it in as-is. The memory leak would have been due to keeping the activity in memory.
|
|
||
| @Override | ||
| protected Long doInBackground(Long... params) { | ||
| ThreadDatabase threadDatabase = DatabaseFactory.getThreadDatabase(contextWeakReference.get()); |
There was a problem hiding this comment.
Null pointer exception if the weak reference has been cleared.
| threadId = threadDatabase.getThreadIdFor(recipient, thisDistributionType); | ||
|
|
||
| draftDatabase.insertDrafts(threadId, drafts); | ||
| threadDatabase.updateSnippet(threadId, drafts.getSnippet(contextWeakReference.get()), |
There was a problem hiding this comment.
formatting for method calls on multiple lines:
threadDatabase.updateSnippet(threadId,
drafts...
| activityWeakReference.get().handleSecurityChange(result[0], result[1]); | ||
| } | ||
| future.set(true); | ||
| activityWeakReference.get().onSecurityUpdated(); |
There was a problem hiding this comment.
This strongly couples this task to the activity. Instead, we should create a callback interface which describes what methods this needs, and pass an implementation of it (which can be the activity, or an inner object in the activity)
interface Callback {
void handleSecurityChange(InitializeSecurityAsyncTaskResult)
void onSecurityUpdated()
}
|
|
||
| @Override | ||
| protected void onPostExecute(Long result) { | ||
| future.set(result); |
There was a problem hiding this comment.
Consider the callback pattern mentioned above, instead of this future (the callback would wrap the future and set the result on it.
This will prevent possible leak if the future outlives the activity, since this class can keep a weak reference to the callback.
Just keep in mind that if your callback implementor is an object in the activity and not the activity itself, the activity needs to keep a strong reference to it, otherwise it'll get GC'd and youi'll never invoke it.
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This issue has been closed due to inactivity. |
3 participants
First time contributor checklist
Contributor checklist
Fixes #1234syntaxDescription
As Described in #9220, there are memory leaks in the ConversationActivity. This PR aims to solve the ones that happen when opening and closing the activity. The AsyncTasks created in the initializeSecurity() and saveDraft() methods have implicit references to the activity. By moving the code of these AsyncTasks to separate classes and making weak references to the activity, these references are not created and the memory can be liberated. As seen in the memory profiling below, when the app is put in background, memory can be freed up.
