Android/frameworks_base
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/13774841

MUST ONLY BE SUBMITTED BY AUTOMERGER

Change-Id: I0a42a7682eccc2fbed203cf3827df4c5bbcfeee8
This commit is contained in:
Wu Ahan
2021-03-10 06:30:33 +00:00
committed by Automerger Merge Worker
parent 333f53d7ea a2397bacb2
commit ef836850a6
2 changed files with 78 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
core/java/com/android/internal/jank/FrameTracker.java
View File

@@ -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;
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);
}
}
@@ -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) {

65
core/tests/coretests/src/com/android/internal/jank/FrameTrackerTest.java
View File

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