From 9e92ae1c96f1cc45133472b518ef069174b57c07 Mon Sep 17 00:00:00 2001
From: Songchun Fan <schfan@google.com>
Date: Thu, 6 Jun 2024 14:18:19 -0700
Subject: [pm] revert lock changes to settings write

This reverts ag/25983279, which introduced a behavior change in the
locking of writeSettings. As a result of that CL, the disk writes were
not guarded by "synchronized (mLock)" which may cause issues if the
settings are being modified at the same time as the writes.

BUG: 345278249
Test: presubmit
Change-Id: I3c855c986bb7bc9b7ab220e416305d562c621a5b
---
 .../java/com/android/server/pm/PackageHandler.java | 13 +------
 .../android/server/pm/PackageManagerService.java   | 43 +++++-----------------
 2 files changed, 11 insertions(+), 45 deletions(-)

diff --git a/services/core/java/com/android/server/pm/PackageHandler.java b/services/core/java/com/android/server/pm/PackageHandler.java
index 68f6ca1c019f..0a0882d80cc1 100644
--- a/services/core/java/com/android/server/pm/PackageHandler.java
+++ b/services/core/java/com/android/server/pm/PackageHandler.java
@@ -123,19 +123,10 @@ final class PackageHandler extends Handler {
                 }
             } break;
             case WRITE_SETTINGS: {
-                if (!mPm.tryWriteSettings(/*sync=*/false)) {
-                    // Failed to write.
-                    this.removeMessages(WRITE_SETTINGS);
-                    mPm.scheduleWriteSettings();
-                }
+                mPm.writeSettings(/*sync=*/false);
             } break;
             case WRITE_PACKAGE_LIST: {
-                int userId = msg.arg1;
-                if (!mPm.tryWritePackageList(userId)) {
-                    // Failed to write.
-                    this.removeMessages(WRITE_PACKAGE_LIST);
-                    mPm.scheduleWritePackageList(userId);
-                }
+                mPm.writePackageList(msg.arg1);
             } break;
             case CHECK_PENDING_VERIFICATION: {
                 final int verificationId = msg.arg1;
diff --git a/services/core/java/com/android/server/pm/PackageManagerService.java b/services/core/java/com/android/server/pm/PackageManagerService.java
index 0925271ab5a3..29c89afa9b96 100644
--- a/services/core/java/com/android/server/pm/PackageManagerService.java
+++ b/services/core/java/com/android/server/pm/PackageManagerService.java
@@ -485,9 +485,6 @@ public class PackageManagerService implements PackageSender, TestUtilityService
      */
     static final long WATCHDOG_TIMEOUT = 1000*60*10;     // ten minutes
 
-    // How long to wait for Lock in async writeSettings and writePackageList.
-    private static final long WRITE_LOCK_TIMEOUT_MS = 1000 * 10;   // 10 seconds
-
     /**
      * Default IncFs timeouts. Maximum values in IncFs is 1hr.
      *
@@ -1576,7 +1573,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         }
     }
 
-    void scheduleWritePackageList(int userId) {
+    void scheduleWritePackageListLocked(int userId) {
         invalidatePackageInfoCache();
         if (!mHandler.hasMessages(WRITE_PACKAGE_LIST)) {
             Message msg = mHandler.obtainMessage(WRITE_PACKAGE_LIST);
@@ -1628,42 +1625,22 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         mSettings.writePackageRestrictions(dirtyUsers);
     }
 
-    private boolean tryUnderLock(boolean sync, long timeoutMs, Runnable runnable) {
-        try {
-            PackageManagerTracedLock.RawLock lock = mLock.getRawLock();
-            if (sync) {
-                lock.lock();
-            } else if (!lock.tryLock(timeoutMs, TimeUnit.MILLISECONDS)) {
-                return false;
-            }
-            try {
-                runnable.run();
-                return true;
-            } finally {
-                lock.unlock();
-            }
-        } catch (InterruptedException e) {
-            Slog.e(TAG, "Failed to obtain mLock", e);
-        }
-        return false;
-    }
-
-    boolean tryWriteSettings(boolean sync) {
-        return tryUnderLock(sync, WRITE_LOCK_TIMEOUT_MS, () -> {
+    void writeSettings(boolean sync) {
+        synchronized (mLock) {
             mHandler.removeMessages(WRITE_SETTINGS);
             mBackgroundHandler.removeMessages(WRITE_DIRTY_PACKAGE_RESTRICTIONS);
             writeSettingsLPrTEMP(sync);
             synchronized (mDirtyUsers) {
                 mDirtyUsers.clear();
             }
-        });
+        }
     }
 
-    boolean tryWritePackageList(int userId) {
-        return tryUnderLock(/*sync=*/false, WRITE_LOCK_TIMEOUT_MS, () -> {
+    void writePackageList(int userId) {
+        synchronized (mLock) {
             mHandler.removeMessages(WRITE_PACKAGE_LIST);
             mSettings.writePackageListLPr(userId);
-        });
+        }
     }
 
     private static final Handler.Callback BACKGROUND_HANDLER_CALLBACK = new Handler.Callback() {
@@ -3067,9 +3044,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
             if (mHandler.hasMessages(WRITE_SETTINGS)
                     || mBackgroundHandler.hasMessages(WRITE_DIRTY_PACKAGE_RESTRICTIONS)
                     || mHandler.hasMessages(WRITE_PACKAGE_LIST)) {
-                while (!tryWriteSettings(/*sync=*/true)) {
-                    Slog.wtf(TAG, "Failed to write settings on shutdown");
-                }
+                writeSettings(/*sync=*/true);
             }
         }
     }
@@ -4429,7 +4404,7 @@ public class PackageManagerService implements PackageSender, TestUtilityService
         }
         synchronized (mLock) {
             scheduleWritePackageRestrictions(userId);
-            scheduleWritePackageList(userId);
+            scheduleWritePackageListLocked(userId);
             mAppsFilter.onUserCreated(snapshotComputer(), userId);
         }
     }
-- 
cgit v1.2.3-59-g8ed1b