Fixed memory leak with the inflater

Because min priority children could be removed from
their parents after the removal, a new inflation task
would be started, leading to the view being instantly
readded again. This lead to memory leaks.

It also fixes a bug where the inflation would not inflate
enough views that could lead to crashes / wrong layouts.

Finally there was a indexing error when handling removal
of group summaries.

Test: runtest -x packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationInflaterTest.java
Change-Id: Iac242946bd30060967ee7877560d40e63f39f996
Fixes: 62067645

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/InflationTask.java b/packages/SystemUI/src/com/android/systemui/statusbar/InflationTask.java
new file mode 100644 (file)
index 0000000..22fd37c
--- /dev/null
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/InflationTask.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright (C) 2017 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License
+ */
+
+package com.android.systemui.statusbar;
+
+/**
+ * An interface for running inflation tasks that allows aborting and superseding existing
+ * operations.
+ */
+public interface InflationTask {
+    void abort();
+
+    /**
+     * Supersedes an existing task. i.e another task was superceeded by this.
+     *
+     * @param task the task that was previously running
+     */
+    default void supersedeTask(InflationTask task) {}
+}

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationData.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationData.java
index f8bad05..1844946 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationData.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationData.java
@@ -24,7 +24,6 @@ import android.content.pm.IPackageManager;
 import android.content.pm.PackageManager;
 import android.content.Context;
 import android.graphics.drawable.Icon;
-import android.os.AsyncTask;
 import android.os.RemoteException;
 import android.os.SystemClock;
 import android.service.notification.NotificationListenerService;
@@ -84,7 +83,7 @@ public class NotificationData {
         public List<SnoozeCriterion> snoozeCriteria;
         private int mCachedContrastColor = COLOR_INVALID;
         private int mCachedContrastColorIsFor = COLOR_INVALID;
-        private Abortable mRunningTask = null;
+        private InflationTask mRunningTask = null;
 
         public Entry(StatusBarNotification n) {
             this.key = n.getKey();
@@ -225,10 +224,14 @@ public class NotificationData {
             }
         }
 
-        public void setInflationTask(Abortable abortableTask) {
+        public void setInflationTask(InflationTask abortableTask) {
             // abort any existing inflation
+            InflationTask existing = mRunningTask;
             abortTask();
             mRunningTask = abortableTask;
+            if (existing != null && mRunningTask != null) {
+                mRunningTask.supersedeTask(existing);
+            }
         }
 
         public void onInflationTaskFinished() {
@@ -236,7 +239,7 @@ public class NotificationData {
         }
 
         @VisibleForTesting
-        public Abortable getRunningTask() {
+        public InflationTask getRunningTask() {
             return mRunningTask;
         }
     }

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInflater.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInflater.java
index f1c26cd..74a3211 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInflater.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/NotificationInflater.java
@@ -28,7 +28,7 @@ import android.widget.RemoteViews;
 
 import com.android.internal.annotations.VisibleForTesting;
 import com.android.systemui.R;
-import com.android.systemui.statusbar.Abortable;
+import com.android.systemui.statusbar.InflationTask;
 import com.android.systemui.statusbar.ExpandableNotificationRow;
 import com.android.systemui.statusbar.NotificationContentView;
 import com.android.systemui.statusbar.NotificationData;
@@ -122,6 +122,12 @@ public class NotificationInflater {
      */
     @VisibleForTesting
     void inflateNotificationViews(int reInflateFlags) {
+        if (mRow.isRemoved()) {
+            // We don't want to reinflate anything for removed notifications. Otherwise views might
+            // be readded to the stack, leading to leaks. This may happen with low-priority groups
+            // where the removal of already removed children can lead to a reinflation.
+            return;
+        }
         StatusBarNotification sbn = mRow.getEntry().notification;
         new AsyncInflationTask(sbn, reInflateFlags, mRow, mIsLowPriority,
                 mIsChildInGroup, mUsesIncreasedHeight, mUsesIncreasedHeadsUpHeight, mRedactAmbient,
@@ -477,17 +483,17 @@ public class NotificationInflater {
     }
 
     public static class AsyncInflationTask extends AsyncTask<Void, Void, InflationProgress>
-            implements InflationCallback, Abortable {
+            implements InflationCallback, InflationTask {
 
         private final StatusBarNotification mSbn;
         private final Context mContext;
-        private final int mReInflateFlags;
         private final boolean mIsLowPriority;
         private final boolean mIsChildInGroup;
         private final boolean mUsesIncreasedHeight;
         private final InflationCallback mCallback;
         private final boolean mUsesIncreasedHeadsUpHeight;
         private final boolean mRedactAmbient;
+        private int mReInflateFlags;
         private ExpandableNotificationRow mRow;
         private Exception mError;
         private RemoteViews.OnClickHandler mRemoteViewClickHandler;
@@ -500,8 +506,6 @@ public class NotificationInflater {
                 InflationCallback callback,
                 RemoteViews.OnClickHandler remoteViewClickHandler) {
             mRow = row;
-            NotificationData.Entry entry = row.getEntry();
-            entry.setInflationTask(this);
             mSbn = notification;
             mReInflateFlags = reInflateFlags;
             mContext = mRow.getContext();
@@ -512,6 +516,13 @@ public class NotificationInflater {
             mRedactAmbient = redactAmbient;
             mRemoteViewClickHandler = remoteViewClickHandler;
             mCallback = callback;
+            NotificationData.Entry entry = row.getEntry();
+            entry.setInflationTask(this);
+        }
+
+        @VisibleForTesting
+        public int getReInflateFlags() {
+            return mReInflateFlags;
         }
 
         @Override
@@ -572,6 +583,14 @@ public class NotificationInflater {
         }
 
         @Override
+        public void supersedeTask(InflationTask task) {
+            if (task instanceof AsyncInflationTask) {
+                // We want to inflate all flags of the previous task as well
+                mReInflateFlags |= ((AsyncInflationTask) task).mReInflateFlags;
+            }
+        }
+
+        @Override
         public void handleInflationException(StatusBarNotification notification, Exception e) {
             handleError(e);
         }

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/RowInflaterTask.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/RowInflaterTask.java
index 1bfc0cc..3491f81 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/RowInflaterTask.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/RowInflaterTask.java
@@ -22,14 +22,14 @@ import android.view.View;
 import android.view.ViewGroup;
 
 import com.android.systemui.R;
-import com.android.systemui.statusbar.Abortable;
+import com.android.systemui.statusbar.InflationTask;
 import com.android.systemui.statusbar.ExpandableNotificationRow;
 import com.android.systemui.statusbar.NotificationData;
 
 /**
  * An inflater task that asynchronously inflates a ExpandableNotificationRow
  */
-public class RowInflaterTask implements Abortable, AsyncLayoutInflater.OnInflateFinishedListener {
+public class RowInflaterTask implements InflationTask, AsyncLayoutInflater.OnInflateFinishedListener {
     private RowInflationFinishedListener mListener;
     private NotificationData.Entry mEntry;
     private boolean mCancelled;

diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
index fc73c0f..7417251 100644 (file)
--- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
+++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/StatusBar.java
@@ -1618,7 +1618,9 @@ public class StatusBar extends SystemUI implements DemoMode,
     @Override
     public void onAsyncInflationFinished(Entry entry) {
         mPendingNotifications.remove(entry.key);
-        if (mNotificationData.get(entry.key) == null) {
+        // If there was an async task started after the removal, we don't want to add it back to
+        // the list, otherwise we might get leaks.
+        if (mNotificationData.get(entry.key) == null && !entry.row.isRemoved()) {
             addEntry(entry);
         }
     }
@@ -1768,10 +1770,10 @@ public class StatusBar extends SystemUI implements DemoMode,
                     continue;
                 }
                 toRemove.add(row);
-                toRemove.get(i).setKeepInParent(true);
+                row.setKeepInParent(true);
                 // we need to set this state earlier as otherwise we might generate some weird
                 // animations
-                toRemove.get(i).setRemoved();
+                row.setRemoved();
             }
         }
     }

diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationInflaterTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationInflaterTest.java
index 0c5bdea..407c495 100644 (file)
--- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationInflaterTest.java
+++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/NotificationInflaterTest.java
@@ -35,6 +35,7 @@ import com.android.systemui.R;
 import com.android.systemui.statusbar.ExpandableNotificationRow;
 import com.android.systemui.statusbar.NotificationData;
 import com.android.systemui.statusbar.NotificationTestHelper;
+import com.android.systemui.statusbar.InflationTask;
 
 import org.junit.Assert;
 import org.junit.Before;
@@ -129,7 +130,29 @@ public class NotificationInflaterTest {
         mRow.getEntry().abortTask();
         runThenWaitForInflation(() -> mNotificationInflater.inflateNotificationViews(),
                 mNotificationInflater);
-        Assert.assertNull(mRow.getEntry().getRunningTask() );
+        verify(mRow).onNotificationUpdated();
+    }
+
+    @Test
+    public void testRemovedNotInflated() throws Exception {
+        mRow.setRemoved();
+        mNotificationInflater.inflateNotificationViews();
+        Assert.assertNull(mRow.getEntry().getRunningTask());
+    }
+
+
+    @Test
+    public void testSupersedesExistingTask() throws Exception {
+        mNotificationInflater.inflateNotificationViews();
+        mNotificationInflater.setIsLowPriority(true);
+        mNotificationInflater.setIsChildInGroup(true);
+        InflationTask runningTask = mRow.getEntry().getRunningTask();
+        NotificationInflater.AsyncInflationTask asyncInflationTask =
+                (NotificationInflater.AsyncInflationTask) runningTask;
+        Assert.assertSame("Successive inflations don't inherit the previous flags!",
+                asyncInflationTask.getReInflateFlags(),
+                NotificationInflater.FLAG_REINFLATE_ALL);
+        runningTask.abort();
     }
 
     public static void runThenWaitForInflation(Runnable block,