From 7b21bb5bb9a11cc523e3dd40bb39dc576fb09d13 Mon Sep 17 00:00:00 2001 From: Suprabh Shukla Date: Fri, 26 Jan 2018 17:19:28 -0800 Subject: [PATCH] Fixed removeJobsOfNonUsers to keep data consistent removeJobsOfNonUsers() was removing different jobs from the maps mJobs and mJobsPerSourceUid. Also modified it to remove jobs from both the caller and the source user id. Added a test for JobStore.JobSet Test: atest com.android.server.job.JobSetTest Fixes: 72459151 Change-Id: I95021b6370b68ad9e8a94dd4ebcad28905ee3c8b --- .../java/com/android/server/job/JobStore.java | 61 +++++--- .../com/android/server/job/JobSetTest.java | 146 ++++++++++++++++++ 2 files changed, 187 insertions(+), 20 deletions(-) create mode 100644 services/tests/servicestests/src/com/android/server/job/JobSetTest.java diff --git a/services/core/java/com/android/server/job/JobStore.java b/services/core/java/com/android/server/job/JobStore.java index a24a4ac3823bf..778cc2ce83341 100644 --- a/services/core/java/com/android/server/job/JobStore.java +++ b/services/core/java/com/android/server/job/JobStore.java @@ -61,6 +61,7 @@ import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.Set; +import java.util.function.Predicate; /** * Maintains the master list of jobs that the job scheduler is tracking. These jobs are compared by @@ -84,7 +85,7 @@ public final class JobStore { private static final int MAX_OPS_BEFORE_WRITE = 1; final Object mLock; - final JobSet mJobSet; // per-caller-uid tracking + final JobSet mJobSet; // per-caller-uid and per-source-uid tracking final Context mContext; // Bookkeeping around incorrect boot-time system clock @@ -998,10 +999,11 @@ public final class JobStore { } static final class JobSet { - // Key is the getUid() originator of the jobs in each sheaf - private SparseArray> mJobs; - // Same data but with the key as getSourceUid() of the jobs in each sheaf - private SparseArray> mJobsPerSourceUid; + @VisibleForTesting // Key is the getUid() originator of the jobs in each sheaf + final SparseArray> mJobs; + + @VisibleForTesting // Same data but with the key as getSourceUid() of the jobs in each sheaf + final SparseArray> mJobsPerSourceUid; public JobSet() { mJobs = new SparseArray>(); @@ -1044,7 +1046,13 @@ public final class JobStore { jobsForSourceUid = new ArraySet<>(); mJobsPerSourceUid.put(sourceUid, jobsForSourceUid); } - return jobs.add(job) && jobsForSourceUid.add(job); + final boolean added = jobs.add(job); + final boolean addedInSource = jobsForSourceUid.add(job); + if (added != addedInSource) { + Slog.wtf(TAG, "mJobs and mJobsPerSourceUid mismatch; caller= " + added + + " source= " + addedInSource); + } + return added || addedInSource; } public boolean remove(JobStatus job) { @@ -1073,27 +1081,40 @@ public final class JobStore { /** * Removes the jobs of all users not specified by the whitelist of user ids. - * The jobs scheduled by non existent users will not be removed if they were + * This will remove jobs scheduled *by* non-existent users as well as jobs scheduled *for* + * non-existent users */ - public void removeJobsOfNonUsers(int[] whitelist) { - for (int jobSetIndex = mJobsPerSourceUid.size() - 1; jobSetIndex >= 0; jobSetIndex--) { - final int jobUserId = UserHandle.getUserId(mJobsPerSourceUid.keyAt(jobSetIndex)); - if (!ArrayUtils.contains(whitelist, jobUserId)) { - mJobsPerSourceUid.removeAt(jobSetIndex); - } - } + public void removeJobsOfNonUsers(final int[] whitelist) { + final Predicate noSourceUser = + job -> !ArrayUtils.contains(whitelist, job.getSourceUserId()); + final Predicate noCallingUser = + job -> !ArrayUtils.contains(whitelist, job.getUserId()); + removeAll(noSourceUser.or(noCallingUser)); + } + + private void removeAll(Predicate predicate) { for (int jobSetIndex = mJobs.size() - 1; jobSetIndex >= 0; jobSetIndex--) { - final ArraySet jobsForUid = mJobs.valueAt(jobSetIndex); - for (int jobIndex = jobsForUid.size() - 1; jobIndex >= 0; jobIndex--) { - final int jobUserId = jobsForUid.valueAt(jobIndex).getUserId(); - if (!ArrayUtils.contains(whitelist, jobUserId)) { - jobsForUid.removeAt(jobIndex); + final ArraySet jobs = mJobs.valueAt(jobSetIndex); + for (int jobIndex = jobs.size() - 1; jobIndex >= 0; jobIndex--) { + if (predicate.test(jobs.valueAt(jobIndex))) { + jobs.removeAt(jobIndex); } } - if (jobsForUid.size() == 0) { + if (jobs.size() == 0) { mJobs.removeAt(jobSetIndex); } } + for (int jobSetIndex = mJobsPerSourceUid.size() - 1; jobSetIndex >= 0; jobSetIndex--) { + final ArraySet jobs = mJobsPerSourceUid.valueAt(jobSetIndex); + for (int jobIndex = jobs.size() - 1; jobIndex >= 0; jobIndex--) { + if (predicate.test(jobs.valueAt(jobIndex))) { + jobs.removeAt(jobIndex); + } + } + if (jobs.size() == 0) { + mJobsPerSourceUid.removeAt(jobSetIndex); + } + } } public boolean contains(JobStatus job) { diff --git a/services/tests/servicestests/src/com/android/server/job/JobSetTest.java b/services/tests/servicestests/src/com/android/server/job/JobSetTest.java new file mode 100644 index 0000000000000..83bd9fc2f6487 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/job/JobSetTest.java @@ -0,0 +1,146 @@ +/* + * Copyright (C) 2018 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.server.job; + +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.junit.Assume.assumeFalse; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import android.app.job.JobInfo; +import android.content.ComponentName; +import android.content.Context; +import android.content.pm.PackageManagerInternal; +import android.os.Build; +import android.os.UserHandle; +import android.support.test.InstrumentationRegistry; +import android.support.test.filters.SmallTest; +import android.support.test.runner.AndroidJUnit4; +import android.util.ArraySet; +import android.util.Log; +import android.util.SparseArray; + +import com.android.server.LocalServices; +import com.android.server.job.controllers.JobStatus; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; + +@RunWith(AndroidJUnit4.class) +@SmallTest +public class JobSetTest { + private static final String TAG = JobSetTest.class.getSimpleName(); + private static final int SECONDARY_USER_ID_1 = 12; + private static final int SECONDARY_USER_ID_2 = 13; + + private Context mContext; + private ComponentName mComponent; + private JobStore.JobSet mJobSet; + + @Before + public void setUp() throws Exception { + mContext = InstrumentationRegistry.getTargetContext(); + mComponent = new ComponentName(mContext, JobStoreTest.class); + mJobSet = new JobStore.JobSet(); + final PackageManagerInternal pm = mock(PackageManagerInternal.class); + when(pm.getPackageTargetSdkVersion(anyString())) + .thenReturn(Build.VERSION_CODES.CUR_DEVELOPMENT); + LocalServices.addService(PackageManagerInternal.class, pm); + assumeFalse("Test cannot run in user " + mContext.getUserId(), + mContext.getUserId() == SECONDARY_USER_ID_1 + || mContext.getUserId() == SECONDARY_USER_ID_2); + } + + private JobStatus getJobStatusWithCallinUid(int jobId, int callingUid) { + final JobInfo jobInfo = new JobInfo.Builder(jobId, mComponent) + .setPeriodic(10) + .setRequiresCharging(true) + .build(); + return JobStatus.createFromJobInfo(jobInfo, callingUid, mContext.getPackageName(), + mContext.getUserId(), "Test"); + } + + @Test + public void testBothMapsHaveSameJobs() { + final int callingUid1 = UserHandle.getUid(SECONDARY_USER_ID_1, 1); + final int callingUid2 = UserHandle.getUid(SECONDARY_USER_ID_2, 1); + final JobStatus testJob1 = getJobStatusWithCallinUid(1, callingUid1); + final JobStatus testJob2 = getJobStatusWithCallinUid(2, callingUid2); + mJobSet.add(testJob1); + mJobSet.add(testJob2); + for (int i = 11; i <= 20; i++) { + mJobSet.add(getJobStatusWithCallinUid(i, (i%2 == 0) ? callingUid2 : callingUid1)); + } + assertHaveSameJobs(mJobSet.mJobsPerSourceUid, mJobSet.mJobs); + mJobSet.remove(testJob1); + mJobSet.remove(testJob2); + assertHaveSameJobs(mJobSet.mJobsPerSourceUid, mJobSet.mJobs); + mJobSet.removeJobsOfNonUsers(new int[] {mContext.getUserId(), SECONDARY_USER_ID_1}); + assertHaveSameJobs(mJobSet.mJobsPerSourceUid, mJobSet.mJobs); + mJobSet.removeJobsOfNonUsers(new int[] {mContext.getUserId()}); + assertTrue("mJobs should be empty", mJobSet.mJobs.size() == 0); + assertTrue("mJobsPerSourceUid should be empty", mJobSet.mJobsPerSourceUid.size() == 0); + } + + private static void assertHaveSameJobs(SparseArray> map1, + SparseArray> map2) { + final ArraySet set1 = new ArraySet<>(); + final ArraySet set2 = new ArraySet<>(); + int size1 = 0; + for (int i = 0; i < map1.size(); i++) { + final ArraySet jobs = map1.valueAt(i); + if (jobs == null) return; + size1 += jobs.size(); + set1.addAll(jobs); + } + for (int i = 0; i < map2.size(); i++) { + final ArraySet jobs = map2.valueAt(i); + if (jobs == null) return; + size1 -= jobs.size(); + set2.addAll(jobs); + } + if (size1 != 0 || !set1.equals(set2)) { + dump("map1", map1); + dump("map2", map2); + fail("Both maps have different sets of jobs"); + } + } + + private static void dump(String prefix, SparseArray> jobMap) { + final StringBuilder str = new StringBuilder(); + for (int i = 0; i < jobMap.size(); i++) { + final ArraySet jobs = jobMap.valueAt(i); + if (jobs == null) return; + str.append("[Key: " + jobMap.keyAt(i) + ", Value: {"); + for (int j = 0; j < jobs.size(); j++) { + final JobStatus job = jobs.valueAt(j); + str.append("(s=" + job.getSourceUid() + ", c=" + job.getUid() + "), "); + } + str.append("}], "); + } + Log.d(TAG, prefix + ": " + str.toString()); + } + + @After + public void tearDown() throws Exception { + LocalServices.removeServiceForTest(PackageManagerInternal.class); + } +}