Skip to content

Prevent memory leak(s) from QuizChecker. #2656

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

Merged
merged 1 commit into from
Mar 19, 2019
Merged

Prevent memory leak(s) from QuizChecker. #2656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions app/src/main/java/fr/free/nrw/commons/contributions/MainActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,7 @@ protected void onResume() {

@Override
protected void onDestroy() {
quizChecker.cleanup();
locationManager.unregisterLocationManager();
// Remove ourself from hashmap to prevent memory leaks
locationManager = null;
Expand Down
23 changes: 10 additions & 13 deletions app/src/main/java/fr/free/nrw/commons/quiz/QuizChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import android.annotation.SuppressLint;
import android.app.Activity;
import android.content.Context;
import android.content.Intent;

import javax.inject.Inject;
Expand All @@ -20,8 +19,6 @@
import io.reactivex.schedulers.Schedulers;
import timber.log.Timber;

import static android.content.Intent.FLAG_ACTIVITY_NEW_TASK;

/**
* fetches the number of images uploaded and number of images reverted.
* Then it calculates the percentage of the images reverted
Expand All @@ -37,7 +34,6 @@ public class QuizChecker {
private boolean isUploadCountFetched;

private CompositeDisposable compositeDisposable = new CompositeDisposable();
public Context context;
Copy link
Member

Choose a reason for hiding this comment

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

Here application context is being injected by dagger. @dbrant Is having an instance of application context also considered dangerous?

In this particular class, I agree that storing the context isn't required and activity context can be used everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here application context is being injected by dagger.

LeakCanary was showing a leak that pointed to the QuizChecker class, and when I looked at it, my eyes automatically jumped to the instance of Context.
But in fact the leak was caused by failing to dispose your CompoundDisposable, which this PR also fixes.

Technically, storing a reference to the application context isn't so bad. However, since Android (in its wisdom) doesn't differentiate between application and activity contexts, it's simply too dangerous to do this. There are almost always ways of accomplishing what you want without keeping a reference to a Context.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, thanks for explaining. Will avoid storing a reference of the context as much as possible. :)


private final SessionManager sessionManager;
private final OkHttpJsonApiClient okHttpJsonApiClient;
Expand All @@ -50,16 +46,13 @@ public class QuizChecker {

/**
* constructor to set the parameters for quiz
* @param context context
* @param sessionManager
* @param okHttpJsonApiClient instance of MediaWikiApi
*/
@Inject
public QuizChecker(Context context,
SessionManager sessionManager,
public QuizChecker(SessionManager sessionManager,
OkHttpJsonApiClient okHttpJsonApiClient,
@Named("default_preferences") JsonKvStore revertKvStore) {
this.context = context;
this.sessionManager = sessionManager;
this.okHttpJsonApiClient = okHttpJsonApiClient;
this.revertKvStore = revertKvStore;
Expand All @@ -70,6 +63,10 @@ public void initQuizCheck(Activity activity) {
setRevertCount(activity);
}

public void cleanup() {
compositeDisposable.clear();
}

/**
* to fet the total number of images uploaded
*/
Expand Down Expand Up @@ -151,10 +148,10 @@ private void calculateRevertParameter(Activity activity) {
@SuppressLint("StringFormatInvalid")
private void callQuiz(Activity activity) {
DialogUtil.showAlertDialog(activity,
context.getResources().getString(R.string.quiz),
context.getResources().getString(R.string.quiz_alert_message, REVERT_PERCENTAGE_FOR_MESSAGE),
context.getResources().getString(R.string.about_translate_proceed),
context.getResources().getString(android.R.string.cancel),
activity.getString(R.string.quiz),
activity.getString(R.string.quiz_alert_message, REVERT_PERCENTAGE_FOR_MESSAGE),
activity.getString(R.string.about_translate_proceed),
activity.getString(android.R.string.cancel),
() -> startQuizActivity(activity), null);
}

Expand All @@ -163,7 +160,7 @@ private void startQuizActivity(Activity activity) {
revertKvStore.putInt(REVERT_SHARED_PREFERENCE, newRevetSharedPrefs);
int newUploadCount = totalUploadCount + revertKvStore.getInt(UPLOAD_SHARED_PREFERENCE, 0);
revertKvStore.putInt(UPLOAD_SHARED_PREFERENCE, newUploadCount);
Intent i = new Intent(context, WelcomeActivity.class);
Intent i = new Intent(activity, WelcomeActivity.class);
i.putExtra("isQuiz", true);
activity.startActivity(i);
}
Expand Down