Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 032d7a0b authored by Ahan Wu's avatar Ahan Wu
Browse files

Always remove the observers in cancel to avoid object leakage 

We didn't remove the observers if the cancel invocation came from end(),
this caused leakage of FrameTracker, so we always remove the observers
no matter where the cancel invocation comes from.

Bug: 181683700
Test: atest FrameworksCoreTests:InteractionJankMonitorTest
FrameworksCoreTests: FrameTrackerTest
Change-Id: Ia03ebc3b579257dd387eaa1be65b9d22e7b7bf91
parent 779ea7fe

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;
@@ -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;

        // 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) {

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