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
diff --git a/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java b/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java
index 7f6eb9b..afe6dd5 100644
--- a/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java
+++ b/src/com/android/settings/fuelgauge/batterytip/AnomalyDetectionJobService.java
@@ -38,6 +38,7 @@
import android.os.UserHandle;
import android.os.UserManager;
import android.provider.Settings;
+import android.support.annotation.GuardedBy;
import android.support.annotation.VisibleForTesting;
import android.util.Log;
import android.util.Pair;
@@ -65,10 +66,13 @@
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 +106,14 @@
.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 +121,10 @@
@Override
public boolean onStopJob(JobParameters jobParameters) {
- return false;
+ synchronized (mLock) {
+ mIsJobCanceled = true;
+ }
+ return true; // Need to reschedule
}
@VisibleForTesting
@@ -229,4 +236,26 @@
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 d6fad34..f159e79 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 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 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.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.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.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 @@
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 @@
mFeatureFactory = FakeFeatureFactory.setupForTest();
when(mBatteryUtils.getAppLongVersionCode(any())).thenReturn(VERSION_CODE);
- mAnomalyDetectionJobService = spy(new AnomalyDetectionJobService());
+ final ServiceController<AnomalyDetectionJobService> 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 @@
}
@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 @@
}
@Test
- public void testSaveAnomalyToDatabase_systemApp_doNotSaveButLog() {
+ public void saveAnomalyToDatabase_systemApp_doNotSaveButLog() {
final ArrayList<String> cookies = new ArrayList<>();
cookies.add(SUBSCRIBER_COOKIES_AUTO_RESTRICTION);
mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies);
@@ -170,7 +187,7 @@
}
@Test
- public void testSaveAnomalyToDatabase_systemUid_doNotSave() {
+ public void saveAnomalyToDatabase_systemUid_doNotSave() {
doReturn(Process.SYSTEM_UID).when(
mAnomalyDetectionJobService).extractUidFromStatsDimensionsValue(any());
@@ -185,7 +202,7 @@
}
@Test
- public void testSaveAnomalyToDatabase_uidNull_doNotSave() {
+ public void saveAnomalyToDatabase_uidNull_doNotSave() {
doReturn(AnomalyDetectionJobService.UID_NULL).when(
mAnomalyDetectionJobService).extractUidFromStatsDimensionsValue(any());
@@ -200,7 +217,7 @@
}
@Test
- public void testSaveAnomalyToDatabase_normalAppWithAutoRestriction_save() {
+ public void saveAnomalyToDatabase_normalAppWithAutoRestriction_save() {
final ArrayList<String> cookies = new ArrayList<>();
cookies.add(SUBSCRIBER_COOKIES_AUTO_RESTRICTION);
mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies);
@@ -224,9 +241,8 @@
Pair.create(MetricsProto.MetricsEvent.FIELD_APP_VERSION_CODE, VERSION_CODE));
}
-
@Test
- public void testSaveAnomalyToDatabase_normalAppWithoutAutoRestriction_save() {
+ public void saveAnomalyToDatabase_normalAppWithoutAutoRestriction_save() {
final ArrayList<String> cookies = new ArrayList<>();
cookies.add(SUBSCRIBER_COOKIES_NOT_AUTO_RESTRICTION);
mBundle.putStringArrayList(StatsManager.EXTRA_STATS_BROADCAST_SUBSCRIBER_COOKIES, cookies);
@@ -251,7 +267,7 @@
}
@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 @@
}
@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 @@
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);
+ }
}