Merge "[pm] unlock mInstallLock before dexopt" into main

7 files changed, 70 insertions, 32 deletions

diff --git a/services/core/java/com/android/server/pm/InstallPackageHelper.java b/services/core/java/com/android/server/pm/InstallPackageHelper.java
index 19a0ba796343..908b47df35e1 100644
--- a/services/core/java/com/android/server/pm/InstallPackageHelper.java
+++ b/services/core/java/com/android/server/pm/InstallPackageHelper.java
@@ -985,13 +985,13 @@ final class InstallPackageHelper {
     }
 
     void installPackagesTraced(List<InstallRequest> requests) {
-        synchronized (mPm.mInstallLock) {
-            try {
-                Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "installPackages");
-                installPackagesLI(requests);
-            } finally {
-                Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER);
-            }
+        mPm.mInstallLock.lock();
+        try {
+            Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "installPackages");
+            installPackagesLI(requests);
+        } finally {
+            Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER);
+            mPm.mInstallLock.unlock();
         }
     }
 
@@ -2590,22 +2590,30 @@ final class InstallPackageHelper {
             final boolean performDexopt = DexOptHelper.shouldPerformDexopt(installRequest,
                     dexoptOptions, mContext);
             if (performDexopt) {
-                Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "dexopt");
+                // dexopt can take long, and ArtService doesn't require installd, so we release
+                // the lock here and re-acquire the lock after dexopt is finished.
+                mPm.mInstallLock.unlock();
+                try {
+                    Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "dexopt");
 
-                // This mirrors logic from commitReconciledScanResultLocked, where the library files
-                // needed for dexopt are assigned.
-                PackageSetting realPkgSetting = installRequest.getRealPackageSetting();
+                    // This mirrors logic from commitReconciledScanResultLocked, where the library
+                    // files needed for dexopt are assigned.
+                    PackageSetting realPkgSetting = installRequest.getRealPackageSetting();
 
-                // Unfortunately, the updated system app flag is only tracked on this PackageSetting
-                boolean isUpdatedSystemApp =
-                        installRequest.getScannedPackageSetting().isUpdatedSystemApp();
+                    // Unfortunately, the updated system app flag is only tracked on this
+                    // PackageSetting
+                    boolean isUpdatedSystemApp =
+                            installRequest.getScannedPackageSetting().isUpdatedSystemApp();
 
-                realPkgSetting.getPkgState().setUpdatedSystemApp(isUpdatedSystemApp);
+                    realPkgSetting.getPkgState().setUpdatedSystemApp(isUpdatedSystemApp);
 
-                DexoptResult dexOptResult =
-                        DexOptHelper.dexoptPackageUsingArtService(installRequest, dexoptOptions);
-                installRequest.onDexoptFinished(dexOptResult);
-                Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER);
+                    DexoptResult dexOptResult = DexOptHelper.dexoptPackageUsingArtService(
+                            installRequest, dexoptOptions);
+                    installRequest.onDexoptFinished(dexOptResult);
+                    Trace.traceEnd(TRACE_TAG_PACKAGE_MANAGER);
+                } finally {
+                    mPm.mInstallLock.lock();
+                }
             }
         }
         PackageManagerServiceUtils.waitForNativeBinariesExtractionForIncremental(
diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index ae485ede1bec..121cf3f231b0 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -626,7 +626,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
     // Lock for state used when installing and doing other long running
     // operations.  Methods that must be called with this lock held have
     // the suffix "LI".
-    final Object mInstallLock;
+    final PackageManagerTracedLock mInstallLock;
 
     // ----------------------------------------------------------------
 
@@ -1692,8 +1692,8 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         final TimingsTraceAndSlog t = new TimingsTraceAndSlog(TAG + "Timing",
                 Trace.TRACE_TAG_PACKAGE_MANAGER);
         t.traceBegin("create package manager");
-        final PackageManagerTracedLock lock = new PackageManagerTracedLock();
-        final Object installLock = new Object();
+        final PackageManagerTracedLock lock = new PackageManagerTracedLock("mLock");
+        final PackageManagerTracedLock installLock = new PackageManagerTracedLock("mInstallLock");
 
         HandlerThread backgroundThread = new ServiceThread("PackageManagerBg",
                 Process.THREAD_PRIORITY_BACKGROUND, true /*allowIo*/);
diff --git a/services/core/java/com/android/server/pm/PackageManagerServiceInjector.java b/services/core/java/com/android/server/pm/PackageManagerServiceInjector.java
index 83f3b16b31d1..ae2eaeb31f81 100644
--- a/services/core/java/com/android/server/pm/PackageManagerServiceInjector.java
+++ b/services/core/java/com/android/server/pm/PackageManagerServiceInjector.java
@@ -86,7 +86,7 @@ public class PackageManagerServiceInjector {
     private final Context mContext;
     private final PackageManagerTracedLock mLock;
     private final Installer mInstaller;
-    private final Object mInstallLock;
+    private final PackageManagerTracedLock mInstallLock;
     private final Handler mBackgroundHandler;
     private final Executor mBackgroundExecutor;
     private final List<ScanPartition> mSystemPartitions;
@@ -144,7 +144,7 @@ public class PackageManagerServiceInjector {
     private final Singleton<PackageMonitorCallbackHelper> mPackageMonitorCallbackHelper;
 
     PackageManagerServiceInjector(Context context, PackageManagerTracedLock lock,
-            Installer installer, Object installLock, PackageAbiHelper abiHelper,
+            Installer installer, PackageManagerTracedLock installLock, PackageAbiHelper abiHelper,
             Handler backgroundHandler,
             List<ScanPartition> systemPartitions,
             Producer<ComponentResolver> componentResolverProducer,
@@ -254,7 +254,7 @@ public class PackageManagerServiceInjector {
         return mAbiHelper;
     }
 
-    public Object getInstallLock() {
+    public PackageManagerTracedLock getInstallLock() {
         return mInstallLock;
     }
 
diff --git a/services/core/java/com/android/server/pm/PackageManagerTracedLock.java b/services/core/java/com/android/server/pm/PackageManagerTracedLock.java
index 75e1803f1695..303b8b9dd817 100644
--- a/services/core/java/com/android/server/pm/PackageManagerTracedLock.java
+++ b/services/core/java/com/android/server/pm/PackageManagerTracedLock.java
@@ -16,6 +16,9 @@
 
 package com.android.server.pm;
 
+import android.annotation.Nullable;
+import android.util.Slog;
+
 import java.util.concurrent.locks.ReentrantLock;
 
 /**
@@ -23,4 +26,31 @@ import java.util.concurrent.locks.ReentrantLock;
  * injection, similar to {@link ActivityManagerGlobalLock}.
  */
 public class PackageManagerTracedLock extends ReentrantLock {
+    private static final String TAG = "PackageManagerTracedLock";
+    private static final boolean DEBUG = false;
+    @Nullable private final String mLockName;
+
+    public PackageManagerTracedLock(@Nullable String lockName) {
+        mLockName = lockName;
+    }
+
+    public PackageManagerTracedLock() {
+        this(null);
+    }
+
+    @Override
+    public void lock() {
+        super.lock();
+        if (DEBUG && mLockName != null) {
+            Slog.i(TAG, "locked " + mLockName);
+        }
+    }
+
+    @Override
+    public void unlock() {
+        super.unlock();
+        if (DEBUG && mLockName != null) {
+            Slog.i(TAG, "unlocked " + mLockName);
+        }
+    }
 }
diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java
index 1d414011cff3..ef32485567f8 100644
--- a/services/core/java/com/android/server/pm/UserDataPreparer.java
+++ b/services/core/java/com/android/server/pm/UserDataPreparer.java
@@ -52,11 +52,11 @@ class UserDataPreparer {
     private static final String TAG = "UserDataPreparer";
     private static final String XATTR_SERIAL = "user.serial";
 
-    private final Object mInstallLock;
+    private final PackageManagerTracedLock mInstallLock;
     private final Context mContext;
     private final Installer mInstaller;
 
-    UserDataPreparer(Installer installer, Object installLock, Context context) {
+    UserDataPreparer(Installer installer, PackageManagerTracedLock installLock, Context context) {
         mInstallLock = installLock;
         mContext = context;
         mInstaller = installer;
diff --git a/services/tests/PackageManagerServiceTests/server/src/com/android/server/pm/UserDataPreparerTest.java b/services/tests/PackageManagerServiceTests/server/src/com/android/server/pm/UserDataPreparerTest.java
index 9e11fa2f0bdf..e545a49d3139 100644
--- a/services/tests/PackageManagerServiceTests/server/src/com/android/server/pm/UserDataPreparerTest.java
+++ b/services/tests/PackageManagerServiceTests/server/src/com/android/server/pm/UserDataPreparerTest.java
@@ -71,7 +71,7 @@ public class UserDataPreparerTest {
     @Mock
     private Installer mInstaller;
 
-    private Object mInstallLock;
+    private PackageManagerTracedLock mInstallLock;
 
     @Before
     public void setup() {
@@ -79,7 +79,7 @@ public class UserDataPreparerTest {
         TEST_USER.serialNumber = TEST_USER_SERIAL;
         Context ctx = InstrumentationRegistry.getContext();
         FileUtils.deleteContents(ctx.getCacheDir());
-        mInstallLock = new Object();
+        mInstallLock = new PackageManagerTracedLock();
         MockitoAnnotations.initMocks(this);
         mUserDataPreparer = new TestUserDataPreparer(mInstaller, mInstallLock, mContextMock,
                 ctx.getCacheDir());
@@ -238,8 +238,8 @@ public class UserDataPreparerTest {
     private static class TestUserDataPreparer extends UserDataPreparer {
         File testDir;
 
-        TestUserDataPreparer(Installer installer, Object installLock, Context context,
-                File testDir) {
+        TestUserDataPreparer(Installer installer, PackageManagerTracedLock installLock,
+                Context context, File testDir) {
             super(installer, installLock, context);
             this.testDir = testDir;
         }
diff --git a/services/tests/mockingservicestests/src/com/android/server/pm/MockSystem.kt b/services/tests/mockingservicestests/src/com/android/server/pm/MockSystem.kt
index c9aab5318840..396edae2f672 100644
--- a/services/tests/mockingservicestests/src/com/android/server/pm/MockSystem.kt
+++ b/services/tests/mockingservicestests/src/com/android/server/pm/MockSystem.kt
@@ -186,7 +186,7 @@ class MockSystem(withSession: (StaticMockitoSessionBuilder) -> Unit = {}) {
 
     class Mocks {
         val lock = PackageManagerTracedLock()
-        val installLock = Any()
+        val installLock = PackageManagerTracedLock()
         val injector: PackageManagerServiceInjector = mock()
         val systemWrapper: PackageManagerServiceInjector.SystemWrapper = mock()
         val context: Context = mock()