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 2cc3bae..a69340c 100644 (file)
--- 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 d6fad34..f159e79 100644 (file)
--- 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<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 @@ 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<String> 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<String> 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<String> 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);
+    }
 }