Merge "Always remove the observers in cancel to avoid object leakage" into sc-dev

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;
 
@@ -163,6 +163,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);
     }
 
@@ -187,9 +189,13 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener
      */
     public synchronized void end() {
         mEndVsyncId = mChoreographer.getVsyncId();
-        Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId);
+        // Cancel the session if:
-        if (mEndVsyncId == mBeginVsyncId) {
+        // 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.
@@ -200,11 +206,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;
+        // The session is ongoing, end the trace session.
-        Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId);
+        // 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);
         }
     }
@@ -393,7 +407,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) {

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