Merge "Fix memory leak due to not removing ended or cancelled tasks" into sc-dev

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

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 = (s, act) -> handleCujEvents(c, act, s);
-            FrameTrackerListener eventsListener =
-                    !needListener ? null : (s, act) -> notifyEvents(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;
+        }
     }
 }