From f8f3b1489ab28c7ceca8432f17aae8d0ebf595b4 Mon Sep 17 00:00:00 2001 From: Andrew Grieve Date: Wed, 21 Oct 2020 16:50:43 +0000 Subject: [PATCH] Android: Add CollectionUtil.strengthen() helper Abstracts the pattern of looping over weak references and removing stale ones. * Converts UnownedUserDataKey to use WeakSet since order does not matter. TBR=agrieve # Trivial refactor in AutofillManagerWrapper.java Bug: 1131047 Change-Id: Idf226faecb31de69f7b7bbae32d6c795dca3dca2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2477050 Commit-Queue: Andrew Grieve Reviewed-by: Tommy Nyquist Cr-Commit-Position: refs/heads/master@{#819407} --- base/BUILD.gn | 1 + .../org/chromium/base/CallbackController.java | 5 +- .../src/org/chromium/base/CollectionUtil.java | 27 ++++++++++ .../org/chromium/base/UnownedUserDataKey.java | 54 ++++++++----------- .../org/chromium/base/CollectionUtilTest.java | 37 +++++++++++++ .../usage_stats/UsageStatsService.java | 10 ++-- .../autofill/AutofillManagerWrapper.java | 20 +------ .../browser_ui/util/BitmapCache.java | 10 +--- 8 files changed, 97 insertions(+), 67 deletions(-) create mode 100644 base/android/junit/src/org/chromium/base/CollectionUtilTest.java diff --git a/base/BUILD.gn b/base/BUILD.gn index 5f62f2e871778c..b66dfe8873fcf2 100644 --- a/base/BUILD.gn +++ b/base/BUILD.gn @@ -3926,6 +3926,7 @@ if (is_android) { sources = [ "android/junit/src/org/chromium/base/ApplicationStatusTest.java", "android/junit/src/org/chromium/base/CallbackControllerTest.java", + "android/junit/src/org/chromium/base/CollectionUtilTest.java", "android/junit/src/org/chromium/base/DiscardableReferencePoolTest.java", "android/junit/src/org/chromium/base/FileUtilsTest.java", "android/junit/src/org/chromium/base/LifetimeAssertTest.java", diff --git a/base/android/java/src/org/chromium/base/CallbackController.java b/base/android/java/src/org/chromium/base/CallbackController.java index cb55ce0c3f02cf..be0070ac06a496 100644 --- a/base/android/java/src/org/chromium/base/CallbackController.java +++ b/base/android/java/src/org/chromium/base/CallbackController.java @@ -207,9 +207,8 @@ public Runnable makeCancelable(@NonNull Runnable runnable) { public void destroy() { try (AutoCloseableLock acl = AutoCloseableLock.lock(mReadWriteLock.writeLock())) { checkNotCanceled(); - for (WeakReference cancelableWeakReference : mCancelables) { - Cancelable cancelable = cancelableWeakReference.get(); - if (cancelable != null) cancelable.cancel(); + for (Cancelable cancelable : CollectionUtil.strengthen(mCancelables)) { + cancelable.cancel(); } mCancelables = null; } diff --git a/base/android/java/src/org/chromium/base/CollectionUtil.java b/base/android/java/src/org/chromium/base/CollectionUtil.java index 7e4c5b25c60b22..fc22c006f96f2e 100644 --- a/base/android/java/src/org/chromium/base/CollectionUtil.java +++ b/base/android/java/src/org/chromium/base/CollectionUtil.java @@ -8,10 +8,13 @@ import androidx.annotation.NonNull; +import java.lang.ref.WeakReference; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Map; import java.util.Map.Entry; @@ -80,4 +83,28 @@ public static void forEach( worker.onResult((Map.Entry) entry); } } + + /** + * Removes null entries from the given collection and then returns a list of strong references. + * + * Note: This helper is relevant if you have a List> or a Map with weak values. + * For Set>, use Collections.newSetFromMap(new WeakHashMap()) instead. + * + * @param weakRefs Collection to iterate. + * @return List of strong references. + */ + public static List strengthen(Collection> weakRefs) { + ArrayList ret = new ArrayList<>(weakRefs.size()); + Iterator> it = weakRefs.iterator(); + while (it.hasNext()) { + WeakReference weakRef = it.next(); + T strongRef = weakRef.get(); + if (strongRef == null) { + it.remove(); + } else { + ret.add(strongRef); + } + } + return ret; + } } diff --git a/base/android/java/src/org/chromium/base/UnownedUserDataKey.java b/base/android/java/src/org/chromium/base/UnownedUserDataKey.java index 603490781f0b49..38b10b321f584f 100644 --- a/base/android/java/src/org/chromium/base/UnownedUserDataKey.java +++ b/base/android/java/src/org/chromium/base/UnownedUserDataKey.java @@ -8,12 +8,11 @@ import androidx.annotation.Nullable; import androidx.annotation.VisibleForTesting; -import java.lang.ref.WeakReference; import java.util.ArrayList; -import java.util.Collection; -import java.util.HashSet; +import java.util.Collections; import java.util.Objects; import java.util.Set; +import java.util.WeakHashMap; /** * UnownedUserDataKey is used in conjunction with a particular {@link UnownedUserData} as the key @@ -63,7 +62,9 @@ public final class UnownedUserDataKey { @NonNull private final Class mClazz; - private final Set> mHostAttachments = new HashSet<>(); + // A Set that uses WeakReference internally. + private final Set mWeakHostAttachments = + Collections.newSetFromMap(new WeakHashMap<>()); /** * Constructs a key to use for attaching to a particular {@link UnownedUserDataHost}. @@ -93,7 +94,7 @@ public final void attachToHost(@NonNull UnownedUserDataHost host, @NonNull T obj host.set(this, object); if (!isAttachedToHost(host)) { - mHostAttachments.add(new WeakReference<>(host)); + mWeakHostAttachments.add(host); } } @@ -108,7 +109,8 @@ public final void attachToHost(@NonNull UnownedUserDataHost host, @NonNull T obj */ @Nullable public final T retrieveDataFromHost(@NonNull UnownedUserDataHost host) { - for (UnownedUserDataHost attachedHost : getStrongRefs()) { + assertNoDestroyedAttachments(); + for (UnownedUserDataHost attachedHost : mWeakHostAttachments) { if (host.equals(attachedHost)) { return host.get(this); } @@ -123,7 +125,8 @@ public final T retrieveDataFromHost(@NonNull UnownedUserDataHost host) { * @param host The host to detach from. */ public final void detachFromHost(@NonNull UnownedUserDataHost host) { - for (UnownedUserDataHost attachedHost : getStrongRefs()) { + assertNoDestroyedAttachments(); + for (UnownedUserDataHost attachedHost : new ArrayList<>(mWeakHostAttachments)) { if (host.equals(attachedHost)) { removeHostAttachment(attachedHost); } @@ -135,7 +138,8 @@ public final void detachFromHost(@NonNull UnownedUserDataHost host) { * this key. It is OK to call this for already detached objects. */ public final void detachFromAllHosts(@NonNull T object) { - for (UnownedUserDataHost attachedHost : getStrongRefs()) { + assertNoDestroyedAttachments(); + for (UnownedUserDataHost attachedHost : new ArrayList<>(mWeakHostAttachments)) { if (object.equals(attachedHost.get(this))) { removeHostAttachment(attachedHost); } @@ -162,8 +166,9 @@ public final boolean isAttachedToAnyHost(@NonNull T object) { @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) /* package */ int getHostAttachmentCount(@NonNull T object) { + assertNoDestroyedAttachments(); int ret = 0; - for (UnownedUserDataHost attachedHost : getStrongRefs()) { + for (UnownedUserDataHost attachedHost : mWeakHostAttachments) { if (object.equals(attachedHost.get(this))) { ret++; } @@ -173,32 +178,17 @@ public final boolean isAttachedToAnyHost(@NonNull T object) { private void removeHostAttachment(UnownedUserDataHost host) { host.remove(this); - for (WeakReference hostWeakReference : mHostAttachments) { - if (host.equals(hostWeakReference.get())) { - // Modifying mHostAttachments while iterating over it is okay here because we - // break out of the loop right away. - mHostAttachments.remove(hostWeakReference); - return; - } - } + mWeakHostAttachments.remove(host); } - // TODO(https://crbug.com/1131047): Make a //base helper for this. - private Collection getStrongRefs() { - ArrayList ret = new ArrayList<>(); - Set> hosts = new HashSet<>(mHostAttachments); - for (WeakReference hostWeakReference : hosts) { - UnownedUserDataHost hostStrongReference = hostWeakReference.get(); - if (hostStrongReference == null) { - mHostAttachments.remove(hostWeakReference); - continue; + private void assertNoDestroyedAttachments() { + if (BuildConfig.DCHECK_IS_ON) { + for (UnownedUserDataHost attachedHost : mWeakHostAttachments) { + if (attachedHost.isDestroyed()) { + assert false : "Host should have been removed already."; + throw new IllegalStateException(); + } } - if (hostStrongReference.isDestroyed()) { - assert false : "Host should have been removed already."; - throw new IllegalStateException(); - } - ret.add(hostStrongReference); } - return ret; } } diff --git a/base/android/junit/src/org/chromium/base/CollectionUtilTest.java b/base/android/junit/src/org/chromium/base/CollectionUtilTest.java new file mode 100644 index 00000000000000..90a1c205d44f9f --- /dev/null +++ b/base/android/junit/src/org/chromium/base/CollectionUtilTest.java @@ -0,0 +1,37 @@ +// Copyright 2020 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +package org.chromium.base; + +import static org.junit.Assert.assertEquals; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.annotation.Config; + +import org.chromium.base.test.BaseRobolectricTestRunner; + +import java.lang.ref.WeakReference; +import java.util.ArrayList; +import java.util.Arrays; + +/** Unit tests for {@link Log}. */ +@RunWith(BaseRobolectricTestRunner.class) +@Config(manifest = Config.NONE) +public class CollectionUtilTest { + /** Tests that the computed call origin is the correct one. */ + @Test + public void testStrengthen() { + // Java never GC's small constants, so there's no risk of the weak refs becoming null. + ArrayList> weakList = new ArrayList<>(); + weakList.add(new WeakReference<>(0)); + weakList.add(new WeakReference<>(1)); + weakList.add(new WeakReference<>(2)); + + assertEquals(Arrays.asList(0, 1, 2), CollectionUtil.strengthen(weakList)); + + weakList.set(1, new WeakReference<>(null)); + assertEquals(Arrays.asList(0, 2), CollectionUtil.strengthen(weakList)); + } +} diff --git a/chrome/android/java/src/org/chromium/chrome/browser/usage_stats/UsageStatsService.java b/chrome/android/java/src/org/chromium/chrome/browser/usage_stats/UsageStatsService.java index dc619ddbcbb6fc..7439e1a3f00919 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/usage_stats/UsageStatsService.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/usage_stats/UsageStatsService.java @@ -9,6 +9,7 @@ import androidx.annotation.VisibleForTesting; import org.chromium.base.BuildInfo; +import org.chromium.base.CollectionUtil; import org.chromium.base.Log; import org.chromium.base.Promise; import org.chromium.base.ThreadUtils; @@ -249,12 +250,9 @@ public List getAllSuspendedWebsites() { } private void notifyObserversOfSuspensions(List fqdns, boolean suspended) { - for (WeakReference observerRef : mPageViewObservers) { - PageViewObserver observer = observerRef.get(); - if (observer != null) { - for (String fqdn : fqdns) { - observer.notifySiteSuspensionChanged(fqdn, suspended); - } + for (PageViewObserver observer : CollectionUtil.strengthen(mPageViewObservers)) { + for (String fqdn : fqdns) { + observer.notifySiteSuspensionChanged(fqdn, suspended); } } } diff --git a/components/autofill/android/provider/java/src/org/chromium/components/autofill/AutofillManagerWrapper.java b/components/autofill/android/provider/java/src/org/chromium/components/autofill/AutofillManagerWrapper.java index 0287ac38f97a36..28b231d82a7e16 100644 --- a/components/autofill/android/provider/java/src/org/chromium/components/autofill/AutofillManagerWrapper.java +++ b/components/autofill/android/provider/java/src/org/chromium/components/autofill/AutofillManagerWrapper.java @@ -14,11 +14,11 @@ import androidx.annotation.VisibleForTesting; +import org.chromium.base.CollectionUtil; import org.chromium.base.Log; import java.lang.ref.WeakReference; import java.util.ArrayList; -import java.util.Iterator; /** * The class to call Android's AutofillManager. @@ -158,25 +158,9 @@ public void addInputUIObserver(InputUIObserver observer) { mInputUIObservers.add(new WeakReference(observer)); } - public void removeInputUIObserver(InputUIObserver observer) { - if (observer == null) return; - for (Iterator> i = mInputUIObservers.listIterator(); - i.hasNext();) { - WeakReference o = i.next(); - if (o.get() == null || o.get() == observer) i.remove(); - } - } - @VisibleForTesting public void notifyInputUIChange() { - for (Iterator> i = mInputUIObservers.listIterator(); - i.hasNext();) { - WeakReference o = i.next(); - InputUIObserver observer = o.get(); - if (observer == null) { - i.remove(); - continue; - } + for (InputUIObserver observer : CollectionUtil.strengthen(mInputUIObservers)) { observer.onInputUIShown(); } } diff --git a/components/browser_ui/util/android/java/src/org/chromium/components/browser_ui/util/BitmapCache.java b/components/browser_ui/util/android/java/src/org/chromium/components/browser_ui/util/BitmapCache.java index fb71cf29832c56..e322aa7cf6e990 100644 --- a/components/browser_ui/util/android/java/src/org/chromium/components/browser_ui/util/BitmapCache.java +++ b/components/browser_ui/util/android/java/src/org/chromium/components/browser_ui/util/BitmapCache.java @@ -12,13 +12,13 @@ import androidx.annotation.VisibleForTesting; import androidx.collection.LruCache; +import org.chromium.base.CollectionUtil; import org.chromium.base.DiscardableReferencePool; import org.chromium.base.SysUtils; import org.chromium.base.ThreadUtils; import java.lang.ref.WeakReference; import java.util.HashMap; -import java.util.Iterator; import java.util.Map; /** @@ -164,13 +164,7 @@ private static void scheduleDeduplicationCache() { * garbage collector. */ private static void compactDeduplicationCache() { - // Too many angle brackets for clang-format :-( - // clang-format off - for (Iterator>> it = - sDeduplicationCache.entrySet().iterator(); it.hasNext();) { - // clang-format on - if (it.next().getValue().get() == null) it.remove(); - } + CollectionUtil.strengthen(sDeduplicationCache.values()); } @VisibleForTesting