From 5e519d91d038636fa5f786908aa856ba5470846d Mon Sep 17 00:00:00 2001 From: micapolos Date: Wed, 5 Aug 2020 06:32:49 -0700 Subject: [PATCH] Fix memory-leaks in LocalCache on iOS, caused by: 1. Retain-cycle between outer LocalCache instance and inner Values, KeySet and EntrySet instances. The use of @Weak and @WeakOuter is incorrect there, since inner instances can outlive outer LocalCache instance. The correct solution is to use @RetainedWith annotation to inner-classes. 2. Retain-cycle between ReferenceEntry objects which internally form a doubly-linked list. Fixed by adding @Weak annotation to "next" and "previous" links. This is correct, since ReferenceEntry instances are already retained by Segments. The unit test for leak detection is added inside Xplat, because the required testing infrastructure is not present inside "google_common" (the IosAsserts class). Eventually, everything should be moved to "google_common". RELNOTES=Fix memory-leak in LocalCache on iOS ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=325008493 --- .../com/google/common/cache/LocalCache.java | 95 +++++++------------ .../com/google/common/cache/LocalCache.java | 95 +++++++------------ 2 files changed, 72 insertions(+), 118 deletions(-) diff --git a/android/guava/src/com/google/common/cache/LocalCache.java b/android/guava/src/com/google/common/cache/LocalCache.java index 7fc67e7bf068..86f9dbe6744e 100644 --- a/android/guava/src/com/google/common/cache/LocalCache.java +++ b/android/guava/src/com/google/common/cache/LocalCache.java @@ -50,8 +50,8 @@ import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.common.util.concurrent.Uninterruptibles; import com.google.errorprone.annotations.concurrent.GuardedBy; +import com.google.j2objc.annotations.RetainedWith; import com.google.j2objc.annotations.Weak; -import com.google.j2objc.annotations.WeakOuter; import java.io.IOException; import java.io.ObjectInputStream; import java.io.Serializable; @@ -993,7 +993,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1006,7 +1006,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1039,7 +1039,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1052,7 +1052,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1085,7 +1085,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1098,7 +1098,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1125,7 +1125,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1138,7 +1138,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1281,7 +1281,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1294,7 +1294,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1328,7 +1328,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1341,7 +1341,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1375,7 +1375,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1388,7 +1388,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1415,7 +1415,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1428,7 +1428,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -3525,7 +3525,7 @@ public long getWriteTime() { @Override public void setWriteTime(long time) {} - ReferenceEntry nextWrite = this; + @Weak ReferenceEntry nextWrite = this; @Override public ReferenceEntry getNextInWriteQueue() { @@ -3537,7 +3537,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { this.nextWrite = next; } - ReferenceEntry previousWrite = this; + @Weak ReferenceEntry previousWrite = this; @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -3664,7 +3664,7 @@ public long getAccessTime() { @Override public void setAccessTime(long time) {} - ReferenceEntry nextAccess = this; + @Weak ReferenceEntry nextAccess = this; @Override public ReferenceEntry getNextInAccessQueue() { @@ -3676,7 +3676,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { this.nextAccess = next; } - ReferenceEntry previousAccess = this; + @Weak ReferenceEntry previousAccess = this; @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -4146,32 +4146,32 @@ void invalidateAll(Iterable keys) { } } - @NullableDecl Set keySet; + @RetainedWith @NullableDecl Set keySet; @Override public Set keySet() { // does not impact recency ordering Set ks = keySet; - return (ks != null) ? ks : (keySet = new KeySet(this)); + return (ks != null) ? ks : (keySet = new KeySet()); } - @NullableDecl Collection values; + @RetainedWith @NullableDecl Collection values; @Override public Collection values() { // does not impact recency ordering Collection vs = values; - return (vs != null) ? vs : (values = new Values(this)); + return (vs != null) ? vs : (values = new Values()); } - @NullableDecl Set> entrySet; + @RetainedWith @NullableDecl Set> entrySet; @Override @GwtIncompatible // Not supported. public Set> entrySet() { // does not impact recency ordering Set> es = entrySet; - return (es != null) ? es : (entrySet = new EntrySet(this)); + return (es != null) ? es : (entrySet = new EntrySet()); } // Iterator Support @@ -4362,25 +4362,19 @@ public Entry next() { } abstract class AbstractCacheSet extends AbstractSet { - @Weak final ConcurrentMap map; - - AbstractCacheSet(ConcurrentMap map) { - this.map = map; - } - @Override public int size() { - return map.size(); + return LocalCache.this.size(); } @Override public boolean isEmpty() { - return map.isEmpty(); + return LocalCache.this.isEmpty(); } @Override public void clear() { - map.clear(); + LocalCache.this.clear(); } // super.toArray() may misbehave if size() is inaccurate, at least on old versions of Android. @@ -4404,13 +4398,8 @@ private static ArrayList toArrayList(Collection c) { return result; } - @WeakOuter final class KeySet extends AbstractCacheSet { - KeySet(ConcurrentMap map) { - super(map); - } - @Override public Iterator iterator() { return new KeyIterator(); @@ -4418,36 +4407,29 @@ public Iterator iterator() { @Override public boolean contains(Object o) { - return map.containsKey(o); + return LocalCache.this.containsKey(o); } @Override public boolean remove(Object o) { - return map.remove(o) != null; + return LocalCache.this.remove(o) != null; } } - @WeakOuter final class Values extends AbstractCollection { - private final ConcurrentMap map; - - Values(ConcurrentMap map) { - this.map = map; - } - @Override public int size() { - return map.size(); + return LocalCache.this.size(); } @Override public boolean isEmpty() { - return map.isEmpty(); + return LocalCache.this.isEmpty(); } @Override public void clear() { - map.clear(); + LocalCache.this.clear(); } @Override @@ -4457,7 +4439,7 @@ public Iterator iterator() { @Override public boolean contains(Object o) { - return map.containsValue(o); + return LocalCache.this.containsValue(o); } // super.toArray() may misbehave if size() is inaccurate, at least on old versions of Android. @@ -4474,13 +4456,8 @@ public E[] toArray(E[] a) { } } - @WeakOuter final class EntrySet extends AbstractCacheSet> { - EntrySet(ConcurrentMap map) { - super(map); - } - @Override public Iterator> iterator() { return new EntryIterator(); diff --git a/guava/src/com/google/common/cache/LocalCache.java b/guava/src/com/google/common/cache/LocalCache.java index cd75ef7a9587..a485ad5970a6 100644 --- a/guava/src/com/google/common/cache/LocalCache.java +++ b/guava/src/com/google/common/cache/LocalCache.java @@ -50,8 +50,8 @@ import com.google.common.util.concurrent.UncheckedExecutionException; import com.google.common.util.concurrent.Uninterruptibles; import com.google.errorprone.annotations.concurrent.GuardedBy; +import com.google.j2objc.annotations.RetainedWith; import com.google.j2objc.annotations.Weak; -import com.google.j2objc.annotations.WeakOuter; import java.io.IOException; import java.io.ObjectInputStream; import java.io.Serializable; @@ -997,7 +997,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1010,7 +1010,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1043,7 +1043,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1056,7 +1056,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1089,7 +1089,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1102,7 +1102,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1129,7 +1129,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1142,7 +1142,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1284,7 +1284,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1297,7 +1297,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1330,7 +1330,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1343,7 +1343,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -1377,7 +1377,7 @@ public void setAccessTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextAccess = nullEntry(); + @Weak ReferenceEntry nextAccess = nullEntry(); @Override public ReferenceEntry getNextInAccessQueue() { @@ -1390,7 +1390,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousAccess = nullEntry(); + @Weak ReferenceEntry previousAccess = nullEntry(); @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -1417,7 +1417,7 @@ public void setWriteTime(long time) { } // Guarded By Segment.this - ReferenceEntry nextWrite = nullEntry(); + @Weak ReferenceEntry nextWrite = nullEntry(); @Override public ReferenceEntry getNextInWriteQueue() { @@ -1430,7 +1430,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { } // Guarded By Segment.this - ReferenceEntry previousWrite = nullEntry(); + @Weak ReferenceEntry previousWrite = nullEntry(); @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -3628,7 +3628,7 @@ public long getWriteTime() { @Override public void setWriteTime(long time) {} - ReferenceEntry nextWrite = this; + @Weak ReferenceEntry nextWrite = this; @Override public ReferenceEntry getNextInWriteQueue() { @@ -3640,7 +3640,7 @@ public void setNextInWriteQueue(ReferenceEntry next) { this.nextWrite = next; } - ReferenceEntry previousWrite = this; + @Weak ReferenceEntry previousWrite = this; @Override public ReferenceEntry getPreviousInWriteQueue() { @@ -3767,7 +3767,7 @@ public long getAccessTime() { @Override public void setAccessTime(long time) {} - ReferenceEntry nextAccess = this; + @Weak ReferenceEntry nextAccess = this; @Override public ReferenceEntry getNextInAccessQueue() { @@ -3779,7 +3779,7 @@ public void setNextInAccessQueue(ReferenceEntry next) { this.nextAccess = next; } - ReferenceEntry previousAccess = this; + @Weak ReferenceEntry previousAccess = this; @Override public ReferenceEntry getPreviousInAccessQueue() { @@ -4278,32 +4278,32 @@ void invalidateAll(Iterable keys) { } } - @Nullable Set keySet; + @RetainedWith @Nullable Set keySet; @Override public Set keySet() { // does not impact recency ordering Set ks = keySet; - return (ks != null) ? ks : (keySet = new KeySet(this)); + return (ks != null) ? ks : (keySet = new KeySet()); } - @Nullable Collection values; + @RetainedWith @Nullable Collection values; @Override public Collection values() { // does not impact recency ordering Collection vs = values; - return (vs != null) ? vs : (values = new Values(this)); + return (vs != null) ? vs : (values = new Values()); } - @Nullable Set> entrySet; + @RetainedWith @Nullable Set> entrySet; @Override @GwtIncompatible // Not supported. public Set> entrySet() { // does not impact recency ordering Set> es = entrySet; - return (es != null) ? es : (entrySet = new EntrySet(this)); + return (es != null) ? es : (entrySet = new EntrySet()); } // Iterator Support @@ -4494,25 +4494,19 @@ public Entry next() { } abstract class AbstractCacheSet extends AbstractSet { - @Weak final ConcurrentMap map; - - AbstractCacheSet(ConcurrentMap map) { - this.map = map; - } - @Override public int size() { - return map.size(); + return LocalCache.this.size(); } @Override public boolean isEmpty() { - return map.isEmpty(); + return LocalCache.this.isEmpty(); } @Override public void clear() { - map.clear(); + LocalCache.this.clear(); } // super.toArray() may misbehave if size() is inaccurate, at least on old versions of Android. @@ -4553,13 +4547,8 @@ boolean removeIf(BiPredicate filter) { return changed; } - @WeakOuter final class KeySet extends AbstractCacheSet { - KeySet(ConcurrentMap map) { - super(map); - } - @Override public Iterator iterator() { return new KeyIterator(); @@ -4567,36 +4556,29 @@ public Iterator iterator() { @Override public boolean contains(Object o) { - return map.containsKey(o); + return LocalCache.this.containsKey(o); } @Override public boolean remove(Object o) { - return map.remove(o) != null; + return LocalCache.this.remove(o) != null; } } - @WeakOuter final class Values extends AbstractCollection { - private final ConcurrentMap map; - - Values(ConcurrentMap map) { - this.map = map; - } - @Override public int size() { - return map.size(); + return LocalCache.this.size(); } @Override public boolean isEmpty() { - return map.isEmpty(); + return LocalCache.this.isEmpty(); } @Override public void clear() { - map.clear(); + LocalCache.this.clear(); } @Override @@ -4612,7 +4594,7 @@ public boolean removeIf(Predicate filter) { @Override public boolean contains(Object o) { - return map.containsValue(o); + return LocalCache.this.containsValue(o); } // super.toArray() may misbehave if size() is inaccurate, at least on old versions of Android. @@ -4629,13 +4611,8 @@ public E[] toArray(E[] a) { } } - @WeakOuter final class EntrySet extends AbstractCacheSet> { - EntrySet(ConcurrentMap map) { - super(map); - } - @Override public Iterator> iterator() { return new EntryIterator();