From 1c95856977a3879070309dde5314c8f5ea92cad3 Mon Sep 17 00:00:00 2001 From: Lei Yu Date: Thu, 12 Apr 2018 16:44:45 -0700 Subject: [PATCH] Fix crash in anomaly job service If job has been stopped by any reason, we should cancel the background thread, otherwise it will throw SecurityException when dequeuing from JobParams. This CL adds a lock to synchronize the method and stops dequeue work if job has been canceled. Change-Id: I7732b7f7d444a55a4b4ba6645cd2c16b6f840a6c Merged-In: I7732b7f7d444a55a4b4ba6645cd2c16b6f840a6c Fixes: 77968649 Test: RunSettingsRoboTests --- .../batterytip/AnomalyDetectionJobService.java | 41 +++++++++++++-- .../batterytip/AnomalyDetectionJobServiceTest.java | 58 ++++++++++++++++++---- 2 files changed, 83 insertions(+), 16 deletions(-) diff --git a/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java b/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java index 2cc3bae09c..a69340c292 100644 --- a/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java +++ b/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java @@ -38,7 +38,10 @@ import android.os.StatsDimensionsValue; import android.os.UserHandle; import android.os.UserManager; import android.provider.Settings; + import androidx.annotation.VisibleForTesting; +import androidx.annotation.GuardedBy; + import android.util.Log; import android.util.Pair; @@ -65,10 +68,13 @@ public class AnomalyDetectionJobService extends JobService { static final int UID_NULL = -1; @VisibleForTesting static final int STATSD_UID_FILED = 1; - @VisibleForTesting static final long MAX_DELAY_MS = TimeUnit.MINUTES.toMillis(30); + private final Object mLock = new Object(); + @GuardedBy("mLock") + private boolean mIsJobCanceled = false; + public static void scheduleAnomalyDetection(Context context, Intent intent) { final JobScheduler jobScheduler = context.getSystemService(JobScheduler.class); final ComponentName component = new ComponentName(context, @@ -102,14 +108,14 @@ public class AnomalyDetectionJobService extends JobService { .getFactory(this).getMetricsFeatureProvider(); batteryUtils.initBatteryStatsHelper(batteryStatsHelper, null /* bundle */, userManager); - for (JobWorkItem item = params.dequeueWork(); item != null; - item = params.dequeueWork()) { + for (JobWorkItem item = dequeueWork(params); item != null; item = dequeueWork(params)) { saveAnomalyToDatabase(context, batteryStatsHelper, userManager, batteryDatabaseManager, batteryUtils, policy, powerWhitelistBackend, contentResolver, powerUsageFeatureProvider, metricsFeatureProvider, item.getIntent().getExtras()); + + completeWork(params, item); } - jobFinished(params, false /* wantsReschedule */); }); return true; @@ -117,7 +123,10 @@ public class AnomalyDetectionJobService extends JobService { @Override public boolean onStopJob(JobParameters jobParameters) { - return false; + synchronized (mLock) { + mIsJobCanceled = true; + } + return true; // Need to reschedule } @VisibleForTesting @@ -229,4 +238,26 @@ public class AnomalyDetectionJobService extends JobService { return anomalyInfo.anomalyType == StatsManagerConfig.AnomalyType.EXCESSIVE_BACKGROUND_SERVICE; } + + @VisibleForTesting + JobWorkItem dequeueWork(JobParameters parameters) { + synchronized (mLock) { + if (mIsJobCanceled) { + return null; + } + + return parameters.dequeueWork(); + } + } + + @VisibleForTesting + void completeWork(JobParameters parameters, JobWorkItem item) { + synchronized (mLock) { + if (mIsJobCanceled) { + return; + } + + parameters.completeWork(item); + } + } } diff --git a/tests/robotests/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobServiceTest.java b/tests/robotests/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobServiceTest.java index d6fad34e5d..f159e7970e 100644 --- a/tests/robotests/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobServiceTest.java +++ b/tests/robotests/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobServiceTest.java @@ -23,11 +23,14 @@ import static android.os.StatsDimensionsValue.TUPLE_VALUE_TYPE; import static com.google.common.truth.Truth.assertThat; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.spy; @@ -37,7 +40,9 @@ import static org.robolectric.RuntimeEnvironment.application; import android.app.StatsManager; import android.app.job.JobInfo; +import android.app.job.JobParameters; import android.app.job.JobScheduler; +import android.app.job.JobWorkItem; import android.content.Context; import android.content.Intent; import android.os.Bundle; @@ -55,6 +60,7 @@ import com.android.settings.fuelgauge.BatteryUtils; import com.android.settings.overlay.FeatureFactory; import com.android.settings.testutils.FakeFeatureFactory; import com.android.settings.testutils.SettingsRobolectricTestRunner; +import com.android.settings.testutils.shadow.ShadowConnectivityManager; import com.android.settingslib.fuelgauge.PowerWhitelistBackend; import org.junit.Before; @@ -62,8 +68,11 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import org.robolectric.Robolectric; import org.robolectric.RuntimeEnvironment; import org.robolectric.Shadows; +import org.robolectric.android.controller.ServiceController; +import org.robolectric.annotation.Config; import org.robolectric.shadows.ShadowJobScheduler; import java.util.ArrayList; @@ -71,6 +80,7 @@ import java.util.List; import java.util.concurrent.TimeUnit; @RunWith(SettingsRobolectricTestRunner.class) +@Config(shadows = ShadowConnectivityManager.class) public class AnomalyDetectionJobServiceTest { private static final int UID = 12345; private static final String SYSTEM_PACKAGE = "com.android.system"; @@ -92,6 +102,10 @@ public class AnomalyDetectionJobServiceTest { private PowerWhitelistBackend mPowerWhitelistBackend; @Mock private StatsDimensionsValue mStatsDimensionsValue; + @Mock + private JobParameters mJobParameters; + @Mock + private JobWorkItem mJobWorkItem; private BatteryTipPolicy mPolicy; private Bundle mBundle; @@ -110,11 +124,14 @@ public class AnomalyDetectionJobServiceTest { mFeatureFactory = FakeFeatureFactory.setupForTest(); when(mBatteryUtils.getAppLongVersionCode(any())).thenReturn(VERSION_CODE); - mAnomalyDetectionJobService = spy(new AnomalyDetectionJobService()); + final ServiceController controller = + Robolectric.buildService(AnomalyDetectionJobService.class); + mAnomalyDetectionJobService = spy(controller.get()); + doNothing().when(mAnomalyDetectionJobService).jobFinished(any(), anyBoolean()); } @Test - public void testScheduleCleanUp() { + public void scheduleCleanUp() { AnomalyDetectionJobService.scheduleAnomalyDetection(application, new Intent()); ShadowJobScheduler shadowJobScheduler = @@ -129,7 +146,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testSaveAnomalyToDatabase_systemWhitelisted_doNotSave() { + public void saveAnomalyToDatabase_systemWhitelisted_doNotSave() { doReturn(UID).when(mAnomalyDetectionJobService).extractUidFromStatsDimensionsValue(any()); doReturn(true).when(mPowerWhitelistBackend).isSysWhitelistedExceptIdle(any(String[].class)); @@ -144,7 +161,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testSaveAnomalyToDatabase_systemApp_doNotSaveButLog() { + public void saveAnomalyToDatabase_systemApp_doNotSaveButLog() { final ArrayList cookies = new ArrayList<>(); cookies.add(SUBSCRIBER_COOKIES_AUTO_RESTRICTION); mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies); @@ -170,7 +187,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testSaveAnomalyToDatabase_systemUid_doNotSave() { + public void saveAnomalyToDatabase_systemUid_doNotSave() { doReturn(Process.SYSTEM_UID).when( mAnomalyDetectionJobService).extractUidFromStatsDimensionsValue(any()); @@ -185,7 +202,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testSaveAnomalyToDatabase_uidNull_doNotSave() { + public void saveAnomalyToDatabase_uidNull_doNotSave() { doReturn(AnomalyDetectionJobService.UID_NULL).when( mAnomalyDetectionJobService).extractUidFromStatsDimensionsValue(any()); @@ -200,7 +217,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testSaveAnomalyToDatabase_normalAppWithAutoRestriction_save() { + public void saveAnomalyToDatabase_normalAppWithAutoRestriction_save() { final ArrayList cookies = new ArrayList<>(); cookies.add(SUBSCRIBER_COOKIES_AUTO_RESTRICTION); mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies); @@ -224,9 +241,8 @@ public class AnomalyDetectionJobServiceTest { Pair.create(MetricsProto.MetricsEvent.FIELD_APP_VERSION_CODE, VERSION_CODE)); } - @Test - public void testSaveAnomalyToDatabase_normalAppWithoutAutoRestriction_save() { + public void saveAnomalyToDatabase_normalAppWithoutAutoRestriction_save() { final ArrayList cookies = new ArrayList<>(); cookies.add(SUBSCRIBER_COOKIES_NOT_AUTO_RESTRICTION); mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies); @@ -251,7 +267,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testExtractUidFromStatsDimensionsValue_extractCorrectUid() { + public void extractUidFromStatsDimensionsValue_extractCorrectUid() { // Build an integer dimensions value. final StatsDimensionsValue intValue = mock(StatsDimensionsValue.class); when(intValue.isValueType(INT_VALUE_TYPE)).thenReturn(true); @@ -270,7 +286,7 @@ public class AnomalyDetectionJobServiceTest { } @Test - public void testExtractUidFromStatsDimensionsValue_wrongFormat_returnNull() { + public void extractUidFromStatsDimensionsValue_wrongFormat_returnNull() { // Build a float dimensions value final StatsDimensionsValue floatValue = mock(StatsDimensionsValue.class); when(floatValue.isValueType(FLOAT_VALUE_TYPE)).thenReturn(true); @@ -280,4 +296,24 @@ public class AnomalyDetectionJobServiceTest { assertThat(mAnomalyDetectionJobService.extractUidFromStatsDimensionsValue( floatValue)).isEqualTo(AnomalyDetectionJobService.UID_NULL); } + + @Test + public void stopJobWhileDequeuingWork_shouldNotCrash() { + when(mJobParameters.dequeueWork()).thenThrow(new SecurityException()); + + mAnomalyDetectionJobService.onStopJob(mJobParameters); + + // Should not crash even job is stopped + mAnomalyDetectionJobService.dequeueWork(mJobParameters); + } + + @Test + public void stopJobWhileCompletingWork_shouldNotCrash() { + doThrow(new SecurityException()).when(mJobParameters).completeWork(any()); + + mAnomalyDetectionJobService.onStopJob(mJobParameters); + + // Should not crash even job is stopped + mAnomalyDetectionJobService.completeWork(mJobParameters, mJobWorkItem); + } } -- 2.11.0