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 */);