LockSettingsStorage: fix concurrent read clobbering a removal

Since Cache.remove() doesn't increment mVersion, the key removal can be
clobbered by a concurrent read that loads the key into the cache.  Fix
this by making Cache.remove() increment mVersion, indicating that the
backing storage has been modified.

Found by code review; this bug isn't known to have been causing any
real-world problems.

Add a best-effort test for this, though due to the very tight race
needed, it didn't reproduce the bug in 2000 iterations.  However, it
reproduced the bug in just a few iterations if a 1 millisecond sleep is
added in the right place (just before the call to
Cache.putKeyValueIfUnchanged() in LockSettingsStorage.readKeyValue()).

Test: atest --iterations 50 LockSettingsStorageTests
Change-Id: I960cc386ecd040af7517d3d62a3eefe57a518620

services/core/java/com/android/server/locksettings/LockSettingsStorage.java

@@ -843,6 +843,7 @@ class LockSettingsStorage extends WatchableImpl {
 
         private synchronized void remove(int type, String key, int userId) {
             mCache.remove(mCacheKey.set(type, key, userId));
+            mVersion++;
         }
 
         private synchronized void put(int type, String key, Object value, int userId) {

services/tests/servicestests/src/com/android/server/locksettings/LockSettingsStorageTests.java

@@ -19,6 +19,7 @@ package com.android.server.locksettings;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -60,7 +61,10 @@ import java.io.File;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
+import java.util.concurrent.BrokenBarrierException;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.atomic.AtomicReference;
 
 /**
  * atest FrameworksServicesTests:LockSettingsStorageTests
@@ -137,12 +141,12 @@ public class LockSettingsStorageTests {
     }
 
     @Test
-    public void testKeyValue_Concurrency() {
+    public void testKeyValue_ReadWriteConcurrency() {
         final CountDownLatch latch = new CountDownLatch(1);
         List<Thread> threads = new ArrayList<>();
         for (int i = 0; i < 100; i++) {
             final int threadId = i;
-            threads.add(new Thread("testKeyValue_Concurrency_" + i) {
+            threads.add(new Thread("testKeyValue_ReadWriteConcurrency_" + i) {
                 @Override
                 public void run() {
                     try {
@@ -164,7 +168,7 @@ public class LockSettingsStorageTests {
             });
             threads.get(i).start();
         }
-        mStorage.writeKeyValue("key", "initalValue", 0);
+        mStorage.writeKeyValue("key", "initialValue", 0);
         latch.countDown();
         joinAll(threads, 10000);
         assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0));
@@ -172,6 +176,52 @@ public class LockSettingsStorageTests {
         assertEquals('5', mStorage.readKeyValue("key", "default", 0).charAt(0));
     }
 
+    // Test that readKeyValue() doesn't pollute the cache when run concurrently with removeKey().
+    @Test
+    @SuppressWarnings("AssertionFailureIgnored") // intentional try-catch of AssertionError
+    public void testKeyValue_ReadRemoveConcurrency() {
+        final int numThreads = 2;
+        final int numIterations = 50;
+        final CyclicBarrier barrier = new CyclicBarrier(numThreads);
+        final List<Thread> threads = new ArrayList<>();
+        final AtomicReference<Throwable> failure = new AtomicReference<>();
+        for (int threadId = 0; threadId < numThreads; threadId++) {
+            final boolean isWriter = (threadId == 0);
+            threads.add(new Thread("testKeyValue_ReadRemoveConcurrency_" + threadId) {
+                @Override
+                public void run() {
+                    try {
+                        for (int iter = 0; iter < numIterations; iter++) {
+                            if (isWriter) {
+                                mStorage.writeKeyValue("key", "value", 0);
+                                mStorage.clearCache();
+                            }
+                            barrier.await();
+                            if (isWriter) {
+                                mStorage.removeKey("key", 0);
+                            } else {
+                                mStorage.readKeyValue("key", "default", 0);
+                            }
+                            barrier.await();
+                            try {
+                                assertEquals("default", mStorage.readKeyValue("key", "default", 0));
+                            } catch (AssertionError e) {
+                                failure.compareAndSet(null, e);
+                            }
+                            barrier.await();
+                        }
+                    } catch (InterruptedException | BrokenBarrierException e) {
+                        failure.compareAndSet(null, e);
+                        return;
+                    }
+                }
+            });
+            threads.get(threadId).start();
+        }
+        joinAll(threads, 60000);
+        assertNull(failure.get());
+    }
+
     @Test
     public void testKeyValue_CacheStarvedWriter() {
         final CountDownLatch latch = new CountDownLatch(1);