From ae02ad50698161a38821eaa88383f9dfd865fd82 Mon Sep 17 00:00:00 2001 From: Winson Chung Date: Thu, 2 Feb 2023 06:07:21 +0000 Subject: [PATCH] Add registry for tracking leaking surface controls - When enabled, the registry will track active surface controls within the process. If the number of surfaces exceeds a specific threshold, info about the registered surfaces can be dumped to help narrow down where they were registered/leaked. This also facilitates adding tests of flows to ensure new surfaces are properly cleaned up. - Only callers with the READ_FRAME_BUFFER permission can enable the registry - Once the max threshold is crossed, information is dumped to logcat only once until the number of surfaces falls under the reset threshold to ensure we don't spam the logs Bug: 266978825 Test: atest FrameworksCoreTests:android.view.SurfaceControlRegistryTests Change-Id: Ife73357e7db4b45d474ef3933afd3e33b8681f85 --- core/java/android/view/SurfaceControl.java | 43 ++- .../android/view/SurfaceControlRegistry.java | 273 ++++++++++++++++++ .../view/SurfaceControlRegistryTests.java | 173 +++++++++++ 3 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 core/java/android/view/SurfaceControlRegistry.java create mode 100644 core/tests/coretests/src/android/view/SurfaceControlRegistryTests.java diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java index 62fdfaea13037..ddaa71c6b1b59 100644 --- a/core/java/android/view/SurfaceControl.java +++ b/core/java/android/view/SurfaceControl.java @@ -461,6 +461,7 @@ public final class SurfaceControl implements Parcelable { private final CloseGuard mCloseGuard = CloseGuard.get(); private String mName; + private String mCallsite; /** * Note: do not rename, this field is used by native code. @@ -774,7 +775,6 @@ public final class SurfaceControl implements Parcelable { release(); } if (nativeObject != 0) { - mCloseGuard.openWithCallSite("release", callsite); mFreeNativeResources = sRegistry.registerNativeAllocation(this, nativeObject); } @@ -785,6 +785,8 @@ public final class SurfaceControl implements Parcelable { } else { mReleaseStack = null; } + setUnreleasedWarningCallSite(callsite); + addToRegistry(); } /** @@ -1322,6 +1324,23 @@ public final class SurfaceControl implements Parcelable { return; } mCloseGuard.openWithCallSite("release", callsite); + mCallsite = callsite; + } + + /** + * Returns the last provided call site when this SurfaceControl was created. + * @hide + */ + @Nullable String getCallsite() { + return mCallsite; + } + + /** + * Returns the name of this SurfaceControl, mainly for debugging purposes. + * @hide + */ + @NonNull String getName() { + return mName; } /** @@ -1435,6 +1454,7 @@ public final class SurfaceControl implements Parcelable { if (mCloseGuard != null) { mCloseGuard.warnIfOpen(); } + removeFromRegistry(); } finally { super.finalize(); } @@ -1465,6 +1485,7 @@ public final class SurfaceControl implements Parcelable { mChoreographer = null; } } + removeFromRegistry(); } } @@ -4304,6 +4325,26 @@ public final class SurfaceControl implements Parcelable { return -1; } + /** + * Adds this surface control to the registry for this process if it is created. + */ + private void addToRegistry() { + final SurfaceControlRegistry registry = SurfaceControlRegistry.getProcessInstance(); + if (registry != null) { + registry.add(this); + } + } + + /** + * Removes this surface control from the registry for this process. + */ + private void removeFromRegistry() { + final SurfaceControlRegistry registry = SurfaceControlRegistry.getProcessInstance(); + if (registry != null) { + registry.remove(this); + } + } + // Called by native private static void invokeReleaseCallback(Consumer callback, long nativeFencePtr) { SyncFence fence = new SyncFence(nativeFencePtr); diff --git a/core/java/android/view/SurfaceControlRegistry.java b/core/java/android/view/SurfaceControlRegistry.java new file mode 100644 index 0000000000000..095189ad03a78 --- /dev/null +++ b/core/java/android/view/SurfaceControlRegistry.java @@ -0,0 +1,273 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.view; + +import static android.Manifest.permission.READ_FRAME_BUFFER; +import static android.content.pm.PackageManager.PERMISSION_GRANTED; + +import android.annotation.NonNull; +import android.annotation.Nullable; +import android.annotation.RequiresPermission; +import android.content.Context; +import android.os.SystemClock; +import android.util.Log; + +import com.android.internal.annotations.GuardedBy; +import com.android.internal.annotations.VisibleForTesting; +import com.android.internal.util.GcUtils; + +import java.io.PrintWriter; +import java.util.ArrayList; +import java.util.Map; +import java.util.WeakHashMap; + +/** + * A thread-safe registry used to track surface controls that are active (not yet released) within a + * process, to help debug and identify leaks. + * @hide + */ +public class SurfaceControlRegistry { + private static final String TAG = "SurfaceControlRegistry"; + + /** + * An interface for processing the registered SurfaceControls when the threshold is exceeded. + */ + public interface Reporter { + /** + * Called when the set of layers exceeds the max threshold. This can be called on any + * thread, and must be handled synchronously. + */ + void onMaxLayersExceeded(WeakHashMap surfaceControls, int limit, + PrintWriter pw); + } + + /** + * The default implementation of the reporter which logs the existing registered surfaces to + * logcat. + */ + private static class DefaultReporter implements Reporter { + public void onMaxLayersExceeded(WeakHashMap surfaceControls, + int limit, PrintWriter pw) { + final int size = Math.min(surfaceControls.size(), limit); + final long now = SystemClock.elapsedRealtime(); + final ArrayList> entries = new ArrayList<>(); + for (Map.Entry entry : surfaceControls.entrySet()) { + entries.add(entry); + } + // Sort entries by time registered when dumping + // TODO: Or should it sort by name? + entries.sort((o1, o2) -> (int) (o1.getValue() - o2.getValue())); + + pw.println("SurfaceControlRegistry"); + pw.println("----------------------"); + pw.println("Listing oldest " + size + " of " + surfaceControls.size()); + for (int i = 0; i < size; i++) { + final Map.Entry entry = entries.get(i); + final SurfaceControl sc = entry.getKey(); + final long timeRegistered = entry.getValue(); + pw.print(" "); + pw.print(sc.getName()); + pw.print(" (" + sc.getCallsite() + ")"); + pw.println(" [" + ((now - timeRegistered) / 1000) + "s ago]"); + } + } + } + + // The threshold at which to dump information about all the known active SurfaceControls in the + // process when the number of layers exceeds a certain count. This should be significantly + // smaller than the MAX_LAYERS (currently 4096) defined in SurfaceFlinger.h + private static final int MAX_LAYERS_REPORTING_THRESHOLD = 1024; + + // The threshold at which to reset the dump state. Needs to be smaller than + // MAX_LAYERS_REPORTING_THRESHOLD + private static final int RESET_REPORTING_THRESHOLD = 256; + + // Number of surface controls to dump when the max threshold is exceeded + private static final int DUMP_LIMIT = 256; + + // Static lock, must be held for all registry operations + private static final Object sLock = new Object(); + + // The default reporter for printing out the registered surfaces + private static final DefaultReporter sDefaultReporter = new DefaultReporter(); + + // The registry for a given process + private static volatile SurfaceControlRegistry sProcessRegistry; + + // Mapping of the active SurfaceControls to the elapsed time when they were registered + @GuardedBy("sLock") + private final WeakHashMap mSurfaceControls; + + // The threshold at which we dump information about the current set of registered surfaces. + // Once this threshold is reached, we no longer report until the number of layers drops below + // mResetReportingThreshold to ensure that we don't spam logcat. + private int mMaxLayersReportingThreshold = MAX_LAYERS_REPORTING_THRESHOLD; + private int mResetReportingThreshold = RESET_REPORTING_THRESHOLD; + + // Whether the current set of layers has exceeded mMaxLayersReportingThreshold, and we have + // already reported the set of registered surfaces. + private boolean mHasReportedExceedingMaxThreshold = false; + + // The handler for when the registry exceeds the max threshold + private Reporter mReporter = sDefaultReporter; + + private SurfaceControlRegistry() { + mSurfaceControls = new WeakHashMap<>(256); + } + + /** + * Sets the thresholds at which the registry reports errors. + * @param maxLayersReportingThreshold The max threshold (inclusive) + * @param resetReportingThreshold The reset threshold (inclusive) + * @hide + */ + @VisibleForTesting + public void setReportingThresholds(int maxLayersReportingThreshold, int resetReportingThreshold, + Reporter reporter) { + synchronized (sLock) { + if (maxLayersReportingThreshold <= 0 + || resetReportingThreshold >= maxLayersReportingThreshold) { + throw new IllegalArgumentException("Expected maxLayersReportingThreshold (" + + maxLayersReportingThreshold + ") to be > 0 and resetReportingThreshold (" + + resetReportingThreshold + ") to be < maxLayersReportingThreshold"); + } + if (reporter == null) { + throw new IllegalArgumentException("Expected non-null reporter"); + } + mMaxLayersReportingThreshold = maxLayersReportingThreshold; + mResetReportingThreshold = resetReportingThreshold; + mHasReportedExceedingMaxThreshold = false; + mReporter = reporter; + } + } + + /** + * Creates and initializes the registry for all SurfaceControls in this process. The caller must + * hold the READ_FRAME_BUFFER permission. + * @hide + */ + @RequiresPermission(READ_FRAME_BUFFER) + @NonNull + public static void createProcessInstance(Context context) { + if (context.checkSelfPermission(READ_FRAME_BUFFER) != PERMISSION_GRANTED) { + throw new SecurityException("Expected caller to hold READ_FRAME_BUFFER"); + } + synchronized (sLock) { + if (sProcessRegistry == null) { + sProcessRegistry = new SurfaceControlRegistry(); + } + } + } + + /** + * Destroys the previously created registry this process. + * @hide + */ + public static void destroyProcessInstance() { + synchronized (sLock) { + if (sProcessRegistry == null) { + return; + } + sProcessRegistry = null; + } + } + + /** + * Returns the instance of the registry for this process, only non-null if + * createProcessInstance(Context) was previously called from a valid caller. + * @hide + */ + @Nullable + @VisibleForTesting + public static SurfaceControlRegistry getProcessInstance() { + synchronized (sLock) { + return sProcessRegistry; + } + } + + /** + * Adds a SurfaceControl to the registry. + */ + void add(SurfaceControl sc) { + synchronized (sLock) { + mSurfaceControls.put(sc, SystemClock.elapsedRealtime()); + if (!mHasReportedExceedingMaxThreshold + && mSurfaceControls.size() >= mMaxLayersReportingThreshold) { + // Dump existing info to logcat for debugging purposes (but don't close the + // System.out output stream otherwise we can't print to it after this call) + PrintWriter pw = new PrintWriter(System.out, true /* autoFlush */); + mReporter.onMaxLayersExceeded(mSurfaceControls, DUMP_LIMIT, pw); + mHasReportedExceedingMaxThreshold = true; + } + } + } + + /** + * Removes a SurfaceControl from the registry. + */ + void remove(SurfaceControl sc) { + synchronized (sLock) { + mSurfaceControls.remove(sc); + if (mHasReportedExceedingMaxThreshold + && mSurfaceControls.size() <= mResetReportingThreshold) { + mHasReportedExceedingMaxThreshold = false; + } + } + } + + /** + * Returns a hash of this registry and is a function of all the active surface controls. This + * is useful for testing to determine whether the registry has changed between creating and + * destroying new SurfaceControls. + */ + @Override + public int hashCode() { + synchronized (sLock) { + // Return a hash of the surface controls + return mSurfaceControls.keySet().hashCode(); + } + } + + /** + * Forces the gc and finalizers to run, used prior to dumping to ensure we only dump strongly + * referenced surface controls. + */ + private static void runGcAndFinalizers() { + long t = SystemClock.elapsedRealtime(); + GcUtils.runGcAndFinalizersSync(); + Log.i(TAG, "Ran gc and finalizers (" + (SystemClock.elapsedRealtime() - t) + "ms)"); + } + + /** + * Dumps information about the set of SurfaceControls in the registry. + * + * @param limit the number of layers to report + * @param runGc whether to run the GC and finalizers before dumping + * @hide + */ + public static void dump(int limit, boolean runGc, PrintWriter pw) { + if (runGc) { + // This needs to run outside the lock since finalization isn't synchronous + runGcAndFinalizers(); + } + synchronized (sLock) { + if (sProcessRegistry != null) { + sDefaultReporter.onMaxLayersExceeded(sProcessRegistry.mSurfaceControls, limit, pw); + } + } + } +} diff --git a/core/tests/coretests/src/android/view/SurfaceControlRegistryTests.java b/core/tests/coretests/src/android/view/SurfaceControlRegistryTests.java new file mode 100644 index 0000000000000..d10ba7ccbac47 --- /dev/null +++ b/core/tests/coretests/src/android/view/SurfaceControlRegistryTests.java @@ -0,0 +1,173 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package android.view; + +import static android.Manifest.permission.READ_FRAME_BUFFER; +import static android.content.pm.PackageManager.PERMISSION_DENIED; + +import static androidx.test.platform.app.InstrumentationRegistry.getInstrumentation; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; + +import android.content.Context; +import android.platform.test.annotations.Presubmit; + +import androidx.test.runner.AndroidJUnit4; + +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; + +import java.io.PrintWriter; +import java.util.WeakHashMap; + +/** + * Class for testing {@link SurfaceControlRegistry}. + * + * Build/Install/Run: + * atest FrameworksCoreTests:android.view.SurfaceControlRegistryTests + */ +@Presubmit +@RunWith(AndroidJUnit4.class) +public class SurfaceControlRegistryTests { + + @BeforeClass + public static void setUpOnce() { + SurfaceControlRegistry.createProcessInstance(getInstrumentation().getTargetContext()); + } + + @AfterClass + public static void tearDownOnce() { + SurfaceControlRegistry.destroyProcessInstance(); + } + + @Test + public void testRequiresPermissionToCreateProcessInstance() { + try { + Context ctx = mock(Context.class); + doReturn(PERMISSION_DENIED).when(ctx).checkSelfPermission(eq(READ_FRAME_BUFFER)); + SurfaceControlRegistry.createProcessInstance(ctx); + fail("Expected SecurityException due to missing permission"); + } catch (SecurityException se) { + // Expected failure + } catch (Exception e) { + fail("Unexpected exception: " + e); + } + } + + @Test + public void testCreateReleaseSurfaceControl() { + int hash0 = SurfaceControlRegistry.getProcessInstance().hashCode(); + SurfaceControl sc = buildTestSurface(); + assertNotEquals(hash0, SurfaceControlRegistry.getProcessInstance().hashCode()); + sc.release(); + assertEquals(hash0, SurfaceControlRegistry.getProcessInstance().hashCode()); + } + + @Test + public void testCreateReleaseMultipleSurfaceControls() { + int hash0 = SurfaceControlRegistry.getProcessInstance().hashCode(); + SurfaceControl sc1 = buildTestSurface(); + int hash1 = SurfaceControlRegistry.getProcessInstance().hashCode(); + assertNotEquals(hash0, hash1); + SurfaceControl sc2 = buildTestSurface(); + int hash1_2 = SurfaceControlRegistry.getProcessInstance().hashCode(); + assertNotEquals(hash0, hash1_2); + assertNotEquals(hash1, hash1_2); + // Release in inverse order to verify hashes still differ + sc1.release(); + int hash2 = SurfaceControlRegistry.getProcessInstance().hashCode(); + assertNotEquals(hash0, hash2); + sc2.release(); + assertEquals(hash0, SurfaceControlRegistry.getProcessInstance().hashCode()); + } + + @Test + public void testThresholds() { + SurfaceControlRegistry registry = SurfaceControlRegistry.getProcessInstance(); + TestReporter reporter = new TestReporter(); + registry.setReportingThresholds(4 /* max */, 2 /* reset */, reporter); + + // Exceed the threshold ensure the callback is made + SurfaceControl sc1 = buildTestSurface(); + SurfaceControl sc2 = buildTestSurface(); + SurfaceControl sc3 = buildTestSurface(); + SurfaceControl sc4 = buildTestSurface(); + reporter.assertMaxThresholdExceededCallCount(1); + reporter.assertLastReportedSetEquals(sc1, sc2, sc3, sc4); + + // Create a few more, ensure we don't report again for the time being + SurfaceControl sc5 = buildTestSurface(); + SurfaceControl sc6 = buildTestSurface(); + reporter.assertMaxThresholdExceededCallCount(1); + reporter.assertLastReportedSetEquals(sc1, sc2, sc3, sc4); + + // Release down to the reset threshold + sc1.release(); + sc2.release(); + sc3.release(); + sc4.release(); + + // Create a few more to hit the max threshold again + SurfaceControl sc7 = buildTestSurface(); + SurfaceControl sc8 = buildTestSurface(); + reporter.assertMaxThresholdExceededCallCount(2); + reporter.assertLastReportedSetEquals(sc5, sc6, sc7, sc8); + } + + private SurfaceControl buildTestSurface() { + return new SurfaceControl.Builder() + .setContainerLayer() + .setName("SurfaceControlRegistryTests") + .setCallsite("SurfaceControlRegistryTests") + .build(); + } + + private static class TestReporter implements SurfaceControlRegistry.Reporter { + WeakHashMap lastSurfaceControls = new WeakHashMap<>(); + int callCount = 0; + + @Override + public void onMaxLayersExceeded(WeakHashMap surfaceControls, + int limit, PrintWriter pw) { + lastSurfaceControls.clear(); + lastSurfaceControls.putAll(surfaceControls); + callCount++; + } + + public void assertMaxThresholdExceededCallCount(int count) { + assertTrue("Expected " + count + " got " + callCount, count == callCount); + } + + public void assertLastReportedSetEquals(SurfaceControl... surfaces) { + WeakHashMap last = new WeakHashMap<>(lastSurfaceControls); + for (int i = 0; i < surfaces.length; i++) { + last.remove(surfaces[i]); + } + if (!last.isEmpty()) { + fail("Sets differ"); + } + } + } +}