From 65a3d5dfa4cea24999ee093451ebbd633d9d738a Mon Sep 17 00:00:00 2001
From: Ahan Wu <ahanwu@google.com>
Date: Mon, 19 Jul 2021 09:53:41 -0800
Subject: [PATCH] Fix potential memory leak of FrameTracker

If begin and end invocations are at the same frame, the end vsync id
will be smaller than the begin vsync id, results in zero size jank info
array, the finish call will be missed as well, a leak happens.

Bug: 192140966
Test: atest FrameTrackerTest InteractionJankMonitorTest
Change-Id: I388558e60bdb84ad248a9afabe7776c4e6e67c57
---
 .../android/internal/jank/FrameTracker.java   |  2 +-
 .../internal/jank/FrameTrackerTest.java       | 29 ++++++++++++++-----
 .../jank/InteractionJankMonitorTest.java      |  4 +++
 3 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/core/java/com/android/internal/jank/FrameTracker.java b/core/java/com/android/internal/jank/FrameTracker.java
index f28c42a10978d..8e7fae7aa061e 100644
--- a/core/java/com/android/internal/jank/FrameTracker.java
+++ b/core/java/com/android/internal/jank/FrameTracker.java
@@ -251,7 +251,7 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener
         // 2. The session never begun.
         if (mBeginVsyncId == INVALID_ID) {
             cancel(REASON_CANCEL_NOT_BEGUN);
-        } else if (mEndVsyncId == mBeginVsyncId) {
+        } else if (mEndVsyncId <= mBeginVsyncId) {
             cancel(REASON_CANCEL_SAME_VSYNC);
         } else {
             if (DEBUG) {
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 c8f8ca9fd5b08..96b4316ffafcc 100644
--- a/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java
+++ b/core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java
@@ -257,21 +257,14 @@ public class FrameTrackerTest {
     }
 
     @Test
-    public void testRemoveObserversWhenCancelledInEnd() {
+    public void testCancelIfEndVsyncIdEqualsToBeginVsyncId() {
         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(FrameTracker.REASON_END_NORMAL);
-        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(FrameTracker.REASON_CANCEL_SAME_VSYNC);
@@ -283,6 +276,26 @@ public class FrameTrackerTest {
         verify(mTracker, never()).triggerPerfetto();
     }
 
+    @Test
+    public void testCancelIfEndVsyncIdLessThanBeginVsyncId() {
+        when(mChoreographer.getVsyncId()).thenReturn(100L);
+        mTracker.begin();
+        verify(mRenderer, only()).addObserver(any());
+
+        // end the trace session at the same vsync id, end vsync id will less than the begin one.
+        // Because the begin vsync id is supposed to the next frame,
+        mTracker.end(FrameTracker.REASON_END_NORMAL);
+
+        // The begin vsync id (101) is larger than the end one (100), will be treat as cancel.
+        verify(mTracker).cancel(FrameTracker.REASON_CANCEL_SAME_VSYNC);
+
+        // 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(FrameTracker.REASON_CANCEL_NORMAL);
diff --git a/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java b/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java
index 8ec1559861f3c..c153b38d3f020 100644
--- a/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java
+++ b/core/tests/coretests/src/com/android/internal/jank/InteractionJankMonitorTest.java
@@ -99,6 +99,8 @@ public class InteractionJankMonitorTest {
                 new FrameMetricsWrapper(), /*traceThresholdMissedFrames=*/ 1,
                 /*traceThresholdFrameTimeMillis=*/ -1, null));
         doReturn(tracker).when(monitor).createFrameTracker(any(), any());
+        doNothing().when(tracker).triggerPerfetto();
+        doNothing().when(tracker).postTraceStartMarker();
 
         // Simulate a trace session and see if begin / end are invoked.
         assertThat(monitor.begin(mView, session.getCuj())).isTrue();
@@ -146,6 +148,8 @@ public class InteractionJankMonitorTest {
                 new FrameMetricsWrapper(), /*traceThresholdMissedFrames=*/ 1,
                 /*traceThresholdFrameTimeMillis=*/ -1, null));
         doReturn(tracker).when(monitor).createFrameTracker(any(), any());
+        doNothing().when(tracker).triggerPerfetto();
+        doNothing().when(tracker).postTraceStartMarker();
 
         assertThat(monitor.begin(mView, session.getCuj())).isTrue();
         verify(tracker).begin();