From eae7d291f9c805aefeb2446a8debca87f81dbd90 Mon Sep 17 00:00:00 2001 From: Ahan Wu Date: Wed, 7 Apr 2021 02:39:31 -0800 Subject: Fix memory leak due to not removing ended or cancelled tasks While the tracker session ends abnormaly, we should also remove the task which is kept by InteractionJankMonitor, or a memory leak happens. Bug: 184651704 Test: atest FrameworksCoreTests:InteractionJankMonitorTest Test: atest FrameworksCoreTests:FrameTrackerTest Change-Id: I56f7759ef39d34cedc795042d3210d9be375cf77 --- .../com/android/internal/jank/FrameTracker.java | 13 +++--- .../internal/jank/InteractionJankMonitor.java | 49 ++++++++++++++++++---- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/core/java/com/android/internal/jank/FrameTracker.java b/core/java/com/android/internal/jank/FrameTracker.java index 594a1a72ffcf..135c076275ba 100644 --- a/core/java/com/android/internal/jank/FrameTracker.java +++ b/core/java/com/android/internal/jank/FrameTracker.java @@ -27,6 +27,7 @@ import static android.view.SurfaceControl.JankData.SURFACE_FLINGER_SCHEDULING; import static com.android.internal.jank.InteractionJankMonitor.ACTION_METRICS_LOGGED; import static com.android.internal.jank.InteractionJankMonitor.ACTION_SESSION_BEGIN; import static com.android.internal.jank.InteractionJankMonitor.ACTION_SESSION_CANCEL; +import static com.android.internal.jank.InteractionJankMonitor.ACTION_SESSION_END; import android.annotation.IntDef; import android.annotation.NonNull; @@ -215,7 +216,7 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener mSurfaceControlWrapper.addJankStatsListener(this, mSurfaceControl); } if (mListener != null) { - mListener.onNotifyCujEvents(mSession, ACTION_SESSION_BEGIN); + mListener.onCujEvents(mSession, ACTION_SESSION_BEGIN); } } @@ -240,7 +241,9 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener } Trace.endAsyncSection(mSession.getName(), (int) mBeginVsyncId); mSession.setReason(reason); - InteractionJankMonitor.getInstance().removeTimeout(mSession.getCuj()); + if (mListener != null) { + mListener.onCujEvents(mSession, ACTION_SESSION_END); + } } // We don't remove observer here, // will remove it when all the frame metrics in this duration are called back. @@ -269,7 +272,7 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener // Notify the listener the session has been cancelled. // We don't notify the listeners if the session never begun. if (mListener != null) { - mListener.onNotifyCujEvents(mSession, ACTION_SESSION_CANCEL); + mListener.onCujEvents(mSession, ACTION_SESSION_CANCEL); } } @@ -445,7 +448,7 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener maxFrameTimeNanos, missedSfFramesCounts); if (mListener != null) { - mListener.onNotifyCujEvents(mSession, ACTION_METRICS_LOGGED); + mListener.onCujEvents(mSession, ACTION_METRICS_LOGGED); } } if (DEBUG) { @@ -587,6 +590,6 @@ public class FrameTracker extends SurfaceControl.OnJankDataListener * @param session the CUJ session * @param action the specific action */ - void onNotifyCujEvents(Session session, String action); + void onCujEvents(Session session, String action); } } diff --git a/core/java/com/android/internal/jank/InteractionJankMonitor.java b/core/java/com/android/internal/jank/InteractionJankMonitor.java index 6c56d421a1fb..5ab2a82ffa3c 100644 --- a/core/java/com/android/internal/jank/InteractionJankMonitor.java +++ b/core/java/com/android/internal/jank/InteractionJankMonitor.java @@ -19,7 +19,9 @@ package com.android.internal.jank; import static android.content.Intent.FLAG_RECEIVER_REGISTERED_ONLY; import static com.android.internal.jank.FrameTracker.ChoreographerWrapper; +import static com.android.internal.jank.FrameTracker.REASON_CANCEL_NORMAL; import static com.android.internal.jank.FrameTracker.REASON_CANCEL_NOT_BEGUN; +import static com.android.internal.jank.FrameTracker.REASON_END_NORMAL; import static com.android.internal.jank.FrameTracker.SurfaceControlWrapper; import static com.android.internal.util.FrameworkStatsLog.UIINTERACTION_FRAME_INFO_REPORTED__INTERACTION_TYPE__LAUNCHER_ALL_APPS_SCROLL; import static com.android.internal.util.FrameworkStatsLog.UIINTERACTION_FRAME_INFO_REPORTED__INTERACTION_TYPE__LAUNCHER_APP_CLOSE_TO_HOME; @@ -100,6 +102,7 @@ public class InteractionJankMonitor { private static final int DEFAULT_TRACE_THRESHOLD_FRAME_TIME_MILLIS = 64; public static final String ACTION_SESSION_BEGIN = ACTION_PREFIX + ".ACTION_SESSION_BEGIN"; + public static final String ACTION_SESSION_END = ACTION_PREFIX + ".ACTION_SESSION_END"; public static final String ACTION_SESSION_CANCEL = ACTION_PREFIX + ".ACTION_SESSION_CANCEL"; public static final String ACTION_METRICS_LOGGED = ACTION_PREFIX + ".ACTION_METRICS_LOGGED"; public static final String BUNDLE_KEY_CUJ_NAME = ACTION_PREFIX + ".CUJ_NAME"; @@ -275,10 +278,7 @@ public class InteractionJankMonitor { public FrameTracker createFrameTracker(View v, Session session) { final Context c = v.getContext().getApplicationContext(); synchronized (this) { - boolean needListener = SystemProperties.getBoolean(PROP_NOTIFY_CUJ_EVENT, false); - FrameTrackerListener eventsListener = - !needListener ? null : (s, act) -> notifyEvents(c, act, s); - + FrameTrackerListener eventsListener = (s, act) -> handleCujEvents(c, act, s); return new FrameTracker(session, mWorker.getThreadHandler(), new ThreadedRendererWrapper(v.getThreadedRenderer()), new ViewRootWrapper(v.getViewRootImpl()), new SurfaceControlWrapper(), @@ -287,6 +287,28 @@ public class InteractionJankMonitor { } } + private void handleCujEvents(Context context, String action, Session session) { + // Clear the running and timeout tasks if the end / cancel was fired within the tracker. + // Or we might have memory leaks. + if (needRemoveTasks(action, session)) { + removeTimeout(session.getCuj()); + removeTracker(session.getCuj()); + } + + // Notify the receivers if necessary. + if (session.shouldNotify()) { + notifyEvents(context, action, session); + } + } + + private boolean needRemoveTasks(String action, Session session) { + final boolean badEnd = action.equals(ACTION_SESSION_END) + && session.getReason() != REASON_END_NORMAL; + final boolean badCancel = action.equals(ACTION_SESSION_CANCEL) + && session.getReason() != REASON_CANCEL_NORMAL; + return badEnd || badCancel; + } + private void notifyEvents(Context context, String action, Session session) { if (action.equals(ACTION_SESSION_CANCEL) && session.getReason() == REASON_CANCEL_NOT_BEGUN) { @@ -299,7 +321,7 @@ public class InteractionJankMonitor { context.sendBroadcast(intent); } - void removeTimeout(@CujType int cujType) { + private void removeTimeout(@CujType int cujType) { synchronized (this) { Runnable timeout = mTimeoutActions.get(cujType); if (timeout != null) { @@ -371,7 +393,7 @@ public class InteractionJankMonitor { // Skip this call since we haven't started a trace yet. if (tracker == null) return false; tracker.end(FrameTracker.REASON_END_NORMAL); - mRunningTrackers.remove(cujType); + removeTracker(cujType); return true; } } @@ -390,7 +412,7 @@ public class InteractionJankMonitor { // Skip this call since we haven't started a trace yet. if (tracker == null) return false; tracker.cancel(FrameTracker.REASON_CANCEL_NORMAL); - mRunningTrackers.remove(cujType); + removeTracker(cujType); return true; } } @@ -401,6 +423,12 @@ public class InteractionJankMonitor { } } + private void removeTracker(@CujType int cuj) { + synchronized (this) { + mRunningTrackers.remove(cuj); + } + } + private void updateProperties(DeviceConfig.Properties properties) { synchronized (this) { mSamplingInterval = properties.getInt(SETTINGS_SAMPLING_INTERVAL_KEY, @@ -516,9 +544,11 @@ public class InteractionJankMonitor { private long mTimeStamp; @FrameTracker.Reasons private int mReason = FrameTracker.REASON_END_UNKNOWN; + private boolean mShouldNotify; public Session(@CujType int cujType) { mCujType = cujType; + mShouldNotify = SystemProperties.getBoolean(PROP_NOTIFY_CUJ_EVENT, false); } @CujType @@ -558,5 +588,10 @@ public class InteractionJankMonitor { public int getReason() { return mReason; } + + /** Determine if should notify the receivers of cuj events */ + public boolean shouldNotify() { + return mShouldNotify; + } } } -- cgit v1.2.3-59-g8ed1b