Merge "Always remove the observers in cancel to avoid object leakage" into sc-dev (a2397bac) · Commits · e / os / android_frameworks_base

core/java/com/android/internal/jank/FrameTracker.java

+25 −7

Original line number	Diff line number	Diff line
		@@ -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);
		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.
		@@ -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.
		// 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

+53 −12

Original line number	Diff line number	Diff line
		@@ -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 */);