From 851813ad16e7da392c1ffb3a4d0555d1cb686b13 Mon Sep 17 00:00:00 2001 From: Riddle Hsu Date: Mon, 14 Feb 2022 18:13:18 +0800 Subject: [PATCH] Skip duplicated ANR before enqueuing Although ProcessErrorStateRecord#appNotResponding has similar condition to check isNotResponding, it may not cover all cases which depends on the timing to set the flag, especially if there are multiple ANR records for the same process. It also reduces the race while an ANR record is dequeued but the process may be killed or restarted before dumping stacktrace. Bug: 218840177 Test: atest AnrHelperTest#testSkipDuplicatedAnr Change-Id: I10dd6678c2d288b6526cf97bf82e24f052fd89f5 --- .../java/com/android/server/am/AnrHelper.java | 36 ++++++++++++++-- .../com/android/server/am/AnrHelperTest.java | 43 ++++++++++++++++++- 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/services/core/java/com/android/server/am/AnrHelper.java b/services/core/java/com/android/server/am/AnrHelper.java index 34d9a60d78aae..1e1024e863142 100644 --- a/services/core/java/com/android/server/am/AnrHelper.java +++ b/services/core/java/com/android/server/am/AnrHelper.java @@ -60,6 +60,10 @@ class AnrHelper { */ private long mLastAnrTimeMs = 0L; + /** The pid which is running appNotResponding(). */ + @GuardedBy("mAnrRecords") + private int mProcessingPid = -1; + AnrHelper(final ActivityManagerService service) { mService = service; } @@ -73,7 +77,18 @@ class AnrHelper { void appNotResponding(ProcessRecord anrProcess, String activityShortComponentName, ApplicationInfo aInfo, String parentShortComponentName, WindowProcessController parentProcess, boolean aboveSystem, String annotation) { + final int incomingPid = anrProcess.mPid; synchronized (mAnrRecords) { + if (mProcessingPid == incomingPid) { + Slog.i(TAG, "Skip duplicated ANR, pid=" + incomingPid + " " + annotation); + return; + } + for (int i = mAnrRecords.size() - 1; i >= 0; i--) { + if (mAnrRecords.get(i).mPid == incomingPid) { + Slog.i(TAG, "Skip queued ANR, pid=" + incomingPid + " " + annotation); + return; + } + } mAnrRecords.add(new AnrRecord(anrProcess, activityShortComponentName, aInfo, parentShortComponentName, parentProcess, aboveSystem, annotation)); } @@ -87,8 +102,8 @@ class AnrHelper { } /** - * The thread to execute {@link ProcessRecord#appNotResponding}. It will terminate if all - * records are handled. + * The thread to execute {@link ProcessErrorStateRecord#appNotResponding}. It will terminate if + * all records are handled. */ private class AnrConsumerThread extends Thread { AnrConsumerThread() { @@ -97,7 +112,12 @@ class AnrHelper { private AnrRecord next() { synchronized (mAnrRecords) { - return mAnrRecords.isEmpty() ? null : mAnrRecords.remove(0); + if (mAnrRecords.isEmpty()) { + return null; + } + final AnrRecord record = mAnrRecords.remove(0); + mProcessingPid = record.mPid; + return record; } } @@ -106,6 +126,13 @@ class AnrHelper { AnrRecord r; while ((r = next()) != null) { scheduleBinderHeavyHitterAutoSamplerIfNecessary(); + final int currentPid = r.mApp.mPid; + if (currentPid != r.mPid) { + // The process may have restarted or died. + Slog.i(TAG, "Skip ANR with mismatched pid=" + r.mPid + ", current pid=" + + currentPid); + continue; + } final long startTime = SystemClock.uptimeMillis(); // If there are many ANR at the same time, the latency may be larger. If the latency // is too large, the stack trace might not be meaningful. @@ -120,6 +147,7 @@ class AnrHelper { mRunning.set(false); synchronized (mAnrRecords) { + mProcessingPid = -1; // The race should be unlikely to happen. Just to make sure we don't miss. if (!mAnrRecords.isEmpty()) { startAnrConsumerIfNeeded(); @@ -139,6 +167,7 @@ class AnrHelper { private static class AnrRecord { final ProcessRecord mApp; + final int mPid; final String mActivityShortComponentName; final String mParentShortComponentName; final String mAnnotation; @@ -151,6 +180,7 @@ class AnrHelper { ApplicationInfo aInfo, String parentShortComponentName, WindowProcessController parentProcess, boolean aboveSystem, String annotation) { mApp = anrProcess; + mPid = anrProcess.mPid; mActivityShortComponentName = activityShortComponentName; mParentShortComponentName = parentShortComponentName; mAnnotation = annotation; diff --git a/services/tests/servicestests/src/com/android/server/am/AnrHelperTest.java b/services/tests/servicestests/src/com/android/server/am/AnrHelperTest.java index 877538cdda6f8..782d519418092 100644 --- a/services/tests/servicestests/src/com/android/server/am/AnrHelperTest.java +++ b/services/tests/servicestests/src/com/android/server/am/AnrHelperTest.java @@ -20,7 +20,12 @@ import static android.testing.DexmakerShareClassLoaderRule.runWithDexmakerShareC import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; +import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.timeout; import static org.mockito.Mockito.verify; @@ -42,6 +47,7 @@ import org.junit.Test; import java.io.File; import java.lang.reflect.Field; import java.lang.reflect.Modifier; +import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; /** @@ -51,6 +57,7 @@ import java.util.concurrent.TimeUnit; @SmallTest @Presubmit public class AnrHelperTest { + private static final long TIMEOUT_MS = TimeUnit.SECONDS.toMillis(5); private AnrHelper mAnrHelper; private ProcessRecord mAnrApp; @@ -106,8 +113,42 @@ public class AnrHelperTest { mAnrHelper.appNotResponding(mAnrApp, activityShortComponentName, appInfo, parentShortComponentName, parentProcess, aboveSystem, annotation); - verify(mAnrApp.mErrorState, timeout(TimeUnit.SECONDS.toMillis(5))).appNotResponding( + verify(mAnrApp.mErrorState, timeout(TIMEOUT_MS)).appNotResponding( eq(activityShortComponentName), eq(appInfo), eq(parentShortComponentName), eq(parentProcess), eq(aboveSystem), eq(annotation), eq(false) /* onlyDumpSelf */); } + + @Test + public void testSkipDuplicatedAnr() { + final CountDownLatch consumerLatch = new CountDownLatch(1); + final CountDownLatch processingLatch = new CountDownLatch(1); + doAnswer(invocation -> { + consumerLatch.countDown(); + // Simulate that it is dumping to block the consumer thread. + processingLatch.await(); + return null; + }).when(mAnrApp.mErrorState).appNotResponding(anyString(), any(), any(), any(), + anyBoolean(), anyString(), anyBoolean()); + final ApplicationInfo appInfo = new ApplicationInfo(); + mAnrApp.mPid = 12345; + final Runnable reportAnr = () -> mAnrHelper.appNotResponding(mAnrApp, + "activityShortComponentName", appInfo, "parentShortComponentName", + null /* parentProcess */, false /* aboveSystem */, "annotation"); + reportAnr.run(); + // This should be skipped because the pid is pending in queue. + reportAnr.run(); + // The first reported ANR must be processed. + try { + assertTrue(consumerLatch.await(TIMEOUT_MS, TimeUnit.MILLISECONDS)); + } catch (InterruptedException ignored) { + } + // This should be skipped because the pid is under processing. + reportAnr.run(); + + // Assume that the first one finishes after all incoming ANRs. + processingLatch.countDown(); + // There is only one ANR reported. + verify(mAnrApp.mErrorState, timeout(TIMEOUT_MS).only()).appNotResponding( + anyString(), any(), any(), any(), anyBoolean(), anyString(), anyBoolean()); + } }