From 67ff248168fb77dadd81886df134cbfe048a6001 Mon Sep 17 00:00:00 2001 From: Selim Cinek Date: Thu, 25 May 2017 10:27:28 -0700 Subject: [PATCH] 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 --- .../systemui/statusbar/InflationTask.java | 32 +++++++++++++++++++ .../systemui/statusbar/NotificationData.java | 11 ++++--- .../notification/NotificationInflater.java | 29 ++++++++++++++--- .../notification/RowInflaterTask.java | 4 +-- .../systemui/statusbar/phone/StatusBar.java | 8 +++-- .../NotificationInflaterTest.java | 25 ++++++++++++++- 6 files changed, 94 insertions(+), 15 deletions(-) create mode 100644 packages/SystemUI/src/com/android/systemui/statusbar/InflationTask.java 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 index 0000000000000..22fd37ceaebd2 --- /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 f8bad053c3ee3..1844946e75b6b 100644 --- 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 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 f1c26cd2daa8f..74a3211c9ebdb 100644 --- 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 - 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 @@ -571,6 +582,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 1bfc0cc6a6df6..3491f81215456 100644 --- 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 fc73c0f24cbf3..741725101dff0 100644 --- 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 0c5bdeacbd4e7..407c49599bee7 100644 --- 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,