From 356fe97185b4bcc838dd9d366ea1a671a082c72b Mon Sep 17 00:00:00 2001 From: Jacky Wang Date: Tue, 17 Dec 2024 09:52:36 +0800 Subject: [PATCH] Fix potential OOM caused by DataProcessManager The AsyncTasks started in DataProcessManager are not cancelled when fragment is closed. Introduce helper class LifecycleAwareAsyncTask to cancel AsyncTask automatically when lifecycle is stopped. Bug: 384473507 Flag: EXEMPT bug fix Test: Unit test & open/close battery usage screen 1000 times Change-Id: I060f559fa85cc5feb9a42cb8dcc0581782a91d09 --- .../batteryusage/BatteryUsageDataLoader.java | 1 + .../batteryusage/DataProcessManager.java | 52 ++++-- .../batteryusage/LifecycleAwareAsyncTask.kt | 61 +++++++ .../batteryusage/PowerUsageAdvanced.java | 6 +- .../batteryusage/DataProcessManagerTest.java | 7 + .../LifecycleAwareAsyncTaskTest.kt | 150 ++++++++++++++++++ 6 files changed, 258 insertions(+), 19 deletions(-) create mode 100644 src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTask.kt create mode 100644 tests/robotests/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTaskTest.kt diff --git a/src/com/android/settings/fuelgauge/batteryusage/BatteryUsageDataLoader.java b/src/com/android/settings/fuelgauge/batteryusage/BatteryUsageDataLoader.java index df84aba2d89..b917d1f7a03 100644 --- a/src/com/android/settings/fuelgauge/batteryusage/BatteryUsageDataLoader.java +++ b/src/com/android/settings/fuelgauge/batteryusage/BatteryUsageDataLoader.java @@ -118,6 +118,7 @@ public final class BatteryUsageDataLoader { final BatteryLevelData batteryLevelData = DataProcessManager.getBatteryLevelData( context, + null, userIdsSeries, /* isFromPeriodJob= */ true, batteryDiffDataMap -> { diff --git a/src/com/android/settings/fuelgauge/batteryusage/DataProcessManager.java b/src/com/android/settings/fuelgauge/batteryusage/DataProcessManager.java index fd548abd274..9992313d5da 100644 --- a/src/com/android/settings/fuelgauge/batteryusage/DataProcessManager.java +++ b/src/com/android/settings/fuelgauge/batteryusage/DataProcessManager.java @@ -18,12 +18,12 @@ package com.android.settings.fuelgauge.batteryusage; import android.app.usage.UsageEvents; import android.content.Context; -import android.os.AsyncTask; import android.util.ArrayMap; import android.util.Log; import androidx.annotation.NonNull; import androidx.annotation.Nullable; +import androidx.lifecycle.Lifecycle; import com.android.internal.annotations.VisibleForTesting; import com.android.settings.fuelgauge.PowerUsageFeatureProvider; @@ -80,6 +80,7 @@ public class DataProcessManager { private final long mLastFullChargeTimestamp; private final boolean mIsFromPeriodJob; private final Context mContext; + private final @Nullable Lifecycle mLifecycle; private final UserIdsSeries mUserIdsSeries; private final OnBatteryDiffDataMapLoadedListener mCallbackFunction; private final List mAppUsageEventList = new ArrayList<>(); @@ -120,6 +121,7 @@ public class DataProcessManager { /** Constructor when there exists battery level data. */ DataProcessManager( Context context, + @Nullable Lifecycle lifecycle, final UserIdsSeries userIdsSeries, final boolean isFromPeriodJob, final long rawStartTimestamp, @@ -128,6 +130,7 @@ public class DataProcessManager { @NonNull final List hourlyBatteryLevelsPerDay, @NonNull final Map> batteryHistoryMap) { mContext = context.getApplicationContext(); + mLifecycle = lifecycle; mUserIdsSeries = userIdsSeries; mIsFromPeriodJob = isFromPeriodJob; mRawStartTimestamp = rawStartTimestamp; @@ -140,9 +143,11 @@ public class DataProcessManager { /** Constructor when there is no battery level data. */ DataProcessManager( Context context, + @Nullable Lifecycle lifecycle, final UserIdsSeries userIdsSeries, @NonNull final OnBatteryDiffDataMapLoadedListener callbackFunction) { mContext = context.getApplicationContext(); + mLifecycle = lifecycle; mUserIdsSeries = userIdsSeries; mCallbackFunction = callbackFunction; mIsFromPeriodJob = false; @@ -223,7 +228,7 @@ public class DataProcessManager { } private void loadCurrentBatteryHistoryMap() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected Map doInBackground(Void... voids) { final long startTime = System.currentTimeMillis(); @@ -242,6 +247,7 @@ public class DataProcessManager { @Override protected void onPostExecute( final Map currentBatteryHistoryMap) { + super.onPostExecute(currentBatteryHistoryMap); if (mBatteryHistoryMap != null) { // Replaces the placeholder in mBatteryHistoryMap. for (Map.Entry> mapEntry : @@ -256,11 +262,11 @@ public class DataProcessManager { mIsCurrentBatteryHistoryLoaded = true; tryToGenerateFinalDataAndApplyCallback(); } - }.execute(); + }.start(); } private void loadCurrentAppUsageList() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override @Nullable protected List doInBackground(Void... voids) { @@ -299,6 +305,7 @@ public class DataProcessManager { @Override protected void onPostExecute(final List currentAppUsageList) { + super.onPostExecute(currentAppUsageList); if (currentAppUsageList == null || currentAppUsageList.isEmpty()) { Log.d(TAG, "currentAppUsageList is null or empty"); } else { @@ -307,11 +314,11 @@ public class DataProcessManager { mIsCurrentAppUsageLoaded = true; tryToProcessAppUsageData(); } - }.execute(); + }.start(); } private void loadDatabaseAppUsageList() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected List doInBackground(Void... voids) { if (!shouldLoadAppUsageData()) { @@ -337,6 +344,7 @@ public class DataProcessManager { @Override protected void onPostExecute(final List databaseAppUsageList) { + super.onPostExecute(databaseAppUsageList); if (databaseAppUsageList == null || databaseAppUsageList.isEmpty()) { Log.d(TAG, "databaseAppUsageList is null or empty"); } else { @@ -345,11 +353,11 @@ public class DataProcessManager { mIsDatabaseAppUsageLoaded = true; tryToProcessAppUsageData(); } - }.execute(); + }.start(); } private void loadPowerConnectionBatteryEventList() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected List doInBackground(Void... voids) { final long startTime = System.currentTimeMillis(); @@ -370,6 +378,7 @@ public class DataProcessManager { @Override protected void onPostExecute(final List batteryEventList) { + super.onPostExecute(batteryEventList); if (batteryEventList == null || batteryEventList.isEmpty()) { Log.d(TAG, "batteryEventList is null or empty"); } else { @@ -379,11 +388,11 @@ public class DataProcessManager { mIsBatteryEventLoaded = true; tryToProcessAppUsageData(); } - }.execute(); + }.start(); } private void loadBatteryUsageSlotList() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected List doInBackground(Void... voids) { final long startTime = System.currentTimeMillis(); @@ -402,6 +411,7 @@ public class DataProcessManager { @Override protected void onPostExecute(final List batteryUsageSlotList) { + super.onPostExecute(batteryUsageSlotList); if (batteryUsageSlotList == null || batteryUsageSlotList.isEmpty()) { Log.d(TAG, "batteryUsageSlotList is null or empty"); } else { @@ -411,11 +421,11 @@ public class DataProcessManager { mIsBatteryUsageSlotLoaded = true; tryToGenerateFinalDataAndApplyCallback(); } - }.execute(); + }.start(); } private void loadAndApplyBatteryMapFromServiceOnly() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected Map doInBackground(Void... voids) { final long startTime = System.currentTimeMillis(); @@ -437,11 +447,12 @@ public class DataProcessManager { @Override protected void onPostExecute(final Map batteryDiffDataMap) { + super.onPostExecute(batteryDiffDataMap); if (mCallbackFunction != null) { mCallbackFunction.onBatteryDiffDataMapLoaded(batteryDiffDataMap); } } - }.execute(); + }.start(); } private void tryToProcessAppUsageData() { @@ -481,7 +492,7 @@ public class DataProcessManager { } private synchronized void generateFinalDataAndApplyCallback() { - new AsyncTask>() { + new LifecycleAwareAsyncTask>(mLifecycle) { @Override protected Map doInBackground(Void... voids) { final long startTime = System.currentTimeMillis(); @@ -523,11 +534,12 @@ public class DataProcessManager { @Override protected void onPostExecute(final Map batteryDiffDataMap) { + super.onPostExecute(batteryDiffDataMap); if (mCallbackFunction != null) { mCallbackFunction.onBatteryDiffDataMapLoaded(batteryDiffDataMap); } } - }.execute(); + }.start(); } // Whether we should load app usage data from service or database. @@ -566,6 +578,7 @@ public class DataProcessManager { @Nullable public static BatteryLevelData getBatteryLevelData( Context context, + @Nullable Lifecycle lifecycle, final UserIdsSeries userIdsSeries, final boolean isFromPeriodJob, final OnBatteryDiffDataMapLoadedListener onBatteryUsageMapLoadedListener) { @@ -585,6 +598,7 @@ public class DataProcessManager { final BatteryLevelData batteryLevelData = getPeriodBatteryLevelData( context, + lifecycle, userIdsSeries, startTimestamp, lastFullChargeTime, @@ -604,6 +618,7 @@ public class DataProcessManager { private static BatteryLevelData getPeriodBatteryLevelData( Context context, + @Nullable Lifecycle lifecycle, final UserIdsSeries userIdsSeries, final long startTimestamp, final long lastFullChargeTime, @@ -631,7 +646,8 @@ public class DataProcessManager { lastFullChargeTime); if (batteryHistoryMap == null || batteryHistoryMap.isEmpty()) { Log.d(TAG, "batteryHistoryMap is null in getPeriodBatteryLevelData()"); - new DataProcessManager(context, userIdsSeries, onBatteryDiffDataMapLoadedListener) + new DataProcessManager(context, lifecycle, userIdsSeries, + onBatteryDiffDataMapLoadedListener) .start(); return null; } @@ -660,7 +676,8 @@ public class DataProcessManager { DataProcessor.getLevelDataThroughProcessedHistoryMap( context, processedBatteryHistoryMap); if (batteryLevelData == null) { - new DataProcessManager(context, userIdsSeries, onBatteryDiffDataMapLoadedListener) + new DataProcessManager(context, lifecycle, userIdsSeries, + onBatteryDiffDataMapLoadedListener) .start(); Log.d(TAG, "getBatteryLevelData() returns null"); return null; @@ -669,6 +686,7 @@ public class DataProcessManager { // Start the async task to compute diff usage data and load labels and icons. new DataProcessManager( context, + lifecycle, userIdsSeries, isFromPeriodJob, startTimestamp, diff --git a/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTask.kt b/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTask.kt new file mode 100644 index 00000000000..a715cb7f363 --- /dev/null +++ b/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTask.kt @@ -0,0 +1,61 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.fuelgauge.batteryusage + +import android.os.AsyncTask +import androidx.annotation.CallSuper +import androidx.lifecycle.DefaultLifecycleObserver +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleOwner +import com.android.settingslib.datastore.HandlerExecutor.Companion.main as mainExecutor + +/** + * Lifecycle aware [AsyncTask] to cancel task automatically when [lifecycle] is stopped. + * + * Must call [start] instead of [execute] to run the task. + */ +abstract class LifecycleAwareAsyncTask(private val lifecycle: Lifecycle?) : + AsyncTask(), DefaultLifecycleObserver { + + @CallSuper + override fun onPostExecute(result: Result) { + lifecycle?.removeObserver(this) + } + + override fun onStop(owner: LifecycleOwner) { + cancel(false) + lifecycle?.removeObserver(this) + } + + /** + * Starts the task, which invokes [execute] (cannot override [execute] as it is final). + * + * This method is expected to be invoked from main thread but current usage might call from + * background thread. + */ + fun start() { + execute() // expects main thread + val lifecycle = lifecycle ?: return + mainExecutor.execute { + // Status is updated to FINISHED if onPoseExecute happened before. And task is cancelled + // if lifecycle is stopped. + if (status == Status.RUNNING && !isCancelled) { + lifecycle.addObserver(this) // requires main thread + } + } + } +} diff --git a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java index 1ed6a747592..9943d0b429b 100644 --- a/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java +++ b/src/com/android/settings/fuelgauge/batteryusage/PowerUsageAdvanced.java @@ -503,9 +503,11 @@ public class PowerUsageAdvanced extends PowerUsageBase { @Override public BatteryLevelData loadInBackground() { + Context context = getContext(); return DataProcessManager.getBatteryLevelData( - getContext(), - new UserIdsSeries(getContext(), /* isNonUIRequest= */ false), + context, + getLifecycle(), + new UserIdsSeries(context, /* isNonUIRequest= */ false), /* isFromPeriodJob= */ false, PowerUsageAdvanced.this::onBatteryDiffDataMapUpdate); } diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/DataProcessManagerTest.java b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/DataProcessManagerTest.java index 1fed13f8e8f..e71d6a3efc7 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/DataProcessManagerTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/DataProcessManagerTest.java @@ -110,6 +110,7 @@ public final class DataProcessManagerTest { mDataProcessManager = new DataProcessManager( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ false, /* rawStartTimestamp= */ 0L, @@ -130,6 +131,7 @@ public final class DataProcessManagerTest { final DataProcessManager dataProcessManager = new DataProcessManager( mContext, + null, mUserIdsSeries, /* callbackFunction= */ null); assertThat(dataProcessManager.getShowScreenOnTime()).isFalse(); @@ -255,6 +257,7 @@ public final class DataProcessManagerTest { final DataProcessManager dataProcessManager = new DataProcessManager( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ false, /* rawStartTimestamp= */ 2L, @@ -346,6 +349,7 @@ public final class DataProcessManagerTest { assertThat( DataProcessManager.getBatteryLevelData( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ false, /* asyncResponseDelegate= */ null)) @@ -353,6 +357,7 @@ public final class DataProcessManagerTest { assertThat( DataProcessManager.getBatteryLevelData( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ true, /* asyncResponseDelegate= */ null)) @@ -374,6 +379,7 @@ public final class DataProcessManagerTest { final BatteryLevelData resultData = DataProcessManager.getBatteryLevelData( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ false, /* asyncResponseDelegate= */ null); @@ -402,6 +408,7 @@ public final class DataProcessManagerTest { final BatteryLevelData resultData = DataProcessManager.getBatteryLevelData( mContext, + null, mUserIdsSeries, /* isFromPeriodJob= */ false, /* asyncResponseDelegate= */ null); diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTaskTest.kt b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTaskTest.kt new file mode 100644 index 00000000000..fde45b7232d --- /dev/null +++ b/tests/robotests/src/com/android/settings/fuelgauge/batteryusage/LifecycleAwareAsyncTaskTest.kt @@ -0,0 +1,150 @@ +/* + * Copyright (C) 2024 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.android.settings.fuelgauge.batteryusage + +import androidx.lifecycle.Lifecycle +import androidx.lifecycle.LifecycleObserver +import androidx.lifecycle.LifecycleOwner +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import java.util.concurrent.CountDownLatch + +@RunWith(AndroidJUnit4::class) +class LifecycleAwareAsyncTaskTest { + private val instrumentation = InstrumentationRegistry.getInstrumentation() + private val lifecycle = mock() + private val lifecycleOwner = mock() + + @Test + fun addObserver_onPostExecute_onStop() { + val taskBeginCountDownLatch = CountDownLatch(1) + val taskEndCountDownLatch = CountDownLatch(1) + val asyncTask = + object : LifecycleAwareAsyncTask(lifecycle) { + override fun doInBackground(vararg params: Void): Void? { + taskBeginCountDownLatch.await() + taskEndCountDownLatch.countDown() + return null + } + } + + asyncTask.start() + taskBeginCountDownLatch.countDown() + verify(lifecycle).addObserver(asyncTask) + + taskEndCountDownLatch.await() + asyncTask.onStop(lifecycleOwner) + assertThat(asyncTask.isCancelled).isTrue() + verify(lifecycle).removeObserver(asyncTask) + } + + @Test + fun addObserver_onStop() { + val executorBlocker = CountDownLatch(1) + object : LifecycleAwareAsyncTask(null) { + override fun doInBackground(vararg params: Void?): Void? { + executorBlocker.await() + return null + } + } + .start() + + val asyncTask = + object : LifecycleAwareAsyncTask(lifecycle) { + override fun doInBackground(vararg params: Void) = null + } + + asyncTask.start() + verify(lifecycle).addObserver(asyncTask) + + asyncTask.onStop(lifecycleOwner) + executorBlocker.countDown() + assertThat(asyncTask.isCancelled).isTrue() + verify(lifecycle).removeObserver(asyncTask) + } + + @Test + fun onPostExecute_addObserver() { + val observers = mutableListOf() + val lifecycle = + object : Lifecycle() { + override val currentState: State + get() = State.RESUMED + + override fun addObserver(observer: LifecycleObserver) { + observers.add(observer) + } + + override fun removeObserver(observer: LifecycleObserver) { + observers.remove(observer) + } + } + val asyncTask = + object : LifecycleAwareAsyncTask(lifecycle) { + override fun doInBackground(vararg params: Void) = null + } + + Thread { asyncTask.start() }.start() + idleAsyncTaskExecutor() + instrumentation.waitForIdleSync() + + assertThat(observers).isEmpty() + } + + private fun idleAsyncTaskExecutor() { + val taskCountDownLatch = CountDownLatch(1) + object : LifecycleAwareAsyncTask(null) { + override fun doInBackground(vararg params: Void): Void? { + taskCountDownLatch.countDown() + return null + } + } + .start() + taskCountDownLatch.await() + } + + @Test + fun onStop_addObserver() { + val executorBlocker = CountDownLatch(1) + object : LifecycleAwareAsyncTask(null) { + override fun doInBackground(vararg params: Void?): Void? { + executorBlocker.await() + return null + } + } + .start() + + val asyncTask = + object : LifecycleAwareAsyncTask(lifecycle) { + override fun doInBackground(vararg params: Void) = null + } + + asyncTask.onStop(lifecycleOwner) + assertThat(asyncTask.isCancelled).isTrue() + verify(lifecycle).removeObserver(asyncTask) + + asyncTask.start() + verify(lifecycle, never()).addObserver(asyncTask) + executorBlocker.countDown() + } +}