From b2b41b2825057bfebd966b3ae0a5225a01565ba3 Mon Sep 17 00:00:00 2001 From: Kweku Adams Date: Tue, 30 Mar 2021 18:17:38 -0700 Subject: [PATCH] Ensure UidStats is not null when needed. We're sometimes being told that an app has been uninstalled before the process has stopped. That puts JS and especially ConnectivityController in an inconsistent state. Make sure we don't crash because of the inconsistency. Bug: 183921387 Bug: 184098842 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: Ied17a461a47160c81042676f43d7f8defe1c4466 --- .../controllers/ConnectivityController.java | 59 +++++++++++-------- 1 file changed, 34 insertions(+), 25 deletions(-) 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 36d3f165c1ac1..c9e1e002b7f11 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 @@ -188,11 +188,8 @@ public final class ConnectivityController extends RestrictingController implemen @Override public void maybeStartTrackingJobLocked(JobStatus jobStatus, JobStatus lastJob) { if (jobStatus.hasConnectivityConstraint()) { - UidStats uidStats = mUidStats.get(jobStatus.getSourceUid()); - if (uidStats == null) { - uidStats = new UidStats(jobStatus.getSourceUid()); - mUidStats.append(jobStatus.getSourceUid(), uidStats); - } + final UidStats uidStats = + getUidStats(jobStatus.getSourceUid(), jobStatus.getSourcePackageName(), false); if (wouldBeReadyWithConstraintLocked(jobStatus, JobStatus.CONSTRAINT_CONNECTIVITY)) { uidStats.numReadyWithConnectivity++; } @@ -211,7 +208,8 @@ public final class ConnectivityController extends RestrictingController implemen @Override public void prepareForExecutionLocked(JobStatus jobStatus) { if (jobStatus.hasConnectivityConstraint()) { - UidStats uidStats = mUidStats.get(jobStatus.getSourceUid()); + final UidStats uidStats = + getUidStats(jobStatus.getSourceUid(), jobStatus.getSourcePackageName(), true); uidStats.numRunning++; } } @@ -225,23 +223,11 @@ public final class ConnectivityController extends RestrictingController implemen if (jobs != null) { jobs.remove(jobStatus); } - UidStats us = mUidStats.get(jobStatus.getSourceUid()); - 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--; - } + final UidStats uidStats = + getUidStats(jobStatus.getSourceUid(), jobStatus.getSourcePackageName(), true); + uidStats.numReadyWithConnectivity--; + if (jobStatus.madeActive != 0) { + uidStats.numRunning--; } maybeRevokeStandbyExceptionLocked(jobStatus); maybeAdjustRegisteredCallbacksLocked(); @@ -266,6 +252,27 @@ public final class ConnectivityController extends RestrictingController implemen } } + @NonNull + private UidStats getUidStats(int uid, String packageName, boolean shouldExist) { + UidStats us = mUidStats.get(uid); + if (us == null) { + if (shouldExist) { + // 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 " + packageName + " was registered"); + } + us = new UidStats(uid); + mUidStats.append(uid, us); + } + return us; + } + /** * Returns true if the job's requested network is available. This DOES NOT necessarily mean * that the UID has been granted access to the network. @@ -332,7 +339,8 @@ public final class ConnectivityController extends RestrictingController implemen return; } - UidStats uidStats = mUidStats.get(jobStatus.getSourceUid()); + final UidStats uidStats = + getUidStats(jobStatus.getSourceUid(), jobStatus.getSourcePackageName(), true); if (jobStatus.shouldTreatAsExpeditedJob()) { if (!jobStatus.isConstraintSatisfied(JobStatus.CONSTRAINT_CONNECTIVITY)) { @@ -584,7 +592,8 @@ public final class ConnectivityController extends RestrictingController implemen if (mCurrentDefaultNetworkCallbacks.contains(sourceUid)) { return; } - UidStats uidStats = mUidStats.get(sourceUid); + final UidStats uidStats = + getUidStats(jobStatus.getSourceUid(), jobStatus.getSourcePackageName(), true); if (!mSortedStats.contains(uidStats)) { mSortedStats.add(uidStats); }