Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 626ec290 authored by Kweku Adams's avatar Kweku Adams
Browse files

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
parent ddf368aa

apex/jobscheduler/service/java/com/android/server/job/JobSchedulerService.java

+30 −29
Original line number Diff line number Diff line
@@ -746,12 +746,15 @@ 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,

                                        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) {

                                    /*

                                     * IllegalArgumentException means that the package doesn't exist.
@@ -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,11 +847,13 @@ public class JobSchedulerService extends com.android.server.SystemService 
                    if (DEBUG) {

                        Slog.d(TAG, "Removing jobs for pkg " + pkgName + " at uid " + pkgUid);

                    }

                    cancelJobsForPackageAndUid(pkgName, pkgUid,

                    synchronized (mLock) {

                        cancelJobsForPackageAndUidLocked(pkgName, pkgUid,

                                JobParameters.STOP_REASON_USER, "app force stopped");

                    }

                }

            }

        }

    };



    private String getPackageName(Intent intent) {
@@ -1106,8 +1111,7 @@ public class JobSchedulerService extends com.android.server.SystemService 
        }

    }



    void cancelJobsForUser(int userHandle) {

        synchronized (mLock) {

    private void cancelJobsForUserLocked(int userHandle) {

        final List<JobStatus> jobsForUser = mJobs.getJobsByUser(userHandle);

        for (int i = 0; i < jobsForUser.size(); i++) {

            JobStatus toRemove = jobsForUser.get(i);
@@ -1117,7 +1121,6 @@ public class JobSchedulerService extends com.android.server.SystemService 
                    "user removed");

        }

    }

    }



    private void cancelJobsForNonExistentUsers() {

        UserManagerInternal umi = LocalServices.getService(UserManagerInternal.class);
@@ -1126,13 +1129,12 @@ 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<JobStatus> jobsForUid = mJobs.getJobsByUid(uid);

        for (int i = jobsForUid.size() - 1; i >= 0; i--) {

            final JobStatus job = jobsForUid.get(i);
@@ -1141,7 +1143,6 @@ public class JobSchedulerService extends com.android.server.SystemService 
            }

        }

    }

    }



    /**

     * Entry point from client to cancel all jobs originating from their uid.

apex/jobscheduler/service/java/com/android/server/job/controllers/ConnectivityController.java

+35 −22
Original line number Diff line number Diff line
@@ -226,10 +226,23 @@ public final class ConnectivityController extends RestrictingController implemen 
                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--;

                }

            }

            maybeRevokeStandbyExceptionLocked(jobStatus);

            maybeAdjustRegisteredCallbacksLocked();

        }
@@ -773,8 +786,8 @@ 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) {

    @GuardedBy("mLock")

    private void updateTrackedJobsLocked(int filterUid, Network filterNetwork) {

        boolean changed = false;

        if (filterUid == -1) {

            for (int i = mTrackedJobs.size() - 1; i >= 0; i--) {
@@ -787,8 +800,8 @@ public final class ConnectivityController extends RestrictingController implemen 
            mStateChangedListener.onControllerStateChanged();

        }

    }

    }



    @GuardedBy("mLock")

    private boolean updateTrackedJobsLocked(ArraySet<JobStatus> jobs, Network filterNetwork) {

        if (jobs == null || jobs.size() == 0) {

            return false;
@@ -875,10 +888,10 @@ public final class ConnectivityController extends RestrictingController implemen 
            }

            synchronized (mLock) {

                mAvailableNetworks.put(network, capabilities);

            }

            updateTrackedJobs(-1, network);

                updateTrackedJobsLocked(-1, network);

                maybeAdjustRegisteredCallbacksLocked();

            }

        }



        @Override

        public void onLost(Network network) {
@@ -893,10 +906,10 @@ public final class ConnectivityController extends RestrictingController implemen 
                        callback.mDefaultNetwork = null;

                    }

                }

            }

            updateTrackedJobs(-1, network);

                updateTrackedJobsLocked(-1, network);

                maybeAdjustRegisteredCallbacksLocked();

            }

        }

    };



    private class CcHandler extends Handler {
@@ -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) {