Skip to content

Commit

Permalink
Fix memory-leaks in LocalCache on iOS, caused by:
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@kluever
micapolos authored and kluever committed Aug 6, 2020
1 parent f5a69c3 commit 5e519d9
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 118 deletions.
95 changes: 36 additions & 59 deletions android/guava/src/com/google/common/cache/LocalCache.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -993,7 +993,7 @@ public void setAccessTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextAccess = nullEntry();
@Weak ReferenceEntry<K, V> nextAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInAccessQueue() {
Expand All @@ -1006,7 +1006,7 @@ public void setNextInAccessQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousAccess = nullEntry();
@Weak ReferenceEntry<K, V> previousAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInAccessQueue() {
Expand Down Expand Up @@ -1039,7 +1039,7 @@ public void setWriteTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextWrite = nullEntry();
@Weak ReferenceEntry<K, V> nextWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInWriteQueue() {
Expand All @@ -1052,7 +1052,7 @@ public void setNextInWriteQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousWrite = nullEntry();
@Weak ReferenceEntry<K, V> previousWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInWriteQueue() {
Expand Down Expand Up @@ -1085,7 +1085,7 @@ public void setAccessTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextAccess = nullEntry();
@Weak ReferenceEntry<K, V> nextAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInAccessQueue() {
Expand All @@ -1098,7 +1098,7 @@ public void setNextInAccessQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousAccess = nullEntry();
@Weak ReferenceEntry<K, V> previousAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInAccessQueue() {
Expand All @@ -1125,7 +1125,7 @@ public void setWriteTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextWrite = nullEntry();
@Weak ReferenceEntry<K, V> nextWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInWriteQueue() {
Expand All @@ -1138,7 +1138,7 @@ public void setNextInWriteQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousWrite = nullEntry();
@Weak ReferenceEntry<K, V> previousWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInWriteQueue() {
Expand Down Expand Up @@ -1281,7 +1281,7 @@ public void setAccessTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextAccess = nullEntry();
@Weak ReferenceEntry<K, V> nextAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInAccessQueue() {
Expand All @@ -1294,7 +1294,7 @@ public void setNextInAccessQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousAccess = nullEntry();
@Weak ReferenceEntry<K, V> previousAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInAccessQueue() {
Expand Down Expand Up @@ -1328,7 +1328,7 @@ public void setWriteTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextWrite = nullEntry();
@Weak ReferenceEntry<K, V> nextWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInWriteQueue() {
Expand All @@ -1341,7 +1341,7 @@ public void setNextInWriteQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousWrite = nullEntry();
@Weak ReferenceEntry<K, V> previousWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInWriteQueue() {
Expand Down Expand Up @@ -1375,7 +1375,7 @@ public void setAccessTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextAccess = nullEntry();
@Weak ReferenceEntry<K, V> nextAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInAccessQueue() {
Expand All @@ -1388,7 +1388,7 @@ public void setNextInAccessQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousAccess = nullEntry();
@Weak ReferenceEntry<K, V> previousAccess = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInAccessQueue() {
Expand All @@ -1415,7 +1415,7 @@ public void setWriteTime(long time) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> nextWrite = nullEntry();
@Weak ReferenceEntry<K, V> nextWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getNextInWriteQueue() {
Expand All @@ -1428,7 +1428,7 @@ public void setNextInWriteQueue(ReferenceEntry<K, V> next) {
}

// Guarded By Segment.this
ReferenceEntry<K, V> previousWrite = nullEntry();
@Weak ReferenceEntry<K, V> previousWrite = nullEntry();

@Override
public ReferenceEntry<K, V> getPreviousInWriteQueue() {
Expand Down Expand Up @@ -3525,7 +3525,7 @@ public long getWriteTime() {
@Override
public void setWriteTime(long time) {}

ReferenceEntry<K, V> nextWrite = this;
@Weak ReferenceEntry<K, V> nextWrite = this;

@Override
public ReferenceEntry<K, V> getNextInWriteQueue() {
Expand All @@ -3537,7 +3537,7 @@ public void setNextInWriteQueue(ReferenceEntry<K, V> next) {
this.nextWrite = next;
}

ReferenceEntry<K, V> previousWrite = this;
@Weak ReferenceEntry<K, V> previousWrite = this;

@Override
public ReferenceEntry<K, V> getPreviousInWriteQueue() {
Expand Down Expand Up @@ -3664,7 +3664,7 @@ public long getAccessTime() {
@Override
public void setAccessTime(long time) {}

ReferenceEntry<K, V> nextAccess = this;
@Weak ReferenceEntry<K, V> nextAccess = this;

@Override
public ReferenceEntry<K, V> getNextInAccessQueue() {
Expand All @@ -3676,7 +3676,7 @@ public void setNextInAccessQueue(ReferenceEntry<K, V> next) {
this.nextAccess = next;
}

ReferenceEntry<K, V> previousAccess = this;
@Weak ReferenceEntry<K, V> previousAccess = this;

@Override
public ReferenceEntry<K, V> getPreviousInAccessQueue() {
Expand Down Expand Up @@ -4146,32 +4146,32 @@ void invalidateAll(Iterable<?> keys) {
}
}

@NullableDecl Set<K> keySet;
@RetainedWith @NullableDecl Set<K> keySet;

@Override
public Set<K> keySet() {
// does not impact recency ordering
Set<K> ks = keySet;
return (ks != null) ? ks : (keySet = new KeySet(this));
return (ks != null) ? ks : (keySet = new KeySet());
}

@NullableDecl Collection<V> values;
@RetainedWith @NullableDecl Collection<V> values;

@Override
public Collection<V> values() {
// does not impact recency ordering
Collection<V> vs = values;
return (vs != null) ? vs : (values = new Values(this));
return (vs != null) ? vs : (values = new Values());
}

@NullableDecl Set<Entry<K, V>> entrySet;
@RetainedWith @NullableDecl Set<Entry<K, V>> entrySet;

@Override
@GwtIncompatible // Not supported.
public Set<Entry<K, V>> entrySet() {
// does not impact recency ordering
Set<Entry<K, V>> es = entrySet;
return (es != null) ? es : (entrySet = new EntrySet(this));
return (es != null) ? es : (entrySet = new EntrySet());
}

// Iterator Support
Expand Down Expand Up @@ -4362,25 +4362,19 @@ public Entry<K, V> next() {
}

abstract class AbstractCacheSet<T> extends AbstractSet<T> {
@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.
Expand All @@ -4404,50 +4398,38 @@ private static <E> ArrayList<E> toArrayList(Collection<E> c) {
return result;
}

@WeakOuter
final class KeySet extends AbstractCacheSet<K> {

KeySet(ConcurrentMap<?, ?> map) {
super(map);
}

@Override
public Iterator<K> iterator() {
return new KeyIterator();
}

@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<V> {
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
Expand All @@ -4457,7 +4439,7 @@ public Iterator<V> 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.
Expand All @@ -4474,13 +4456,8 @@ public <E> E[] toArray(E[] a) {
}
}

@WeakOuter
final class EntrySet extends AbstractCacheSet<Entry<K, V>> {

EntrySet(ConcurrentMap<?, ?> map) {
super(map);
}

@Override
public Iterator<Entry<K, V>> iterator() {
return new EntryIterator();
Expand Down
Loading

0 comments on commit 5e519d9

Please sign in to comment.