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 <agrieve@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819407}

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",

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<Cancelable> cancelableWeakReference : mCancelables) {
-                Cancelable cancelable = cancelableWeakReference.get();
-                if (cancelable != null) cancelable.cancel();
+            for (Cancelable cancelable : CollectionUtil.strengthen(mCancelables)) {
+                cancelable.cancel();
             }
             mCancelables = null;
         }

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 <K, V> void forEach(
             worker.onResult((Map.Entry<K, V>) 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<WeakReference<T>> or a Map with weak values.
+     * For Set<WeakReference<T>>, use Collections.newSetFromMap(new WeakHashMap()) instead.
+     *
+     * @param weakRefs Collection to iterate.
+     * @return List of strong references.
+     */
+    public static <T> List<T> strengthen(Collection<WeakReference<T>> weakRefs) {
+        ArrayList<T> ret = new ArrayList<>(weakRefs.size());
+        Iterator<WeakReference<T>> it = weakRefs.iterator();
+        while (it.hasNext()) {
+            WeakReference<T> weakRef = it.next();
+            T strongRef = weakRef.get();
+            if (strongRef == null) {
+                it.remove();
+            } else {
+                ret.add(strongRef);
+            }
+        }
+        return ret;
+    }
 }

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<T extends UnownedUserData> {
     @NonNull
     private final Class<T> mClazz;
-    private final Set<WeakReference<UnownedUserDataHost>> mHostAttachments = new HashSet<>();
+    // A Set that uses WeakReference<UnownedUserDataHost> internally.
+    private final Set<UnownedUserDataHost> 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<UnownedUserDataHost> 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<UnownedUserDataHost> getStrongRefs() {
-        ArrayList<UnownedUserDataHost> ret = new ArrayList<>();
-        Set<WeakReference<UnownedUserDataHost>> hosts = new HashSet<>(mHostAttachments);
-        for (WeakReference<UnownedUserDataHost> 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;
     }
 }

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<WeakReference<Integer>> 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));
+    }
+}

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<String> getAllSuspendedWebsites() {
     }
 
     private void notifyObserversOfSuspensions(List<String> fqdns, boolean suspended) {
-        for (WeakReference<PageViewObserver> 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);
             }
         }
     }

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<InputUIObserver>(observer));
     }
 
-    public void removeInputUIObserver(InputUIObserver observer) {
-        if (observer == null) return;
-        for (Iterator<WeakReference<InputUIObserver>> i = mInputUIObservers.listIterator();
-                i.hasNext();) {
-            WeakReference<InputUIObserver> o = i.next();
-            if (o.get() == null || o.get() == observer) i.remove();
-        }
-    }
-
     @VisibleForTesting
     public void notifyInputUIChange() {
-        for (Iterator<WeakReference<InputUIObserver>> i = mInputUIObservers.listIterator();
-                i.hasNext();) {
-            WeakReference<InputUIObserver> o = i.next();
-            InputUIObserver observer = o.get();
-            if (observer == null) {
-                i.remove();
-                continue;
-            }
+        for (InputUIObserver observer : CollectionUtil.strengthen(mInputUIObservers)) {
             observer.onInputUIShown();
         }
     }

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<Map.Entry<String, WeakReference<Bitmap>>> it =
-                sDeduplicationCache.entrySet().iterator(); it.hasNext();) {
-            // clang-format on
-            if (it.next().getValue().get() == null) it.remove();
-        }
+        CollectionUtil.strengthen(sDeduplicationCache.values());
     }
 
     @VisibleForTesting