From f7213e7093755db358cd8525eefe2417cb21236a Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 2 Dec 2021 03:21:44 +0000 Subject: [PATCH 1/2] Disable pooling of MessagingGroup and MessagingMessage instances. A bug with the pooling may be causing a content crossover between notifications that (despite being purely visual) is extremely unnerving for users. Bug: 208508846 Test: manual Change-Id: Iabfd62ba4ecc776805eae0c5e6e807856c682441 --- .../internal/widget/MessagingGroup.java | 7 +-- .../widget/MessagingImageMessage.java | 7 +-- .../internal/widget/MessagingPool.java | 55 +++++++++++++++++++ .../internal/widget/MessagingTextMessage.java | 7 +-- 4 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 core/java/com/android/internal/widget/MessagingPool.java diff --git a/core/java/com/android/internal/widget/MessagingGroup.java b/core/java/com/android/internal/widget/MessagingGroup.java index f30b8442dc359..9e06e33b79b5e 100644 --- a/core/java/com/android/internal/widget/MessagingGroup.java +++ b/core/java/com/android/internal/widget/MessagingGroup.java @@ -32,7 +32,6 @@ import android.graphics.drawable.Icon; import android.text.TextUtils; import android.util.AttributeSet; import android.util.DisplayMetrics; -import android.util.Pools; import android.util.TypedValue; import android.view.LayoutInflater; import android.view.View; @@ -57,8 +56,8 @@ import java.util.List; */ @RemoteViews.RemoteView public class MessagingGroup extends LinearLayout implements MessagingLinearLayout.MessagingChild { - private static Pools.SimplePool sInstancePool - = new Pools.SynchronizedPool<>(10); + private static final MessagingPool sInstancePool = + new MessagingPool<>(10); /** * Images are displayed inline. @@ -338,7 +337,7 @@ public class MessagingGroup extends LinearLayout implements MessagingLinearLayou } public static void dropCache() { - sInstancePool = new Pools.SynchronizedPool<>(10); + sInstancePool.clear(); } @Override diff --git a/core/java/com/android/internal/widget/MessagingImageMessage.java b/core/java/com/android/internal/widget/MessagingImageMessage.java index 27689d4d43f95..f7955c3f72dad 100644 --- a/core/java/com/android/internal/widget/MessagingImageMessage.java +++ b/core/java/com/android/internal/widget/MessagingImageMessage.java @@ -28,7 +28,6 @@ import android.graphics.drawable.Drawable; import android.net.Uri; import android.util.AttributeSet; import android.util.Log; -import android.util.Pools; import android.view.LayoutInflater; import android.view.ViewGroup; import android.widget.ImageView; @@ -44,8 +43,8 @@ import java.io.IOException; @RemoteViews.RemoteView public class MessagingImageMessage extends ImageView implements MessagingMessage { private static final String TAG = "MessagingImageMessage"; - private static Pools.SimplePool sInstancePool - = new Pools.SynchronizedPool<>(10); + private static final MessagingPool sInstancePool = + new MessagingPool<>(10); private final MessagingMessageState mState = new MessagingMessageState(this); private final int mMinImageHeight; private final Path mPath = new Path(); @@ -194,7 +193,7 @@ public class MessagingImageMessage extends ImageView implements MessagingMessage } public static void dropCache() { - sInstancePool = new Pools.SynchronizedPool<>(10); + sInstancePool.clear(); } @Override diff --git a/core/java/com/android/internal/widget/MessagingPool.java b/core/java/com/android/internal/widget/MessagingPool.java new file mode 100644 index 0000000000000..1c2c0151bd996 --- /dev/null +++ b/core/java/com/android/internal/widget/MessagingPool.java @@ -0,0 +1,55 @@ +/* + * Copyright (C) 2021 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.internal.widget; + +import android.util.Pools; + +/** + * A trivial wrapper around Pools.SynchronizedPool which allows clearing the pool, as well as + * disabling the pool class altogether. + * @param the type of object in the pool + */ +public class MessagingPool implements Pools.Pool { + private static final boolean ENABLED = false; // disabled to test b/208508846 + private final int mMaxPoolSize; + private Pools.SynchronizedPool mCurrentPool; + + public MessagingPool(int maxPoolSize) { + mMaxPoolSize = maxPoolSize; + if (ENABLED) { + mCurrentPool = new Pools.SynchronizedPool<>(mMaxPoolSize); + } + } + + @Override + public T acquire() { + return ENABLED ? mCurrentPool.acquire() : null; + } + + @Override + public boolean release(T instance) { + return ENABLED && mCurrentPool.release(instance); + } + + /** Clear the pool */ + public void clear() { + if (ENABLED) { + mCurrentPool = new Pools.SynchronizedPool<>(mMaxPoolSize); + } + } + +} diff --git a/core/java/com/android/internal/widget/MessagingTextMessage.java b/core/java/com/android/internal/widget/MessagingTextMessage.java index d778c59670462..19791dbad31ed 100644 --- a/core/java/com/android/internal/widget/MessagingTextMessage.java +++ b/core/java/com/android/internal/widget/MessagingTextMessage.java @@ -24,7 +24,6 @@ import android.app.Notification; import android.content.Context; import android.text.Layout; import android.util.AttributeSet; -import android.util.Pools; import android.view.LayoutInflater; import android.widget.RemoteViews; @@ -36,8 +35,8 @@ import com.android.internal.R; @RemoteViews.RemoteView public class MessagingTextMessage extends ImageFloatingTextView implements MessagingMessage { - private static Pools.SimplePool sInstancePool - = new Pools.SynchronizedPool<>(20); + private static final MessagingPool sInstancePool = + new MessagingPool<>(20); private final MessagingMessageState mState = new MessagingMessageState(this); public MessagingTextMessage(@NonNull Context context) { @@ -92,7 +91,7 @@ public class MessagingTextMessage extends ImageFloatingTextView implements Messa } public static void dropCache() { - sInstancePool = new Pools.SynchronizedPool<>(10); + sInstancePool.clear(); } @Override From 33d97893e6e1e82dcf70a9476efaf0edb1c1049f Mon Sep 17 00:00:00 2001 From: Jeff DeCew Date: Thu, 2 Dec 2021 03:56:09 +0000 Subject: [PATCH 2/2] Add safety checks for MessagingPool and other view reuse conditions Bug: 208508846 Test: manual Change-Id: I951c0f60bbf354e7dd2aa2f2f028b6e1ca955976 --- .../internal/widget/ConversationLayout.java | 4 ++++ .../internal/widget/MessagingLayout.java | 4 ++++ .../internal/widget/MessagingPool.java | 24 ++++++++++++++++--- 3 files changed, 29 insertions(+), 3 deletions(-) diff --git a/core/java/com/android/internal/widget/ConversationLayout.java b/core/java/com/android/internal/widget/ConversationLayout.java index e6deada45fc1a..78bb53d335393 100644 --- a/core/java/com/android/internal/widget/ConversationLayout.java +++ b/core/java/com/android/internal/widget/ConversationLayout.java @@ -871,6 +871,10 @@ public class ConversationLayout extends FrameLayout if (newGroup == null) { newGroup = MessagingGroup.createGroup(mMessagingLinearLayout); mAddedGroups.add(newGroup); + } else if (newGroup.getParent() != mMessagingLinearLayout) { + throw new IllegalStateException( + "group parent was " + newGroup.getParent() + " but expected " + + mMessagingLinearLayout); } newGroup.setImageDisplayLocation(mIsCollapsed ? IMAGE_DISPLAY_LOCATION_EXTERNAL diff --git a/core/java/com/android/internal/widget/MessagingLayout.java b/core/java/com/android/internal/widget/MessagingLayout.java index e1602a9819204..21ca196886ab8 100644 --- a/core/java/com/android/internal/widget/MessagingLayout.java +++ b/core/java/com/android/internal/widget/MessagingLayout.java @@ -419,6 +419,10 @@ public class MessagingLayout extends FrameLayout if (newGroup == null) { newGroup = MessagingGroup.createGroup(mMessagingLinearLayout); mAddedGroups.add(newGroup); + } else if (newGroup.getParent() != mMessagingLinearLayout) { + throw new IllegalStateException( + "group parent was " + newGroup.getParent() + " but expected " + + mMessagingLinearLayout); } newGroup.setImageDisplayLocation(mIsCollapsed ? IMAGE_DISPLAY_LOCATION_EXTERNAL diff --git a/core/java/com/android/internal/widget/MessagingPool.java b/core/java/com/android/internal/widget/MessagingPool.java index 1c2c0151bd996..9c0fe4b07beb8 100644 --- a/core/java/com/android/internal/widget/MessagingPool.java +++ b/core/java/com/android/internal/widget/MessagingPool.java @@ -16,15 +16,18 @@ package com.android.internal.widget; +import android.util.Log; import android.util.Pools; +import android.view.View; /** * A trivial wrapper around Pools.SynchronizedPool which allows clearing the pool, as well as * disabling the pool class altogether. * @param the type of object in the pool */ -public class MessagingPool implements Pools.Pool { +public class MessagingPool implements Pools.Pool { private static final boolean ENABLED = false; // disabled to test b/208508846 + private static final String TAG = "MessagingPool"; private final int mMaxPoolSize; private Pools.SynchronizedPool mCurrentPool; @@ -37,12 +40,27 @@ public class MessagingPool implements Pools.Pool { @Override public T acquire() { - return ENABLED ? mCurrentPool.acquire() : null; + if (!ENABLED) { + return null; + } + T instance = mCurrentPool.acquire(); + if (instance.getParent() != null) { + Log.wtf(TAG, "acquired " + instance + " with parent " + instance.getParent()); + return null; + } + return instance; } @Override public boolean release(T instance) { - return ENABLED && mCurrentPool.release(instance); + if (instance.getParent() != null) { + Log.wtf(TAG, "releasing " + instance + " with parent " + instance.getParent()); + return false; + } + if (!ENABLED) { + return false; + } + return mCurrentPool.release(instance); } /** Clear the pool */