diff options
7 files changed, 92 insertions, 12 deletions
diff --git a/dexopt_chroot_setup/binder/com/android/server/art/IDexoptChrootSetup.aidl b/dexopt_chroot_setup/binder/com/android/server/art/IDexoptChrootSetup.aidl index f8770147fc..867fdf9e38 100644 --- a/dexopt_chroot_setup/binder/com/android/server/art/IDexoptChrootSetup.aidl +++ b/dexopt_chroot_setup/binder/com/android/server/art/IDexoptChrootSetup.aidl @@ -38,6 +38,12 @@ interface IDexoptChrootSetup { */ void init(); - /** Tears down the chroot environment. */ - void tearDown(); + /** + * Tears down the chroot environment. + * + * @param allowConcurrent If true, allows this method to be called concurrently when another + * call to the service is still being processed. Note that the service does not process this + * call concurrently but waits until the other call is done. + */ + void tearDown(boolean allowConcurrent); } diff --git a/dexopt_chroot_setup/dexopt_chroot_setup.cc b/dexopt_chroot_setup/dexopt_chroot_setup.cc index ff016d64d5..1710f07b5f 100644 --- a/dexopt_chroot_setup/dexopt_chroot_setup.cc +++ b/dexopt_chroot_setup/dexopt_chroot_setup.cc @@ -365,9 +365,16 @@ ScopedAStatus DexoptChrootSetup::init() { return ScopedAStatus::ok(); } -ScopedAStatus DexoptChrootSetup::tearDown() { - if (!mu_.try_lock()) { - return Fatal("Unexpected concurrent calls"); +ScopedAStatus DexoptChrootSetup::tearDown(bool in_allowConcurrent) { + if (in_allowConcurrent) { + // Normally, we don't expect concurrent calls, but this method may be called upon system server + // restart when another call initiated by the previous system_server instance is still being + // processed. + mu_.lock(); + } else { + if (!mu_.try_lock()) { + return Fatal("Unexpected concurrent calls"); + } } std::lock_guard<std::mutex> lock(mu_, std::adopt_lock); diff --git a/dexopt_chroot_setup/dexopt_chroot_setup.h b/dexopt_chroot_setup/dexopt_chroot_setup.h index de0e35fa4d..3e39be4bb8 100644 --- a/dexopt_chroot_setup/dexopt_chroot_setup.h +++ b/dexopt_chroot_setup/dexopt_chroot_setup.h @@ -35,7 +35,7 @@ class DexoptChrootSetup : public aidl::com::android::server::art::BnDexoptChroot ndk::ScopedAStatus init() override; - ndk::ScopedAStatus tearDown() override; + ndk::ScopedAStatus tearDown(bool in_allowConcurrent) override; android::base::Result<void> Start(); diff --git a/dexopt_chroot_setup/dexopt_chroot_setup_test.cc b/dexopt_chroot_setup/dexopt_chroot_setup_test.cc index 1b4186f0dd..c5faca7f4e 100644 --- a/dexopt_chroot_setup/dexopt_chroot_setup_test.cc +++ b/dexopt_chroot_setup/dexopt_chroot_setup_test.cc @@ -89,7 +89,7 @@ class DexoptChrootSetupTest : public CommonArtTest { return; } scratch_dir_.reset(); - dexopt_chroot_setup_->tearDown(); + dexopt_chroot_setup_->tearDown(/*in_allowConcurrent=*/false); CommonArtTest::TearDown(); } @@ -184,17 +184,17 @@ TEST_F(DexoptChrootSetupTest, Run) { EXPECT_EQ(status.getExceptionCode(), EX_ILLEGAL_STATE); EXPECT_STREQ(status.getMessage(), "init must not be repeatedly called"); - ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown()); + ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown(/*in_allowConcurrent=*/false)); EXPECT_FALSE(std::filesystem::exists(DexoptChrootSetup::CHROOT_DIR)); // Check that `tearDown` can be repeatedly called too. - ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown()); + ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown(/*in_allowConcurrent=*/false)); // Check that `setUp` can be followed directly by a `tearDown`. ASSERT_STATUS_OK( dexopt_chroot_setup_->setUp(/*in_otaSlot=*/std::nullopt, /*in_mapSnapshotsForOta=*/false)); - ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown()); + ASSERT_STATUS_OK(dexopt_chroot_setup_->tearDown(/*in_allowConcurrent=*/false)); EXPECT_FALSE(std::filesystem::exists(DexoptChrootSetup::CHROOT_DIR)); } diff --git a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java index 63700648a0..83336805d7 100644 --- a/libartservice/service/java/com/android/server/art/ArtManagerLocal.java +++ b/libartservice/service/java/com/android/server/art/ArtManagerLocal.java @@ -1549,6 +1549,14 @@ public final class ArtManagerLocal { getDexUseManager(); getStorageManager(); GlobalInjector.getInstance().checkArtModuleServiceManager(); + + // `PreRebootDexoptJob` does not depend on external dependencies, so unlike the calls + // above, this call is not for checking the dependencies. Rather, we make this call here + // to trigger the construction of `PreRebootDexoptJob`, which may clean up leftover + // chroot if there is any. + if (SdkLevel.isAtLeastV()) { + getPreRebootDexoptJob(); + } } @RequiresApi(Build.VERSION_CODES.UPSIDE_DOWN_CAKE) diff --git a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java index a7cc8331c1..c560300b48 100644 --- a/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java +++ b/libartservice/service/java/com/android/server/art/PreRebootDexoptJob.java @@ -48,6 +48,8 @@ import java.time.Duration; import java.util.Objects; import java.util.UUID; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; @@ -72,6 +74,7 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { @NonNull private final Injector mInjector; // Job state variables. The monitor of `this` is notified when `mRunningJob` is changed. + // `mRunningJob` and `mCancellationSignal` have the same nullness. @GuardedBy("this") @Nullable private CompletableFuture<Void> mRunningJob = null; @GuardedBy("this") @Nullable private CancellationSignal mCancellationSignal = null; @@ -93,6 +96,17 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { new ThreadPoolExecutor(1 /* corePoolSize */, 1 /* maximumPoolSize */, 60 /* keepAliveTime */, TimeUnit.SECONDS, new LinkedBlockingQueue<Runnable>()); + /** + * A separate thread for executing `mRunningJob`. We avoid using any known thread / thread pool + * such as {@link java.util.concurrent.ForkJoinPool} and {@link + * com.android.internal.os.BackgroundThread} because we don't want to block other things that + * use known threads / thread pools. + */ + @NonNull + private final ThreadPoolExecutor mExecutor = + 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. @@ -106,6 +120,10 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { mInjector = injector; // Recycle the thread if it's not used for `keepAliveTime`. mSerializedExecutor.allowsCoreThreadTimeOut(); + mExecutor.allowsCoreThreadTimeOut(); + if (hasStarted()) { + maybeCleanUpChrootAsyncForStartup(); + } } @Override @@ -209,6 +227,31 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { cancelAnyLocked(); } + /** Cleans up chroot if it exists. Only expected to be called on system server startup. */ + private synchronized void maybeCleanUpChrootAsyncForStartup() { + // We only get here when there was a system server restart (probably due to a crash). In + // this case, it's possible that a previous Pre-reboot Dexopt job didn't end normally and + // left over a chroot, so we need to clean it up. + // We assign this operation to `mRunningJob` to block other operations on their calls to + // `cancelAnyLocked`. + // `mCancellationSignal` is a placeholder and the signal actually ignored. It's created just + // for keeping the invariant that `mRunningJob` and `mCancellationSignal` have the same + // nullness, to make other code simpler. + mCancellationSignal = new CancellationSignal(); + mRunningJob = new CompletableFuture().runAsync(() -> { + try { + mInjector.getPreRebootDriver().maybeCleanUpChroot(); + } finally { + synchronized (this) { + mRunningJob = null; + mCancellationSignal = null; + this.notifyAll(); + } + } + }, mExecutor); + this.notifyAll(); + } + @VisibleForTesting public synchronized void waitForRunningJob() { while (mRunningJob != null) { @@ -322,7 +365,7 @@ public class PreRebootDexoptJob implements ArtServiceJobInterface { this.notifyAll(); } } - }); + }, mExecutor); this.notifyAll(); return mRunningJob; } diff --git a/libartservice/service/java/com/android/server/art/prereboot/PreRebootDriver.java b/libartservice/service/java/com/android/server/art/prereboot/PreRebootDriver.java index a30ca4ab74..abcaa488f1 100644 --- a/libartservice/service/java/com/android/server/art/prereboot/PreRebootDriver.java +++ b/libartservice/service/java/com/android/server/art/prereboot/PreRebootDriver.java @@ -55,6 +55,8 @@ import java.io.FileOutputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.nio.file.Files; +import java.nio.file.Paths; /** * Drives Pre-reboot Dexopt, through reflection. @@ -158,6 +160,20 @@ public class PreRebootDriver { } } + public void maybeCleanUpChroot() { + if (!Files.exists(Paths.get(CHROOT_DIR))) { + return; + } + try { + ArtJni.ensureNoProcessInDir(CHROOT_DIR, 5000 /* timeoutMs */); + mInjector.getDexoptChrootSetup().tearDown(true /* allowConcurrent */); + } catch (RemoteException e) { + Utils.logArtdException(e); + } catch (ServiceSpecificException | IOException e) { + AsLog.e("Failed to clean up leftover chroot", e); + } + } + private void setUp(@Nullable String otaSlot, boolean mapSnapshotsForOta) throws RemoteException, SystemRequirementException { mInjector.getDexoptChrootSetup().setUp(otaSlot, mapSnapshotsForOta); @@ -182,7 +198,7 @@ public class PreRebootDriver { // blocks on `artd` calls, even upon cancellation, and `artd` in turn waits for child // processes to exit, even if they are killed due to the cancellation. ArtJni.ensureNoProcessInDir(CHROOT_DIR, 5000 /* timeoutMs */); - mInjector.getDexoptChrootSetup().tearDown(); + mInjector.getDexoptChrootSetup().tearDown(false /* allowConcurrent */); } private void runFromChroot(@NonNull CancellationSignal cancellationSignal) |