Offload `onStartJob` and `onStopJob` calls from the main thread.

The job scheduler calls `onStartJob` and `onStopJob` on system_server's
main thread. Our handlers can take long, to synchronize on the lock and
to wait for cancellation to be done, causing device freezing.

This change is a low-risk change that offloads the calls from the main
thread while keeping the execution order as the main thread does.

Bug: 350999413
Test: atest ArtServiceTests:com.android.server.art.PreRebootDexoptJobTest --iterations 10
Test: Run Pre-reboot Dexopt and cancel it.
Change-Id: I270de362f69a1f89e843e219f76b2e1f8b5bda61

2 files changed, 54 insertions, 21 deletions

diff --git a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java
index 5c4a9093ca..a7cc8331c1 100644
--- a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java
+++ b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java
@@ -48,6 +48,9 @@ import java.time.Duration;
 import java.util.Objects;
 import java.util.UUID;
 import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 /**
  * The Pre-reboot Dexopt job.
@@ -81,6 +84,15 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface {
     /** Whether to map/unmap snapshots. Only applicable to an OTA update. */
     @GuardedBy("this") private boolean mMapSnapshotsForOta = false;
 
+    /**
+     * Offloads `onStartJob` and `onStopJob` calls from the main thread while keeping the execution
+     * order as the main thread does.
+     */
+    @NonNull
+    private final ThreadPoolExecutor mSerializedExecutor =
+            new ThreadPoolExecutor(1 /* corePoolSize */, 1 /* maximumPoolSize */,
+                    60 /* keepAliveTime */, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>());
+
     // Mutations to the global state of Pre-reboot Dexopt, including mounts, staged files, and
     // stats, should only be done when there is no job running and the `this` lock is held, or by
     // the job itself.
@@ -92,17 +104,28 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface {
     @VisibleForTesting
     public PreRebootDexoptJob(@NonNull Injector injector) {
         mInjector = injector;
+        // Recycle the thread if it's not used for `keepAliveTime`.
+        mSerializedExecutor.allowsCoreThreadTimeOut();
     }
 
     @Override
-    public synchronized boolean onStartJob(
+    public boolean onStartJob(
+            @NonNull BackgroundDexoptJobService jobService, @NonNull JobParameters params) {
+        mSerializedExecutor.execute(() -> onStartJobImpl(jobService, params));
+        // "true" means the job will continue running until `jobFinished` is called.
+        return true;
+    }
+
+    @VisibleForTesting
+    public synchronized void onStartJobImpl(
             @NonNull BackgroundDexoptJobService jobService, @NonNull JobParameters params) {
         JobInfo pendingJob = mInjector.getJobScheduler().getPendingJob(JOB_ID);
         if (pendingJob == null
                 || !params.getExtras().getString("ticket").equals(
                         pendingJob.getExtras().getString("ticket"))) {
             // Job expired. We can only get here due to a race, and this should be very rare.
-            return false;
+            Utils.check(!mIsRunningJobKnownByJobScheduler);
+            return;
         }
 
         mIsRunningJobKnownByJobScheduler = true;
@@ -118,17 +141,20 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface {
         // No need to handle exceptions thrown by the future because exceptions are handled inside
         // the future itself.
         startLocked(onJobFinishedLocked);
-        // "true" means the job will continue running until `jobFinished` is called.
-        return true;
     }
 
     @Override
-    public synchronized boolean onStopJob(@NonNull JobParameters params) {
+    public boolean onStopJob(@NonNull JobParameters params) {
+        mSerializedExecutor.execute(() -> onStopJobImpl(params));
+        // "true" means to execute again with the default retry policy.
+        return true;
+    }
+
+    @VisibleForTesting
+    public synchronized void onStopJobImpl(@NonNull JobParameters params) {
         if (mIsRunningJobKnownByJobScheduler) {
             cancelGivenLocked(mRunningJob, false /* expectInterrupt */);
         }
-        // "true" means to execute again with the default retry policy.
-        return true;
     }
 
     /**
@@ -194,6 +220,11 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface {
         }
     }
 
+    @VisibleForTesting
+    public synchronized boolean hasRunningJob() {
+        return mRunningJob != null;
+    }
+
     @GuardedBy("this")
     private @ScheduleStatus int scheduleLocked() {
         if (this != BackgroundDexoptJobService.getJob(JOB_ID)) {
diff --git a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java
index 7103d4023b..fc83a32c9a 100644
--- a/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java
+++ b/libartservice/service/javatests/com/android/server/art/PreRebootDexoptJobTest.java
@@ -249,7 +249,7 @@ public class PreRebootDexoptJobTest {
 
         assertThat(mPreRebootDexoptJob.hasStarted()).isFalse();
         mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */);
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
         assertThat(jobStarted.tryAcquire(TIMEOUT_SEC, TimeUnit.SECONDS)).isTrue();
         assertThat(mPreRebootDexoptJob.hasStarted()).isTrue();
 
@@ -280,8 +280,8 @@ public class PreRebootDexoptJobTest {
         });
 
         mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */);
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
-        mPreRebootDexoptJob.onStopJob(mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStopJobImpl(mJobParameters);
 
         // Check that `onStopJob` is really blocking. If it wasn't, the check below might still pass
         // due to a race, but we would have a flaky test.
@@ -316,7 +316,7 @@ public class PreRebootDexoptJobTest {
 
         when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true);
 
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
         mPreRebootDexoptJob.waitForRunningJob();
     }
 
@@ -327,7 +327,7 @@ public class PreRebootDexoptJobTest {
 
         when(mPreRebootDriver.run(eq("_a"), anyBoolean(), any())).thenReturn(true);
 
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
         mPreRebootDexoptJob.waitForRunningJob();
     }
 
@@ -338,7 +338,7 @@ public class PreRebootDexoptJobTest {
 
         when(mPreRebootDriver.run(isNull(), anyBoolean(), any())).thenReturn(true);
 
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
         mPreRebootDexoptJob.waitForRunningJob();
     }
 
@@ -349,7 +349,7 @@ public class PreRebootDexoptJobTest {
 
         when(mPreRebootDriver.run(eq("_b"), anyBoolean(), any())).thenReturn(true);
 
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
         mPreRebootDexoptJob.waitForRunningJob();
     }
 
@@ -382,7 +382,7 @@ public class PreRebootDexoptJobTest {
         mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */);
 
         // The job scheduler starts the job.
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
 
         var jobFinishedCalledAfterNewJobStarted = new Semaphore(0);
 
@@ -395,10 +395,10 @@ public class PreRebootDexoptJobTest {
             // The job scheduler tries to cancel the old job because of the new update. This call
             // doesn't matter because the job has already been cancelled by ourselves during the
             // `onUpdateReady` call above.
-            mPreRebootDexoptJob.onStopJob(oldParameters);
+            mPreRebootDexoptJob.onStopJobImpl(oldParameters);
 
             // The job scheduler starts the new job.
-            mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+            mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
 
             doAnswer(invocation -> {
                 jobFinishedCalledAfterNewJobStarted.release();
@@ -441,10 +441,12 @@ public class PreRebootDexoptJobTest {
         // lock, the order of execution may be reversed. When this happens, the `onStartJob` request
         // should not succeed.
         mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */);
-        assertThat(mPreRebootDexoptJob.onStartJob(mJobService, oldParameters)).isFalse();
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, oldParameters);
+        assertThat(mPreRebootDexoptJob.hasRunningJob()).isFalse();
 
         // The job scheduler starts the new job. This request should succeed.
-        assertThat(mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters)).isTrue();
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
+        assertThat(mPreRebootDexoptJob.hasRunningJob()).isTrue();
     }
 
     /**
@@ -466,7 +468,7 @@ public class PreRebootDexoptJobTest {
         mPreRebootDexoptJob.onUpdateReady(null /* otaSlot */);
 
         // The job scheduler starts the job.
-        mPreRebootDexoptJob.onStartJob(mJobService, mJobParameters);
+        mPreRebootDexoptJob.onStartJobImpl(mJobService, mJobParameters);
 
         // Another update arrives, requesting a synchronous job run, replacing the old job. The new
         // job, which is synchronous, is started right after the old job is cancelled by
@@ -482,7 +484,7 @@ public class PreRebootDexoptJobTest {
         // The `onStopJob` call finally arrives. This call should be a no-op because the job has
         // already been cancelled by ourselves during the `onUpdateReadyStartNow` call above. It
         // should not cancel the new job.
-        mPreRebootDexoptJob.onStopJob(oldParameters);
+        mPreRebootDexoptJob.onStopJobImpl(oldParameters);
 
         // The new job should not be cancelled.
         assertThat(jobExited.tryAcquire()).isFalse();