Fix lock ordering in JobScheduler

BUG: 17625667
Two part clean-up.
1) Don't try to lock in onControllerStateChanged. Do it in the handleMessage
instead where the rest of the locking is. This is sufficient to fix this bug.
2) The other side of the deadlock came b/c we lock when cancelling and calling
stopTrackingJob. Controllers handle their own locking so this isn't
necessary. B/c of a potential race from the controller side, added an explicit
check for the JSS to only run an expired job if it still exists.

Change-Id: Iaeebbc19437eb5b73e3ced3168f1fc13e564a4be
diff --git a/services/core/java/com/android/server/job/JobSchedulerService.java b/services/core/java/com/android/server/job/JobSchedulerService.java
index 30154d7..f456bcd 100644
--- a/services/core/java/com/android/server/job/JobSchedulerService.java
+++ b/services/core/java/com/android/server/job/JobSchedulerService.java
@@ -195,12 +195,13 @@
     }
 
     private void cancelJobsForUser(int userHandle) {
+        List<JobStatus> jobsForUser;
         synchronized (mJobs) {
-            List<JobStatus> jobsForUser = mJobs.getJobsByUser(userHandle);
-            for (int i=0; i<jobsForUser.size(); i++) {
-                JobStatus toRemove = jobsForUser.get(i);
-                cancelJobLocked(toRemove);
-            }
+            jobsForUser = mJobs.getJobsByUser(userHandle);
+        }
+        for (int i=0; i<jobsForUser.size(); i++) {
+            JobStatus toRemove = jobsForUser.get(i);
+            cancelJobImpl(toRemove);
         }
     }
 
@@ -208,16 +209,16 @@
      * Entry point from client to cancel all jobs originating from their uid.
      * This will remove the job from the master list, and cancel the job if it was staged for
      * execution or being executed.
-     * @param uid To check against for removal of a job.
+     * @param uid Uid to check against for removal of a job.
      */
     public void cancelJobsForUid(int uid) {
-        // Remove from master list.
+        List<JobStatus> jobsForUid;
         synchronized (mJobs) {
-            List<JobStatus> jobsForUid = mJobs.getJobsByUid(uid);
-            for (int i=0; i<jobsForUid.size(); i++) {
-                JobStatus toRemove = jobsForUid.get(i);
-                cancelJobLocked(toRemove);
-            }
+            jobsForUid = mJobs.getJobsByUid(uid);
+        }
+        for (int i=0; i<jobsForUid.size(); i++) {
+            JobStatus toRemove = jobsForUid.get(i);
+            cancelJobImpl(toRemove);
         }
     }
 
@@ -232,22 +233,23 @@
         JobStatus toCancel;
         synchronized (mJobs) {
             toCancel = mJobs.getJobByUidAndJobId(uid, jobId);
-            if (toCancel != null) {
-                cancelJobLocked(toCancel);
-            }
+        }
+        if (toCancel != null) {
+            cancelJobImpl(toCancel);
         }
     }
 
-    private void cancelJobLocked(JobStatus cancelled) {
+    private void cancelJobImpl(JobStatus cancelled) {
         if (DEBUG) {
             Slog.d(TAG, "Cancelling: " + cancelled);
         }
-        // Remove from store.
         stopTrackingJob(cancelled);
-        // Remove from pending queue.
-        mPendingJobs.remove(cancelled);
-        // Cancel if running.
-        stopJobOnServiceContextLocked(cancelled);
+        synchronized (mJobs) {
+            // Remove from pending queue.
+            mPendingJobs.remove(cancelled);
+            // Cancel if running.
+            stopJobOnServiceContextLocked(cancelled);
+        }
     }
 
     /**
@@ -482,21 +484,13 @@
     // StateChangedListener implementations.
 
     /**
-     * Off-board work to our handler thread as quickly as possible, b/c this call is probably being
-     * made on the main thread.
-     * For now this takes the job and if it's ready to run it will run it. In future we might not
-     * provide the job, so that the StateChangedListener has to run through its list of jobs to
-     * see which are ready. This will further decouple the controllers from the execution logic.
+     * Posts a message to the {@link com.android.server.job.JobSchedulerService.JobHandler} that
+     * some controller's state has changed, so as to run through the list of jobs and start/stop
+     * any that are eligible.
      */
     @Override
     public void onControllerStateChanged() {
-        synchronized (mJobs) {
-            if (mReadyToRock) {
-                // Post a message to to run through the list of jobs and start/stop any that
-                // are eligible.
-                mHandler.obtainMessage(MSG_CHECK_JOB).sendToTarget();
-            }
-        }
+        mHandler.obtainMessage(MSG_CHECK_JOB).sendToTarget();
     }
 
     @Override
@@ -512,21 +506,29 @@
 
         @Override
         public void handleMessage(Message message) {
+            synchronized (mJobs) {
+                if (!mReadyToRock) {
+                    return;
+                }
+            }
             switch (message.what) {
                 case MSG_JOB_EXPIRED:
                     synchronized (mJobs) {
                         JobStatus runNow = (JobStatus) message.obj;
                         // runNow can be null, which is a controller's way of indicating that its
                         // state is such that all ready jobs should be run immediately.
-                        if (runNow != null && !mPendingJobs.contains(runNow)) {
+                        if (runNow != null && !mPendingJobs.contains(runNow)
+                                && mJobs.containsJob(runNow)) {
                             mPendingJobs.add(runNow);
                         }
+                        queueReadyJobsForExecutionLockedH();
                     }
-                    queueReadyJobsForExecutionH();
                     break;
                 case MSG_CHECK_JOB:
-                    // Check the list of jobs and run some of them if we feel inclined.
-                    maybeQueueReadyJobsForExecutionH();
+                    synchronized (mJobs) {
+                        // Check the list of jobs and run some of them if we feel inclined.
+                        maybeQueueReadyJobsForExecutionLockedH();
+                    }
                     break;
             }
             maybeRunPendingJobsH();
@@ -538,30 +540,28 @@
          * Run through list of jobs and execute all possible - at least one is expired so we do
          * as many as we can.
          */
-        private void queueReadyJobsForExecutionH() {
-            synchronized (mJobs) {
-                ArraySet<JobStatus> jobs = mJobs.getJobs();
-                if (DEBUG) {
-                    Slog.d(TAG, "queuing all ready jobs for execution:");
-                }
-                for (int i=0; i<jobs.size(); i++) {
-                    JobStatus job = jobs.valueAt(i);
-                    if (isReadyToBeExecutedLocked(job)) {
-                        if (DEBUG) {
-                            Slog.d(TAG, "    queued " + job.toShortString());
-                        }
-                        mPendingJobs.add(job);
-                    } else if (isReadyToBeCancelledLocked(job)) {
-                        stopJobOnServiceContextLocked(job);
+        private void queueReadyJobsForExecutionLockedH() {
+            ArraySet<JobStatus> jobs = mJobs.getJobs();
+            if (DEBUG) {
+                Slog.d(TAG, "queuing all ready jobs for execution:");
+            }
+            for (int i=0; i<jobs.size(); i++) {
+                JobStatus job = jobs.valueAt(i);
+                if (isReadyToBeExecutedLocked(job)) {
+                    if (DEBUG) {
+                        Slog.d(TAG, "    queued " + job.toShortString());
                     }
+                    mPendingJobs.add(job);
+                } else if (isReadyToBeCancelledLocked(job)) {
+                    stopJobOnServiceContextLocked(job);
                 }
-                if (DEBUG) {
-                    final int queuedJobs = mPendingJobs.size();
-                    if (queuedJobs == 0) {
-                        Slog.d(TAG, "No jobs pending.");
-                    } else {
-                        Slog.d(TAG, queuedJobs + " jobs queued.");
-                    }
+            }
+            if (DEBUG) {
+                final int queuedJobs = mPendingJobs.size();
+                if (queuedJobs == 0) {
+                    Slog.d(TAG, "No jobs pending.");
+                } else {
+                    Slog.d(TAG, queuedJobs + " jobs queued.");
                 }
             }
         }
@@ -575,55 +575,53 @@
          * If more than 4 jobs total are ready we send them all off.
          * TODO: It would be nice to consolidate these sort of high-level policies somewhere.
          */
-        private void maybeQueueReadyJobsForExecutionH() {
-            synchronized (mJobs) {
-                int chargingCount = 0;
-                int idleCount =  0;
-                int backoffCount = 0;
-                int connectivityCount = 0;
-                List<JobStatus> runnableJobs = new ArrayList<JobStatus>();
-                ArraySet<JobStatus> jobs = mJobs.getJobs();
-                for (int i=0; i<jobs.size(); i++) {
-                    JobStatus job = jobs.valueAt(i);
-                    if (isReadyToBeExecutedLocked(job)) {
-                        if (job.getNumFailures() > 0) {
-                            backoffCount++;
-                        }
-                        if (job.hasIdleConstraint()) {
-                            idleCount++;
-                        }
-                        if (job.hasConnectivityConstraint() || job.hasUnmeteredConstraint()) {
-                            connectivityCount++;
-                        }
-                        if (job.hasChargingConstraint()) {
-                            chargingCount++;
-                        }
-                        runnableJobs.add(job);
-                    } else if (isReadyToBeCancelledLocked(job)) {
-                        stopJobOnServiceContextLocked(job);
+        private void maybeQueueReadyJobsForExecutionLockedH() {
+            int chargingCount = 0;
+            int idleCount =  0;
+            int backoffCount = 0;
+            int connectivityCount = 0;
+            List<JobStatus> runnableJobs = new ArrayList<JobStatus>();
+            ArraySet<JobStatus> jobs = mJobs.getJobs();
+            for (int i=0; i<jobs.size(); i++) {
+                JobStatus job = jobs.valueAt(i);
+                if (isReadyToBeExecutedLocked(job)) {
+                    if (job.getNumFailures() > 0) {
+                        backoffCount++;
                     }
+                    if (job.hasIdleConstraint()) {
+                        idleCount++;
+                    }
+                    if (job.hasConnectivityConstraint() || job.hasUnmeteredConstraint()) {
+                        connectivityCount++;
+                    }
+                    if (job.hasChargingConstraint()) {
+                        chargingCount++;
+                    }
+                    runnableJobs.add(job);
+                } else if (isReadyToBeCancelledLocked(job)) {
+                    stopJobOnServiceContextLocked(job);
                 }
-                if (backoffCount > 0 ||
-                        idleCount >= MIN_IDLE_COUNT ||
-                        connectivityCount >= MIN_CONNECTIVITY_COUNT ||
-                        chargingCount >= MIN_CHARGING_COUNT ||
-                        runnableJobs.size() >= MIN_READY_JOBS_COUNT) {
-                    if (DEBUG) {
-                        Slog.d(TAG, "maybeQueueReadyJobsForExecutionH: Running jobs.");
-                    }
-                    for (int i=0; i<runnableJobs.size(); i++) {
-                        mPendingJobs.add(runnableJobs.get(i));
-                    }
-                } else {
-                    if (DEBUG) {
-                        Slog.d(TAG, "maybeQueueReadyJobsForExecutionH: Not running anything.");
-                    }
-                }
+            }
+            if (backoffCount > 0 ||
+                    idleCount >= MIN_IDLE_COUNT ||
+                    connectivityCount >= MIN_CONNECTIVITY_COUNT ||
+                    chargingCount >= MIN_CHARGING_COUNT ||
+                    runnableJobs.size() >= MIN_READY_JOBS_COUNT) {
                 if (DEBUG) {
-                    Slog.d(TAG, "idle=" + idleCount + " connectivity=" +
-                    connectivityCount + " charging=" + chargingCount + " tot=" +
-                            runnableJobs.size());
+                    Slog.d(TAG, "maybeQueueReadyJobsForExecutionLockedH: Running jobs.");
                 }
+                for (int i=0; i<runnableJobs.size(); i++) {
+                    mPendingJobs.add(runnableJobs.get(i));
+                }
+            } else {
+                if (DEBUG) {
+                    Slog.d(TAG, "maybeQueueReadyJobsForExecutionLockedH: Not running anything.");
+                }
+            }
+            if (DEBUG) {
+                Slog.d(TAG, "idle=" + idleCount + " connectivity=" +
+                connectivityCount + " charging=" + chargingCount + " tot=" +
+                        runnableJobs.size());
             }
         }
 
diff --git a/services/core/java/com/android/server/job/JobStore.java b/services/core/java/com/android/server/job/JobStore.java
index df12e1a..b64c677 100644
--- a/services/core/java/com/android/server/job/JobStore.java
+++ b/services/core/java/com/android/server/job/JobStore.java
@@ -50,15 +50,9 @@
 import org.xmlpull.v1.XmlSerializer;
 
 /**
- * Maintain a list of classes, and accessor methods/logic for these jobs.
- * This class offers the following functionality:
- *     - When a job is added, it will determine if the job requirements have changed (update) and
- *       whether the controllers need to be updated.
- *     - Persists JobInfos, figures out when to to rewrite the JobInfo to disk.
- *     - Handles rescheduling of jobs.
- *       - When a periodic job is executed and must be re-added.
- *       - When a job fails and the client requests that it be retried with backoff.
- *       - This class <strong>is not</strong> thread-safe.
+ * Maintains the master list of jobs that the job scheduler is tracking. These jobs are compared by
+ * reference, so none of the functions in this class should make a copy.
+ * Also handles read/write of persisted jobs.
  *
  * Note on locking:
  *      All callers to this class must <strong>lock on the class object they are calling</strong>.
@@ -152,6 +146,10 @@
         return false;
     }
 
+    boolean containsJob(JobStatus jobStatus) {
+        return mJobSet.contains(jobStatus);
+    }
+
     public int size() {
         return mJobSet.size();
     }
@@ -180,6 +178,10 @@
         maybeWriteStatusToDiskAsync();
     }
 
+    /**
+     * @param userHandle User for whom we are querying the list of jobs.
+     * @return A list of all the jobs scheduled by the provided user. Never null.
+     */
     public List<JobStatus> getJobsByUser(int userHandle) {
         List<JobStatus> matchingJobs = new ArrayList<JobStatus>();
         Iterator<JobStatus> it = mJobSet.iterator();
@@ -194,7 +196,7 @@
 
     /**
      * @param uid Uid of the requesting app.
-     * @return All JobStatus objects for a given uid from the master list.
+     * @return All JobStatus objects for a given uid from the master list. Never null.
      */
     public List<JobStatus> getJobsByUid(int uid) {
         List<JobStatus> matchingJobs = new ArrayList<JobStatus>();
diff --git a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
index 402f0dd..bd64392 100644
--- a/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
+++ b/services/tests/servicestests/src/com/android/server/job/JobStoreTest.java
@@ -67,6 +67,7 @@
         assertEquals("Didn't get expected number of persisted tasks.", 1, jobStatusSet.size());
         final JobStatus loadedTaskStatus = jobStatusSet.iterator().next();
         assertTasksEqual(task, loadedTaskStatus.getJob());
+        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(ts));
         assertEquals("Different uids.", SOME_UID, loadedTaskStatus.getUid());
         compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
                 ts.getEarliestRunTime(), loadedTaskStatus.getEarliestRunTime());
@@ -103,7 +104,8 @@
         JobStatus loaded2 = it.next();
         assertTasksEqual(task1, loaded1.getJob());
         assertTasksEqual(task2, loaded2.getJob());
-
+        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus1));
+        assertTrue("JobStore#contains invalid.", mTaskStoreUnderTest.containsJob(taskStatus2));
         // Check that the loaded task has the correct runtimes.
         compareTimestampsSubjectToIoLatency("Early run-times not the same after read.",
                 taskStatus1.getEarliestRunTime(), loaded1.getEarliestRunTime());