[DO NOT MERGE] Handle config override via settings correctly

am: dc0f443cb6

Change-Id: Ifd39caee9a0dcea1314a3559a69d29ae08f1ce03

services/core/java/com/android/server/appop/AppOpsService.java

@@ -201,7 +201,7 @@ public class AppOpsService extends IAppOpsService.Stub {
     @VisibleForTesting
     final SparseArray<UidState> mUidStates = new SparseArray<>();
 
-    private final HistoricalRegistry mHistoricalRegistry = new HistoricalRegistry(this);
+    final HistoricalRegistry mHistoricalRegistry = new HistoricalRegistry(this);
 
     long mLastRealtime;
 

services/core/java/com/android/server/appop/HistoricalRegistry.java

@@ -108,6 +108,12 @@ import java.util.concurrent.TimeUnit;
  * must be called with the mInMemoryLock, xxxDMLocked suffix means the method
  * must be called with the mOnDiskLock and mInMemoryLock locks acquired in that
  * exact order.
+ * <p>
+ * INITIALIZATION: We can initialize persistence only after the system is ready
+ * as we need to check the optional configuration override from the settings
+ * database which is not initialized at the time the app ops service is created.
+ * This means that all entry points that touch persistence should be short
+ * circuited via isPersistenceInitialized() check.
  */
 // TODO (bug:122218838): Make sure we handle start of epoch time
 // TODO (bug:122218838): Validate changed time is handled correctly
@@ -177,14 +183,33 @@ final class HistoricalRegistry {
 
     // Object managing persistence (read/write)
     @GuardedBy("mOnDiskLock")
-    private Persistence mPersistence = new Persistence(mBaseSnapshotInterval,
+    private Persistence mPersistence;
-            mIntervalCompressionMultiplier);
 
     HistoricalRegistry(@NonNull Object lock) {
         mInMemoryLock = lock;
-        if (mMode != AppOpsManager.HISTORICAL_MODE_DISABLED) {
+    }
-            synchronized (mOnDiskLock) {
+
-                synchronized (mInMemoryLock) {
+    void systemReady(@NonNull ContentResolver resolver) {
+        final Uri uri = Settings.Global.getUriFor(Settings.Global.APPOP_HISTORY_PARAMETERS);
+        resolver.registerContentObserver(uri, false, new ContentObserver(
+                FgThread.getHandler()) {
+            @Override
+            public void onChange(boolean selfChange) {
+                updateParametersFromSetting(resolver);
+            }
+        });
+
+        updateParametersFromSetting(resolver);
+
+        synchronized (mOnDiskLock) {
+            synchronized (mInMemoryLock) {
+                if (mMode != AppOpsManager.HISTORICAL_MODE_DISABLED) {
+                    // Can be uninitialized if there is no config in the settings table.
+                    if (!isPersistenceInitializedMLocked()) {
+                        mPersistence = new Persistence(mBaseSnapshotInterval,
+                                mIntervalCompressionMultiplier);
+                    }
+
                     // When starting always adjust history to now.
                     final long lastPersistTimeMills =
                             mPersistence.getLastPersistTimeMillisDLocked();
@@ -197,16 +222,8 @@ final class HistoricalRegistry {
         }
     }
 
-    void systemReady(@NonNull ContentResolver resolver) {
+    private boolean isPersistenceInitializedMLocked() {
-        updateParametersFromSetting(resolver);
+        return mPersistence != null;
-        final Uri uri = Settings.Global.getUriFor(Settings.Global.APPOP_HISTORY_PARAMETERS);
-        resolver.registerContentObserver(uri, false, new ContentObserver(
-                FgThread.getHandler()) {
-            @Override
-            public void onChange(boolean selfChange) {
-                updateParametersFromSetting(resolver);
-            }
-        });
     }
 
     private void updateParametersFromSetting(@NonNull ContentResolver resolver) {
@@ -274,6 +291,11 @@ final class HistoricalRegistry {
                 makeRelativeToEpochStart(currentOps, nowMillis);
                 currentOps.accept(visitor);
 
+                if(isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
+
                 final List<HistoricalOps> ops = mPersistence.readHistoryDLocked();
                 if (ops != null) {
                     // TODO (bug:122218838): Make sure this is properly dumped
@@ -302,12 +324,21 @@ final class HistoricalRegistry {
     void getHistoricalOpsFromDiskRaw(int uid, @NonNull String packageName,
             @Nullable String[] opNames, long beginTimeMillis, long endTimeMillis,
             @OpFlags int flags, @NonNull RemoteCallback callback) {
-        final HistoricalOps result = new HistoricalOps(beginTimeMillis, endTimeMillis);
+        synchronized (mOnDiskLock) {
-        mPersistence.collectHistoricalOpsDLocked(result, uid, packageName, opNames,
+            synchronized (mInMemoryLock) {
-                beginTimeMillis, endTimeMillis, flags);
+                if (!isPersistenceInitializedMLocked()) {
-        final Bundle payload = new Bundle();
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
-        payload.putParcelable(AppOpsManager.KEY_HISTORICAL_OPS, result);
+                    callback.sendResult(new Bundle());
-        callback.sendResult(payload);
+                    return;
+                }
+                final HistoricalOps result = new HistoricalOps(beginTimeMillis, endTimeMillis);
+                mPersistence.collectHistoricalOpsDLocked(result, uid, packageName, opNames,
+                        beginTimeMillis, endTimeMillis, flags);
+                final Bundle payload = new Bundle();
+                payload.putParcelable(AppOpsManager.KEY_HISTORICAL_OPS, result);
+                callback.sendResult(payload);
+            }
+        }
     }
 
     void getHistoricalOps(int uid, @NonNull String packageName,
@@ -331,6 +362,12 @@ final class HistoricalRegistry {
             boolean collectOpsFromDisk;
 
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    callback.sendResult(new Bundle());
+                    return;
+                }
+
                 currentOps = getUpdatedPendingHistoricalOpsMLocked(currentTimeMillis);
                 if (!(inMemoryAdjBeginTimeMillis >= currentOps.getEndTimeMillis()
                         || inMemoryAdjEndTimeMillis <= currentOps.getBeginTimeMillis())) {
@@ -374,6 +411,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseAccessCount(op, uid, packageName, uidState, flags, 1);
             }
@@ -384,6 +425,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseRejectCount(op, uid, packageName, uidState, flags, 1);
             }
@@ -394,6 +439,10 @@ final class HistoricalRegistry {
             @UidState int uidState, @OpFlags int flags, long increment) {
         synchronized (mInMemoryLock) {
             if (mMode == AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 getUpdatedPendingHistoricalOpsMLocked(System.currentTimeMillis())
                         .increaseAccessDuration(op, uid, packageName, uidState, flags, increment);
             }
@@ -404,6 +453,8 @@ final class HistoricalRegistry {
             long baseSnapshotInterval, long intervalCompressionMultiplier) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                // NOTE: We allow this call if persistence is not initialized as
+                // it is a part of the persistence initialization process.
                 boolean resampleHistory = false;
                 Slog.i(LOG_TAG, "New history parameters: mode:"
                         + AppOpsManager.historicalModeToString(mMode) + " baseSnapshotInterval:"
@@ -412,7 +463,7 @@ final class HistoricalRegistry {
                 if (mMode != mode) {
                     mMode = mode;
                     if (mMode == AppOpsManager.HISTORICAL_MODE_DISABLED) {
-                        clearHistoryOnDiskLocked();
+                        clearHistoryOnDiskDLocked();
                     }
                 }
                 if (mBaseSnapshotInterval != baseSnapshotInterval) {
@@ -433,6 +484,10 @@ final class HistoricalRegistry {
     void offsetHistory(long offsetMillis) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 final List<HistoricalOps> history = mPersistence.readHistoryDLocked();
                 clearHistory();
                 if (history != null) {
@@ -453,6 +508,10 @@ final class HistoricalRegistry {
     void addHistoricalOps(HistoricalOps ops) {
         final List<HistoricalOps> pendingWrites;
         synchronized (mInMemoryLock) {
+            if (!isPersistenceInitializedMLocked()) {
+                Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                return;
+            }
             // The history files start from mBaseSnapshotInterval - take this into account.
             ops.offsetBeginAndEndTime(mBaseSnapshotInterval);
             mPendingWrites.offerFirst(ops);
@@ -468,6 +527,10 @@ final class HistoricalRegistry {
     }
 
     void resetHistoryParameters() {
+        if (!isPersistenceInitializedMLocked()) {
+            Slog.e(LOG_TAG, "Interaction before persistence initialized");
+            return;
+        }
         setHistoryParameters(DEFAULT_MODE, DEFAULT_SNAPSHOT_INTERVAL_MILLIS,
                 DEFAULT_COMPRESSION_STEP);
     }
@@ -475,6 +538,10 @@ final class HistoricalRegistry {
     void clearHistory(int uid, String packageName) {
         synchronized (mOnDiskLock) {
             synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
                 if (mMode != AppOpsManager.HISTORICAL_MODE_ENABLED_ACTIVE) {
                     return;
                 }
@@ -493,18 +560,24 @@ final class HistoricalRegistry {
 
     void clearHistory() {
         synchronized (mOnDiskLock) {
-            clearHistoryOnDiskLocked();
+            synchronized (mInMemoryLock) {
+                if (!isPersistenceInitializedMLocked()) {
+                    Slog.e(LOG_TAG, "Interaction before persistence initialized");
+                    return;
+                }
+                clearHistoryOnDiskDLocked();
+            }
         }
     }
 
-    private void clearHistoryOnDiskLocked() {
+    private void clearHistoryOnDiskDLocked() {
         BackgroundThread.getHandler().removeMessages(MSG_WRITE_PENDING_HISTORY);
         synchronized (mInMemoryLock) {
             mCurrentHistoricalOps = null;
             mNextPersistDueTimeMillis = System.currentTimeMillis();
             mPendingWrites.clear();
         }
-        mPersistence.clearHistoryDLocked();
+        Persistence.clearHistoryDLocked();
     }
 
     private @NonNull HistoricalOps getUpdatedPendingHistoricalOpsMLocked(long now) {
@@ -639,7 +712,7 @@ final class HistoricalRegistry {
             mIntervalCompressionMultiplier = intervalCompressionMultiplier;
         }
 
-        private final AtomicDirectory mHistoricalAppOpsDir = new AtomicDirectory(
+        private static final AtomicDirectory sHistoricalAppOpsDir = new AtomicDirectory(
                 new File(new File(Environment.getDataSystemDirectory(), "appops"), "history"));
 
         private File generateFile(@NonNull File baseDir, int depth) {
@@ -663,8 +736,8 @@ final class HistoricalRegistry {
             persistHistoricalOpsDLocked(historicalOps);
         }
 
-        void clearHistoryDLocked() {
+        static void clearHistoryDLocked() {
-            mHistoricalAppOpsDir.delete();
+            sHistoricalAppOpsDir.delete();
         }
 
         void persistHistoricalOpsDLocked(@NonNull List<HistoricalOps> ops) {
@@ -673,8 +746,8 @@ final class HistoricalRegistry {
                 enforceOpsWellFormed(ops);
             }
             try {
-                final File newBaseDir = mHistoricalAppOpsDir.startWrite();
+                final File newBaseDir = sHistoricalAppOpsDir.startWrite();
-                final File oldBaseDir = mHistoricalAppOpsDir.getBackupDirectory();
+                final File oldBaseDir = sHistoricalAppOpsDir.getBackupDirectory();
                 final HistoricalFilesInvariant filesInvariant;
                 if (DEBUG) {
                     filesInvariant = new HistoricalFilesInvariant();
@@ -686,10 +759,10 @@ final class HistoricalRegistry {
                 if (DEBUG) {
                     filesInvariant.stopTracking(newBaseDir);
                 }
-                mHistoricalAppOpsDir.finishWrite();
+                sHistoricalAppOpsDir.finishWrite();
             } catch (Throwable t) {
                 wtf("Failed to write historical app ops, restoring backup", t, null);
-                mHistoricalAppOpsDir.failWrite();
+                sHistoricalAppOpsDir.failWrite();
             }
         }
 
@@ -715,22 +788,36 @@ final class HistoricalRegistry {
         long getLastPersistTimeMillisDLocked() {
             File baseDir = null;
             try {
-                baseDir = mHistoricalAppOpsDir.startRead();
+                baseDir = sHistoricalAppOpsDir.startRead();
                 final File[] files = baseDir.listFiles();
                 if (files != null && files.length > 0) {
-                    final Set<File> historyFiles = new ArraySet<>();
+                    File shortestFile = null;
-                    Collections.addAll(historyFiles, files);
+                    for (File candidate : files) {
-                    for (int i = 0;; i++) {
+                        final String candidateName = candidate.getName();
-                        final File file = generateFile(baseDir, i);
+                        if (!candidateName.endsWith(HISTORY_FILE_SUFFIX)) {
-                        if (historyFiles.contains(file)) {
+                            continue;
-                            return file.lastModified();
+                        }
+                        if (shortestFile == null) {
+                            shortestFile = candidate;
+                        } else if (candidateName.length() < shortestFile.getName().length()) {
+                            shortestFile = candidate;
                         }
                     }
+                    if (shortestFile == null) {
+                        return 0;
+                    }
+                    final String shortestNameNoExtension = shortestFile.getName()
+                            .replace(HISTORY_FILE_SUFFIX, "");
+                    try {
+                        return Long.parseLong(shortestNameNoExtension);
+                    } catch (NumberFormatException e) {
+                        return 0;
+                    }
                 }
-                mHistoricalAppOpsDir.finishRead();
+                sHistoricalAppOpsDir.finishRead();
             } catch (Throwable e) {
                 wtf("Error reading historical app ops. Deleting history.", e, baseDir);
-                mHistoricalAppOpsDir.delete();
+                sHistoricalAppOpsDir.delete();
             }
             return 0;
         }
@@ -755,7 +842,7 @@ final class HistoricalRegistry {
                 long filterBeginTimeMillis, long filterEndTimeMillis, @OpFlags int filterFlags) {
             File baseDir = null;
             try {
-                baseDir = mHistoricalAppOpsDir.startRead();
+                baseDir = sHistoricalAppOpsDir.startRead();
                 final HistoricalFilesInvariant filesInvariant;
                 if (DEBUG) {
                     filesInvariant = new HistoricalFilesInvariant();
@@ -770,11 +857,11 @@ final class HistoricalRegistry {
                 if (DEBUG) {
                     filesInvariant.stopTracking(baseDir);
                 }
-                mHistoricalAppOpsDir.finishRead();
+                sHistoricalAppOpsDir.finishRead();
                 return ops;
             } catch (Throwable t) {
                 wtf("Error reading historical app ops. Deleting history.", t, baseDir);
-                mHistoricalAppOpsDir.delete();
+                sHistoricalAppOpsDir.delete();
             }
             return null;
         }
@@ -1241,7 +1328,7 @@ final class HistoricalRegistry {
 
         private void writeHistoricalOpsDLocked(@Nullable List<HistoricalOps> allOps,
                 long intervalOverflowMillis, @NonNull File file) throws IOException {
-            final FileOutputStream output = mHistoricalAppOpsDir.openWrite(file);
+            final FileOutputStream output = sHistoricalAppOpsDir.openWrite(file);
             try {
                 final XmlSerializer serializer = Xml.newSerializer();
                 serializer.setOutput(output, StandardCharsets.UTF_8.name());
@@ -1263,9 +1350,9 @@ final class HistoricalRegistry {
                 }
                 serializer.endTag(null, TAG_HISTORY);
                 serializer.endDocument();
-                mHistoricalAppOpsDir.closeWrite(output);
+                sHistoricalAppOpsDir.closeWrite(output);
             } catch (IOException e) {
-                mHistoricalAppOpsDir.failWrite(output);
+                sHistoricalAppOpsDir.failWrite(output);
                 throw e;
             }
         }

services/tests/servicestests/src/com/android/server/appop/AppOpsServiceTest.java

@@ -37,12 +37,15 @@ import android.os.Handler;
 import android.os.HandlerThread;
 import android.os.Process;
 import android.os.RemoteCallback;
+import android.provider.Settings;
 
 import androidx.test.InstrumentationRegistry;
 import androidx.test.filters.SmallTest;
 import androidx.test.runner.AndroidJUnit4;
 
+import org.junit.AfterClass;
 import org.junit.Before;
+import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
@@ -68,11 +71,14 @@ public class AppOpsServiceTest {
     private File mAppOpsFile;
     private Context mContext;
     private Handler mHandler;
+    private AppOpsManager mAppOpsManager;
     private AppOpsService mAppOpsService;
     private String mMyPackageName;
     private int mMyUid;
     private long mTestStartMillis;
 
+    private static String sDefaultAppopHistoryParameters;
+
     @Before
     public void setUp() {
         mContext = InstrumentationRegistry.getTargetContext();
@@ -88,11 +94,29 @@ public class AppOpsServiceTest {
         mMyPackageName = mContext.getOpPackageName();
         mMyUid = Process.myUid();
 
+        mAppOpsManager = mContext.getSystemService(AppOpsManager.class);
         mAppOpsService = new AppOpsService(mAppOpsFile, mHandler);
+        mAppOpsService.mHistoricalRegistry.systemReady(mContext.getContentResolver());
         mAppOpsService.mContext = mContext;
         mTestStartMillis = System.currentTimeMillis();
     }
 
+    @BeforeClass
+    public static void configureDesiredAppopHistoryParameters() {
+        final Context context = InstrumentationRegistry.getTargetContext();
+        sDefaultAppopHistoryParameters = Settings.Global.getString(context.getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS);
+        Settings.Global.putString(InstrumentationRegistry.getTargetContext().getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS, null);
+    }
+
+    @AfterClass
+    public static void restoreDefaultAppopHistoryParameters() {
+        Settings.Global.putString(InstrumentationRegistry.getTargetContext().getContentResolver(),
+                Settings.Global.APPOP_HISTORY_PARAMETERS,
+                sDefaultAppopHistoryParameters);
+    }
+
     @Test
     public void testGetOpsForPackage_noOpsLogged() {
         assertThat(getLoggedOps()).isNull();