Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 851813ad authored by Riddle Hsu's avatar Riddle Hsu
Browse files

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
parent cdd2a77e

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

+33 −3
Original line number Diff line number Diff line
@@ -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;

services/tests/servicestests/src/com/android/server/am/AnrHelperTest.java

+42 −1
Original line number Diff line number Diff line
@@ -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());

    }

}