Merge "Layer "locked" methods for better locking logs." into udc-dev

services/core/java/com/android/server/am/BroadcastProcessQueue.java

@@ -1050,13 +1050,13 @@ class BroadcastProcessQueue {
      * Check overall health, confirming things are in a reasonable state and
      * that we're not wedged.
      */
-    public void checkHealthLocked() {
+    public void assertHealthLocked() {
-        checkHealthLocked(mPending);
+        assertHealthLocked(mPending);
-        checkHealthLocked(mPendingUrgent);
+        assertHealthLocked(mPendingUrgent);
-        checkHealthLocked(mPendingOffload);
+        assertHealthLocked(mPendingOffload);
     }
 
-    private void checkHealthLocked(@NonNull ArrayDeque<SomeArgs> queue) {
+    private void assertHealthLocked(@NonNull ArrayDeque<SomeArgs> queue) {
         if (queue.isEmpty()) return;
 
         final Iterator<SomeArgs> it = queue.descendingIterator();

services/core/java/com/android/server/am/BroadcastQueueModernImpl.java

@@ -246,21 +246,15 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
     private final Handler.Callback mLocalCallback = (msg) -> {
         switch (msg.what) {
             case MSG_UPDATE_RUNNING_LIST: {
-                synchronized (mService) {
+                updateRunningList();
-                    updateRunningListLocked();
-                }
                 return true;
             }
             case MSG_DELIVERY_TIMEOUT_SOFT: {
-                synchronized (mService) {
+                deliveryTimeoutSoft((BroadcastProcessQueue) msg.obj, msg.arg1);
-                    deliveryTimeoutSoftLocked((BroadcastProcessQueue) msg.obj, msg.arg1);
-                }
                 return true;
             }
             case MSG_DELIVERY_TIMEOUT_HARD: {
-                synchronized (mService) {
+                deliveryTimeoutHard((BroadcastProcessQueue) msg.obj);
-                    deliveryTimeoutHardLocked((BroadcastProcessQueue) msg.obj);
-                }
                 return true;
             }
             case MSG_BG_ACTIVITY_START_TIMEOUT: {
@@ -274,9 +268,7 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
                 return true;
             }
             case MSG_CHECK_HEALTH: {
-                synchronized (mService) {
+                checkHealth();
-                    checkHealthLocked();
-                }
                 return true;
             }
         }
@@ -365,6 +357,12 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
         }
     }
 
+    private void updateRunningList() {
+        synchronized (mService) {
+            updateRunningListLocked();
+        }
+    }
+
     /**
      * Consider updating the list of "running" queues.
      * <p>
@@ -965,6 +963,13 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
         r.resultTo = null;
     }
 
+    private void deliveryTimeoutSoft(@NonNull BroadcastProcessQueue queue,
+            int softTimeoutMillis) {
+        synchronized (mService) {
+            deliveryTimeoutSoftLocked(queue, softTimeoutMillis);
+        }
+    }
+
     private void deliveryTimeoutSoftLocked(@NonNull BroadcastProcessQueue queue,
             int softTimeoutMillis) {
         if (queue.app != null) {
@@ -981,6 +986,12 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
         }
     }
 
+    private void deliveryTimeoutHard(@NonNull BroadcastProcessQueue queue) {
+        synchronized (mService) {
+            deliveryTimeoutHardLocked(queue);
+        }
+    }
+
     private void deliveryTimeoutHardLocked(@NonNull BroadcastProcessQueue queue) {
         finishReceiverActiveLocked(queue, BroadcastRecord.DELIVERY_TIMEOUT,
                 "deliveryTimeoutHardLocked");
@@ -1458,52 +1469,19 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
         // TODO: implement
     }
 
-    /**
+    private void checkHealth() {
-     * Check overall health, confirming things are in a reasonable state and
+        synchronized (mService) {
-     * that we're not wedged. If we determine we're in an unhealthy state, dump
+            checkHealthLocked();
-     * current state once and stop future health checks to avoid spamming.
+        }
-     */
+    }
-    @VisibleForTesting
+
-    void checkHealthLocked() {
+    private void checkHealthLocked() {
         try {
-            // Verify all runnable queues are sorted
+            assertHealthLocked();
-            BroadcastProcessQueue prev = null;
-            BroadcastProcessQueue next = mRunnableHead;
-            while (next != null) {
-                checkState(next.runnableAtPrev == prev, "runnableAtPrev");
-                checkState(next.isRunnable(), "isRunnable " + next);
-                if (prev != null) {
-                    checkState(next.getRunnableAt() >= prev.getRunnableAt(),
-                            "getRunnableAt " + next + " vs " + prev);
-                }
-                prev = next;
-                next = next.runnableAtNext;
-            }
-
-            // Verify all running queues are active
-            for (BroadcastProcessQueue queue : mRunning) {
-                if (queue != null) {
-                    checkState(queue.isActive(), "isActive " + queue);
-                }
-            }
-
-            // Verify that pending cold start hasn't been orphaned
-            if (mRunningColdStart != null) {
-                checkState(getRunningIndexOf(mRunningColdStart) >= 0,
-                        "isOrphaned " + mRunningColdStart);
-            }
-
-            // Verify health of all known process queues
-            for (int i = 0; i < mProcessQueues.size(); i++) {
-                BroadcastProcessQueue leaf = mProcessQueues.valueAt(i);
-                while (leaf != null) {
-                    leaf.checkHealthLocked();
-                    leaf = leaf.processNameNext;
-                }
-            }
 
             // If no health issues found above, check again in the future
-            mLocalHandler.sendEmptyMessageDelayed(MSG_CHECK_HEALTH, DateUtils.MINUTE_IN_MILLIS);
+            mLocalHandler.sendEmptyMessageDelayed(MSG_CHECK_HEALTH,
+                    DateUtils.MINUTE_IN_MILLIS);
 
         } catch (Exception e) {
             // Throw up a message to indicate that something went wrong, and
@@ -1513,6 +1491,50 @@ class BroadcastQueueModernImpl extends BroadcastQueue {
         }
     }
 
+    /**
+     * Check overall health, confirming things are in a reasonable state and
+     * that we're not wedged. If we determine we're in an unhealthy state, dump
+     * current state once and stop future health checks to avoid spamming.
+     */
+    @VisibleForTesting
+    void assertHealthLocked() {
+        // Verify all runnable queues are sorted
+        BroadcastProcessQueue prev = null;
+        BroadcastProcessQueue next = mRunnableHead;
+        while (next != null) {
+            checkState(next.runnableAtPrev == prev, "runnableAtPrev");
+            checkState(next.isRunnable(), "isRunnable " + next);
+            if (prev != null) {
+                checkState(next.getRunnableAt() >= prev.getRunnableAt(),
+                        "getRunnableAt " + next + " vs " + prev);
+            }
+            prev = next;
+            next = next.runnableAtNext;
+        }
+
+        // Verify all running queues are active
+        for (BroadcastProcessQueue queue : mRunning) {
+            if (queue != null) {
+                checkState(queue.isActive(), "isActive " + queue);
+            }
+        }
+
+        // Verify that pending cold start hasn't been orphaned
+        if (mRunningColdStart != null) {
+            checkState(getRunningIndexOf(mRunningColdStart) >= 0,
+                    "isOrphaned " + mRunningColdStart);
+        }
+
+        // Verify health of all known process queues
+        for (int i = 0; i < mProcessQueues.size(); i++) {
+            BroadcastProcessQueue leaf = mProcessQueues.valueAt(i);
+            while (leaf != null) {
+                leaf.assertHealthLocked();
+                leaf = leaf.processNameNext;
+            }
+        }
+    }
+
     private void updateWarmProcess(@NonNull BroadcastProcessQueue queue) {
         if (!queue.isProcessWarm()) {
             setQueueProcess(queue, mService.getProcessRecordLocked(queue.processName, queue.uid));

services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueModernImplTest.java

@@ -91,8 +91,8 @@ import org.junit.Rule;
 import org.junit.Test;
 import org.mockito.Mock;
 
-import java.io.ByteArrayOutputStream;
 import java.io.PrintWriter;
+import java.io.Writer;
 import java.lang.reflect.Array;
 import java.util.ArrayList;
 import java.util.List;
@@ -596,7 +596,7 @@ public final class BroadcastQueueModernImplTest {
         // about the actual output, just that we don't crash
         queue.getActive().setDeliveryState(0, BroadcastRecord.DELIVERY_SCHEDULED, "Test-driven");
         queue.dumpLocked(SystemClock.uptimeMillis(),
-                new IndentingPrintWriter(new PrintWriter(new ByteArrayOutputStream())));
+                new IndentingPrintWriter(new PrintWriter(Writer.nullWriter())));
 
         queue.makeActiveNextPending();
         assertEquals(Intent.ACTION_LOCALE_CHANGED, queue.getActive().intent.getAction());
@@ -1166,6 +1166,11 @@ public final class BroadcastQueueModernImplTest {
             List<Intent> intents) {
         for (int i = 0; i < intents.size(); i++) {
             queue.makeActiveNextPending();
+
+            // While we're here, give our health check some test coverage
+            queue.assertHealthLocked();
+            queue.dumpLocked(0L, new IndentingPrintWriter(Writer.nullWriter()));
+
             final Intent actualIntent = queue.getActive().intent;
             final Intent expectedIntent = intents.get(i);
             final String errMsg = "actual=" + actualIntent + ", expected=" + expectedIntent

services/tests/mockingservicestests/src/com/android/server/am/BroadcastQueueTest.java

@@ -39,6 +39,7 @@ import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
@@ -106,10 +107,10 @@ import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.verification.VerificationMode;
 
-import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
+import java.io.Writer;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -231,6 +232,7 @@ public class BroadcastQueueTest {
         doAnswer((invocation) -> {
             Log.v(TAG, "Intercepting startProcessLocked() for "
                     + Arrays.toString(invocation.getArguments()));
+            assertHealth();
             final ProcessStartBehavior behavior = mNextProcessStartBehavior
                     .getAndSet(ProcessStartBehavior.SUCCESS);
             if (behavior == ProcessStartBehavior.FAIL_NULL) {
@@ -462,6 +464,7 @@ public class BroadcastQueueTest {
         doAnswer((invocation) -> {
             Log.v(TAG, "Intercepting scheduleReceiver() for "
                     + Arrays.toString(invocation.getArguments()));
+            assertHealth();
             final Intent intent = invocation.getArgument(0);
             final Bundle extras = invocation.getArgument(5);
             mScheduledBroadcasts.add(makeScheduledBroadcast(r, intent));
@@ -483,6 +486,7 @@ public class BroadcastQueueTest {
         doAnswer((invocation) -> {
             Log.v(TAG, "Intercepting scheduleRegisteredReceiver() for "
                     + Arrays.toString(invocation.getArguments()));
+            assertHealth();
             final Intent intent = invocation.getArgument(1);
             final Bundle extras = invocation.getArgument(4);
             final boolean ordered = invocation.getArgument(5);
@@ -600,6 +604,13 @@ public class BroadcastQueueTest {
                 BackgroundStartPrivileges.NONE, false, null);
     }
 
+    private void assertHealth() {
+        if (mImpl == Impl.MODERN) {
+            // If this fails, it'll throw a clear reason message
+            ((BroadcastQueueModernImpl) mQueue).assertHealthLocked();
+        }
+    }
+
     private static Map<String, Object> asMap(Bundle bundle) {
         final Map<String, Object> map = new HashMap<>();
         if (bundle != null) {
@@ -769,7 +780,7 @@ public class BroadcastQueueTest {
         // about the actual output, just that we don't crash
         mQueue.dumpDebug(new ProtoOutputStream(),
                 ActivityManagerServiceDumpBroadcastsProto.BROADCAST_QUEUE);
-        mQueue.dumpLocked(FileDescriptor.err, new PrintWriter(new ByteArrayOutputStream()),
+        mQueue.dumpLocked(FileDescriptor.err, new PrintWriter(Writer.nullWriter()),
                 null, 0, true, true, true, null, false);
         mQueue.dumpToDropBoxLocked(TAG);
 
@@ -1166,7 +1177,7 @@ public class BroadcastQueueTest {
             // about the actual output, just that we don't crash
             mQueue.dumpDebug(new ProtoOutputStream(),
                     ActivityManagerServiceDumpBroadcastsProto.BROADCAST_QUEUE);
-            mQueue.dumpLocked(FileDescriptor.err, new PrintWriter(new ByteArrayOutputStream()),
+            mQueue.dumpLocked(FileDescriptor.err, new PrintWriter(Writer.nullWriter()),
                     null, 0, true, true, true, null, false);
         }