From 626ec2902cdd876f33a3e588053724ba0f45af6b Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Mon, 29 Mar 2021 17:03:54 -0700 Subject: [PATCH] Fix async behavior and address possible race conditions. 1. Don't release the lock until we've fully processed an app or user removal so that JS stays in a consistent state outside of the lock. Before, we released the lock between cancelling jobs and telling controllers of the removal, so there was a short period where JS was internally inconsistent and an app scheduling a job could land in JS's inconsistency. 2. Actually lock around methods that require the lock be held. Bug: 183921387 Test: atest frameworks/base/services/tests/servicestests/src/com/android/server/job Test: atest frameworks/base/services/tests/mockingservicestests/src/com/android/server/job Test: atest CtsJobSchedulerTestCases Change-Id: I440e3c7753d119e025b0359d2b35bec90fa19628 --- .../server/job/JobSchedulerService.java | 59 ++++++++++--------- .../controllers/ConnectivityController.java | 57 +++++++++++------- 2 files changed, 65 insertions(+), 51 deletions(-) diff --git a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java index a452fbcbe42df..af867b936d72a 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java +++ b/apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java @@ -746,11 +746,14 @@ public class JobSchedulerService extends com.android.server.SystemService Slog.d(TAG, "Removing jobs for package " + pkgName + " in user " + userId); } - // By the time we get here, the process should have already - // been stopped, so the app wouldn't get the stop reason, - // so just put USER instead of UNINSTALL or DISABLED. - cancelJobsForPackageAndUid(pkgName, pkgUid, - JobParameters.STOP_REASON_USER, "app disabled"); + synchronized (mLock) { + // By the time we get here, the process should have + // already been stopped, so the app wouldn't get the + // stop reason, so just put USER instead of UNINSTALL + // or DISABLED. + cancelJobsForPackageAndUidLocked(pkgName, pkgUid, + JobParameters.STOP_REASON_USER, "app disabled"); + } } } catch (RemoteException|IllegalArgumentException e) { /* @@ -791,9 +794,9 @@ public class JobSchedulerService extends com.android.server.SystemService // By the time we get here, the process should have already // been stopped, so the app wouldn't get the stop reason, // so just put USER instead of UNINSTALL or DISABLED. - cancelJobsForPackageAndUid(pkgName, uidRemoved, - JobParameters.STOP_REASON_USER, "app uninstalled"); synchronized (mLock) { + cancelJobsForPackageAndUidLocked(pkgName, uidRemoved, + JobParameters.STOP_REASON_USER, "app uninstalled"); for (int c = 0; c < mControllers.size(); ++c) { mControllers.get(c).onAppRemovedLocked(pkgName, pkgUid); } @@ -812,8 +815,8 @@ public class JobSchedulerService extends com.android.server.SystemService if (DEBUG) { Slog.d(TAG, "Removing jobs for user: " + userId); } - cancelJobsForUser(userId); synchronized (mLock) { + cancelJobsForUserLocked(userId); for (int c = 0; c < mControllers.size(); ++c) { mControllers.get(c).onUserRemovedLocked(userId); } @@ -844,8 +847,10 @@ public class JobSchedulerService extends com.android.server.SystemService if (DEBUG) { Slog.d(TAG, "Removing jobs for pkg " + pkgName + " at uid " + pkgUid); } - cancelJobsForPackageAndUid(pkgName, pkgUid, - JobParameters.STOP_REASON_USER, "app force stopped"); + synchronized (mLock) { + cancelJobsForPackageAndUidLocked(pkgName, pkgUid, + JobParameters.STOP_REASON_USER, "app force stopped"); + } } } } @@ -1106,16 +1111,14 @@ public class JobSchedulerService extends com.android.server.SystemService } } - void cancelJobsForUser(int userHandle) { - synchronized (mLock) { - final List jobsForUser = mJobs.getJobsByUser(userHandle); - for (int i=0; i jobsForUser = mJobs.getJobsByUser(userHandle); + for (int i = 0; i < jobsForUser.size(); i++) { + JobStatus toRemove = jobsForUser.get(i); + // By the time we get here, the process should have already been stopped, so the + // app wouldn't get the stop reason, so just put USER instead of UNINSTALL. + cancelJobImplLocked(toRemove, null, JobParameters.STOP_REASON_USER, + "user removed"); } } @@ -1126,19 +1129,17 @@ public class JobSchedulerService extends com.android.server.SystemService } } - void cancelJobsForPackageAndUid(String pkgName, int uid, @JobParameters.StopReason int reason, - String debugReason) { + private void cancelJobsForPackageAndUidLocked(String pkgName, int uid, + @JobParameters.StopReason int reason, String debugReason) { if ("android".equals(pkgName)) { Slog.wtfStack(TAG, "Can't cancel all jobs for system package"); return; } - synchronized (mLock) { - final List jobsForUid = mJobs.getJobsByUid(uid); - for (int i = jobsForUid.size() - 1; i >= 0; i--) { - final JobStatus job = jobsForUid.get(i); - if (job.getSourcePackageName().equals(pkgName)) { - cancelJobImplLocked(job, null, reason, debugReason); - } + final List jobsForUid = mJobs.getJobsByUid(uid); + for (int i = jobsForUid.size() - 1; i >= 0; i--) { + final JobStatus job = jobsForUid.get(i); + if (job.getSourcePackageName().equals(pkgName)) { + cancelJobImplLocked(job, null, reason, debugReason); } } } diff --git a/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java b/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java index 1e78ec33efa40..36d3f165c1ac1 100644 --- a/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java +++ b/apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java @@ -226,9 +226,22 @@ public final class ConnectivityController extends RestrictingController implemen jobs.remove(jobStatus); } UidStats us = mUidStats.get(jobStatus.getSourceUid()); - us.numReadyWithConnectivity--; - if (jobStatus.madeActive != 0) { - us.numRunning--; + if (us == null) { + // This shouldn't be happening. We create a UidStats object for the app when the + // first job is scheduled in maybeStartTrackingJobLocked() and only ever drop the + // object if the app is uninstalled or the user is removed. That means that if we + // end up in this situation, onAppRemovedLocked() or onUserRemovedLocked() was + // called before maybeStopTrackingJobLocked(), which is the reverse order of what + // JobSchedulerService does (JSS calls maybeStopTrackingJobLocked() for all jobs + // before calling onAppRemovedLocked() or onUserRemovedLocked()). + Slog.wtfStack(TAG, + "UidStats was null after job for " + jobStatus.getSourcePackageName() + + " was registered"); + } else { + us.numReadyWithConnectivity--; + if (jobStatus.madeActive != 0) { + us.numRunning--; + } } maybeRevokeStandbyExceptionLocked(jobStatus); maybeAdjustRegisteredCallbacksLocked(); @@ -773,22 +786,22 @@ public final class ConnectivityController extends RestrictingController implemen * @param filterNetwork only update jobs that would use this * {@link Network}, or {@code null} to update all tracked jobs. */ - private void updateTrackedJobs(int filterUid, Network filterNetwork) { - synchronized (mLock) { - boolean changed = false; - if (filterUid == -1) { - for (int i = mTrackedJobs.size() - 1; i >= 0; i--) { - changed |= updateTrackedJobsLocked(mTrackedJobs.valueAt(i), filterNetwork); - } - } else { - changed = updateTrackedJobsLocked(mTrackedJobs.get(filterUid), filterNetwork); - } - if (changed) { - mStateChangedListener.onControllerStateChanged(); + @GuardedBy("mLock") + private void updateTrackedJobsLocked(int filterUid, Network filterNetwork) { + boolean changed = false; + if (filterUid == -1) { + for (int i = mTrackedJobs.size() - 1; i >= 0; i--) { + changed |= updateTrackedJobsLocked(mTrackedJobs.valueAt(i), filterNetwork); } + } else { + changed = updateTrackedJobsLocked(mTrackedJobs.get(filterUid), filterNetwork); + } + if (changed) { + mStateChangedListener.onControllerStateChanged(); } } + @GuardedBy("mLock") private boolean updateTrackedJobsLocked(ArraySet jobs, Network filterNetwork) { if (jobs == null || jobs.size() == 0) { return false; @@ -875,9 +888,9 @@ public final class ConnectivityController extends RestrictingController implemen } synchronized (mLock) { mAvailableNetworks.put(network, capabilities); + updateTrackedJobsLocked(-1, network); + maybeAdjustRegisteredCallbacksLocked(); } - updateTrackedJobs(-1, network); - maybeAdjustRegisteredCallbacksLocked(); } @Override @@ -893,9 +906,9 @@ public final class ConnectivityController extends RestrictingController implemen callback.mDefaultNetwork = null; } } + updateTrackedJobsLocked(-1, network); + maybeAdjustRegisteredCallbacksLocked(); } - updateTrackedJobs(-1, network); - maybeAdjustRegisteredCallbacksLocked(); } }; @@ -909,7 +922,7 @@ public final class ConnectivityController extends RestrictingController implemen synchronized (mLock) { switch (msg.what) { case MSG_REEVALUATE_JOBS: - updateTrackedJobs(-1, null); + updateTrackedJobsLocked(-1, null); break; } } @@ -949,8 +962,8 @@ public final class ConnectivityController extends RestrictingController implemen synchronized (mLock) { mDefaultNetwork = network; mBlocked = blocked; + updateTrackedJobsLocked(mUid, network); } - updateTrackedJobs(mUid, network); } // Network transitions have some complicated behavior that JS doesn't handle very well. @@ -993,8 +1006,8 @@ public final class ConnectivityController extends RestrictingController implemen if (Objects.equals(mDefaultNetwork, network)) { mDefaultNetwork = null; } + updateTrackedJobsLocked(mUid, network); } - updateTrackedJobs(mUid, network); } private void dumpLocked(IndentingPrintWriter pw) {