From 032d7a0b53d4643ccfda9d2958ba169b6348fc96 Mon Sep 17 00:00:00 2001 From: Ahan Wu Date: Tue, 9 Mar 2021 00:03:39 +0800 Subject: [PATCH] Always remove the observers in cancel to avoid object leakage We didn't remove the observers if the cancel invocation came from end(), this caused leakage of FrameTracker, so we always remove the observers no matter where the cancel invocation comes from. Bug: 181683700 Test: atest FrameworksCoreTests:InteractionJankMonitorTest FrameworksCoreTests: FrameTrackerTest Change-Id: Ia03ebc3b579257dd387eaa1be65b9d22e7b7bf91 --- .../android/internal/jank/FrameTracker.java | 32 +++++++-- .../internal/jank/FrameTrackerTest.java | 65 +++++++++++++++---- 2 files changed, 78 insertions(+), 19 deletions(-) diff --git a/core/java/com/android/internal/jank/FrameTracker.java b/core/java/com/android/internal/jank/FrameTracker.java index db0b48e130a37..3a1e70f02ea39 100644 --- a/core/java/com/android/internal/jank/FrameTracker.java +++ b/core/java/com/android/internal/jank/FrameTracker.java @@ -16,7 +16,6 @@ package com.android.internal.jank; -import static android.view.SurfaceControl.JankData.BUFFER_STUFFING; import static android.view.SurfaceControl.JankData.DISPLAY_HAL; import static android.view.SurfaceControl.JankData.JANK_APP_DEADLINE_MISSED; import static android.view.SurfaceControl.JankData.JANK_NONE; @@ -42,6 +41,7 @@ import android.view.SurfaceControl.JankData.JankType; import android.view.ThreadedRenderer; import android.view.ViewRootImpl; +import com.android.internal.annotations.VisibleForTesting; import com.android.internal.jank.InteractionJankMonitor.Session; import com.android.internal.util.FrameworkStatsLog; @@ -162,6 +162,8 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener }, 50); } }; + + // This callback has a reference to FrameTracker, remember to remove it to avoid leakage. viewRootWrapper.addSurfaceChangedCallback(mSurfaceChangedCallback); } @@ -186,9 +188,13 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener */ public synchronized void end() { mEndVsyncId = mChoreographer.getVsyncId(); - Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId); - if (mEndVsyncId == mBeginVsyncId) { + // Cancel the session if: + // 1. The session begins and ends at the same vsync id. + // 2. The session never begun. + if (mEndVsyncId == mBeginVsyncId || mBeginVsyncId == INVALID_ID) { cancel(); + } else { + Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId); } // We don't remove observer here, // will remove it when all the frame metrics in this duration are called back. @@ -199,11 +205,19 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener * Cancel the trace session of the CUJ. */ public synchronized void cancel() { - if (mBeginVsyncId == INVALID_ID || mEndVsyncId != INVALID_ID) return; - Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId); + // The session is ongoing, end the trace session. + // That means the cancel call is from external invocation, not from end(). + if (mBeginVsyncId != INVALID_ID && mEndVsyncId == INVALID_ID) { + Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId); + } mCancelled = true; + + // Always remove the observers in cancel call to avoid leakage. removeObservers(); - if (mListener != null) { + + // Notify the listener the session has been cancelled. + // We don't notify the listeners if the session never begun. + if (mListener != null && mBeginVsyncId != INVALID_ID) { mListener.onNotifyCujEvents(mSession, InteractionJankMonitor.ACTION_SESSION_CANCEL); } } @@ -392,7 +406,11 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener } } - private void removeObservers() { + /** + * Remove all the registered listeners, observers and callbacks. + */ + @VisibleForTesting + public void removeObservers() { mRendererWrapper.removeObserver(mObserver); mSurfaceControlWrapper.removeJankStatsListener(this); if (mSurfaceChangedCallback != null) { diff --git a/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java b/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java index 9cb7876b3e5a6..64bcc1c379ba0 100644 --- a/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java +++ b/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java @@ -98,9 +98,10 @@ public class FrameTrackerTest { mListenerCapture = ArgumentCaptor.forClass(OnJankDataListener.class); doNothing().when(mSurfaceControlWrapper).addJankStatsListener( mListenerCapture.capture(), any()); + doNothing().when(mSurfaceControlWrapper).removeJankStatsListener( + mListenerCapture.capture()); mChoreographer = mock(ChoreographerWrapper.class); - Session session = new Session(CUJ_NOTIFICATION_SHADE_EXPAND_COLLAPSE); mTracker = Mockito.spy( new FrameTracker(session, handler, mRenderer, mViewRootWrapper, @@ -127,11 +128,12 @@ public class FrameTrackerTest { sendFirstWindowFrame(5, JANK_NONE, 101L); // end the trace session, the last janky frame is after the end() so is discarded. - when(mChoreographer.getVsyncId()).thenReturn(101L); + when(mChoreographer.getVsyncId()).thenReturn(102L); mTracker.end(); - sendFrame(500, JANK_APP_DEADLINE_MISSED, 102L); + sendFrame(5, JANK_NONE, 102L); + sendFrame(500, JANK_APP_DEADLINE_MISSED, 103L); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); verify(mTracker, never()).triggerPerfetto(); } @@ -148,11 +150,11 @@ public class FrameTrackerTest { sendFrame(40, JANK_SURFACEFLINGER_DEADLINE_MISSED, 101L); // end the trace session - when(mChoreographer.getVsyncId()).thenReturn(101L); + when(mChoreographer.getVsyncId()).thenReturn(102L); mTracker.end(); sendFrame(4, JANK_NONE, 102L); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); // We detected a janky frame - trigger Perfetto verify(mTracker).triggerPerfetto(); @@ -171,11 +173,11 @@ public class FrameTrackerTest { sendFrame(4, JANK_NONE, 101L); // end the trace session - when(mChoreographer.getVsyncId()).thenReturn(101L); + when(mChoreographer.getVsyncId()).thenReturn(102L); mTracker.end(); sendFrame(4, JANK_NONE, 102L); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); // We detected a janky frame - trigger Perfetto verify(mTracker, never()).triggerPerfetto(); @@ -194,11 +196,11 @@ public class FrameTrackerTest { sendFrame(40, JANK_APP_DEADLINE_MISSED, 101L); // end the trace session - when(mChoreographer.getVsyncId()).thenReturn(101L); + when(mChoreographer.getVsyncId()).thenReturn(102L); mTracker.end(); sendFrame(4, JANK_NONE, 102L); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); // We detected a janky frame - trigger Perfetto verify(mTracker).triggerPerfetto(); @@ -224,7 +226,7 @@ public class FrameTrackerTest { // One more callback with VSYNC after the end() vsync id. sendFrame(4, JANK_NONE, 103L); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); // We detected a janky frame - trigger Perfetto verify(mTracker).triggerPerfetto(); @@ -246,11 +248,50 @@ public class FrameTrackerTest { sendFrame(50, JANK_APP_DEADLINE_MISSED, 102L); mTracker.cancel(); - verify(mRenderer).removeObserver(any()); + verify(mTracker).removeObservers(); // Since the tracker has been cancelled, shouldn't trigger perfetto. verify(mTracker, never()).triggerPerfetto(); } + @Test + public void testRemoveObserversWhenCancelledInEnd() { + when(mChoreographer.getVsyncId()).thenReturn(100L); + mTracker.begin(); + verify(mRenderer, only()).addObserver(any()); + + // send first frame - not janky + sendFrame(4, JANK_NONE, 100L); + + // send another frame - should be considered janky + sendFrame(40, JANK_APP_DEADLINE_MISSED, 101L); + + // end the trace session + when(mChoreographer.getVsyncId()).thenReturn(101L); + mTracker.end(); + sendFrame(4, JANK_NONE, 102L); + + // Since the begin vsync id (101) equals to the end vsync id (101), will be treat as cancel. + verify(mTracker).cancel(); + + // Observers should be removed in this case, or FrameTracker object will be leaked. + verify(mTracker).removeObservers(); + + // Should never trigger Perfetto since it is a cancel. + verify(mTracker, never()).triggerPerfetto(); + } + + @Test + public void testCancelWhenSessionNeverBegun() { + mTracker.cancel(); + verify(mTracker).removeObservers(); + } + + @Test + public void testEndWhenSessionNeverBegun() { + mTracker.end(); + verify(mTracker).removeObservers(); + } + private void sendFirstWindowFrame(long durationMillis, @JankType int jankType, long vsyncId) { sendFrame(durationMillis, jankType, vsyncId, true /* firstWindowFrame */);