Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a rare LottieTask leak #1986

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Fix a rare LottieTask leak #1986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 33 additions & 68 deletions lottie/src/main/java/com/airbnb/lottie/LottieCompositionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.Callable;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.zip.ZipEntry;
import java.util.zip.ZipInputStream;

Expand Down Expand Up @@ -92,15 +93,12 @@ public static LottieTask<LottieComposition> fromUrl(final Context context, final
* might need an animation in the future.
*/
public static LottieTask<LottieComposition> fromUrl(final Context context, final String url, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
LottieResult<LottieComposition> result = L.networkFetcher(context).fetchSync(url, cacheKey);
if (cacheKey != null && result.getValue() != null) {
LottieCompositionCache.getInstance().put(cacheKey, result.getValue());
}
return result;
return cache(cacheKey, () -> {
LottieResult<LottieComposition> result = L.networkFetcher(context).fetchSync(url, cacheKey);
if (cacheKey != null && result.getValue() != null) {
LottieCompositionCache.getInstance().put(cacheKey, result.getValue());
}
return result;
});
}

Expand Down Expand Up @@ -155,12 +153,7 @@ public static LottieTask<LottieComposition> fromAsset(Context context, final Str
public static LottieTask<LottieComposition> fromAsset(Context context, final String fileName, @Nullable final String cacheKey) {
// Prevent accidentally leaking an Activity.
final Context appContext = context.getApplicationContext();
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return fromAssetSync(appContext, fileName, cacheKey);
}
});
return cache(cacheKey, () -> fromAssetSync(appContext, fileName, cacheKey));
}

/**
Expand Down Expand Up @@ -226,13 +219,10 @@ public static LottieTask<LottieComposition> fromRawRes(Context context, @RawRes
// Prevent accidentally leaking an Activity.
final WeakReference<Context> contextRef = new WeakReference<>(context);
final Context appContext = context.getApplicationContext();
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
@Nullable Context originalContext = contextRef.get();
Context context = originalContext != null ? originalContext : appContext;
return fromRawResSync(context, rawRes, cacheKey);
}
return cache(cacheKey, () -> {
@Nullable Context originalContext = contextRef.get();
Context context1 = originalContext != null ? originalContext : appContext;
return fromRawResSync(context1, rawRes, cacheKey);
});
}

Expand Down Expand Up @@ -290,12 +280,7 @@ private static boolean isNightMode(Context context) {
* @see #fromJsonInputStreamSync(InputStream, String, boolean)
*/
public static LottieTask<LottieComposition> fromJsonInputStream(final InputStream stream, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return fromJsonInputStreamSync(stream, cacheKey);
}
});
return cache(cacheKey, () -> fromJsonInputStreamSync(stream, cacheKey));
}

/**
Expand Down Expand Up @@ -324,12 +309,9 @@ private static LottieResult<LottieComposition> fromJsonInputStreamSync(InputStre
*/
@Deprecated
public static LottieTask<LottieComposition> fromJson(final JSONObject json, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
//noinspection deprecation
return fromJsonSync(json, cacheKey);
}
return cache(cacheKey, () -> {
//noinspection deprecation
return fromJsonSync(json, cacheKey);
});
}

Expand All @@ -348,12 +330,7 @@ public static LottieResult<LottieComposition> fromJsonSync(JSONObject json, @Nul
* @see #fromJsonStringSync(String, String)
*/
public static LottieTask<LottieComposition> fromJsonString(final String json, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return fromJsonStringSync(json, cacheKey);
}
});
return cache(cacheKey, () -> fromJsonStringSync(json, cacheKey));
}

/**
Expand All @@ -369,12 +346,7 @@ public static LottieResult<LottieComposition> fromJsonStringSync(String json, @N
}

public static LottieTask<LottieComposition> fromJsonReader(final JsonReader reader, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return fromJsonReaderSync(reader, cacheKey);
}
});
return cache(cacheKey, () -> fromJsonReaderSync(reader, cacheKey));
}


Expand Down Expand Up @@ -403,12 +375,7 @@ private static LottieResult<LottieComposition> fromJsonReaderSyncInternal(


public static LottieTask<LottieComposition> fromZipStream(final ZipInputStream inputStream, @Nullable final String cacheKey) {
return cache(cacheKey, new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return fromZipStreamSync(inputStream, cacheKey);
}
});
return cache(cacheKey, () -> fromZipStreamSync(inputStream, cacheKey));
}

/**
Expand Down Expand Up @@ -521,32 +488,30 @@ private static LottieTask<LottieComposition> cache(
@Nullable final String cacheKey, Callable<LottieResult<LottieComposition>> callable) {
final LottieComposition cachedComposition = cacheKey == null ? null : LottieCompositionCache.getInstance().get(cacheKey);
if (cachedComposition != null) {
return new LottieTask<>(new Callable<LottieResult<LottieComposition>>() {
@Override
public LottieResult<LottieComposition> call() {
return new LottieResult<>(cachedComposition);
}
});
return new LottieTask<>(() -> new LottieResult<>(cachedComposition));
}
if (cacheKey != null && taskCache.containsKey(cacheKey)) {
return taskCache.get(cacheKey);
}

LottieTask<LottieComposition> task = new LottieTask<>(callable);
if (cacheKey != null) {
task.addListener(new LottieListener<LottieComposition>() {
@Override
public void onResult(LottieComposition result) {
taskCache.remove(cacheKey);
}
AtomicBoolean resultAlreadyCalled = new AtomicBoolean(false);
task.addListener(result -> {
taskCache.remove(cacheKey);
resultAlreadyCalled.set(true);
});
task.addFailureListener(new LottieListener<Throwable>() {
@Override
public void onResult(Throwable result) {
taskCache.remove(cacheKey);
}
task.addFailureListener(result -> {
taskCache.remove(cacheKey);
resultAlreadyCalled.set(true);
});
taskCache.put(cacheKey, task);
// It is technically possible for the task to finish and for the listeners to get called
// before this code runs. If this happens, the task will be put in taskCache but never removed.
// This would require this thread to be sleeping at exactly this point in the code
// for long enough for the task to finish and call the listeners. Unlikely but not impossible.
if (!resultAlreadyCalled.get()) {
taskCache.put(cacheKey, task);
}
}
return task;
}
Expand Down
34 changes: 17 additions & 17 deletions lottie/src/main/java/com/airbnb/lottie/LottieTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
* <p>
* A task will produce a single result or a single failure.
*/
public class LottieTask<T> {
@SuppressWarnings("UnusedReturnValue") public class LottieTask<T> {

/**
* Set this to change the executor that LottieTasks are run on. This will be the executor that composition parsing and url
Expand Down Expand Up @@ -56,7 +56,7 @@ public LottieTask(Callable<LottieResult<T>> runnable) {
try {
setResult(runnable.call());
} catch (Throwable e) {
setResult(new LottieResult<T>(e));
setResult(new LottieResult<>(e));
}
} else {
EXECUTOR.execute(new LottieFutureTask(runnable));
Expand All @@ -77,6 +77,7 @@ private void setResult(@Nullable LottieResult<T> result) {
* @return the task for call chaining.
*/
public synchronized LottieTask<T> addListener(LottieListener<T> listener) {
LottieResult<T> result = this.result;
if (result != null && result.getValue() != null) {
listener.onResult(result.getValue());
}
Expand All @@ -87,7 +88,7 @@ public synchronized LottieTask<T> addListener(LottieListener<T> listener) {

/**
* Remove a given task listener. The task will continue to execute so you can re-add
* a listener if neccesary.
* a listener if necessary.
*
* @return the task for call chaining.
*/
Expand All @@ -103,6 +104,7 @@ public synchronized LottieTask<T> removeListener(LottieListener<T> listener) {
* @return the task for call chaining.
*/
public synchronized LottieTask<T> addFailureListener(LottieListener<Throwable> listener) {
LottieResult<T> result = this.result;
if (result != null && result.getException() != null) {
listener.onResult(result.getException());
}
Expand All @@ -113,7 +115,7 @@ public synchronized LottieTask<T> addFailureListener(LottieListener<Throwable> l

/**
* Remove a given task failure listener. The task will continue to execute so you can re-add
* a listener if neccesary.
* a listener if necessary.
*
* @return the task for call chaining.
*/
Expand All @@ -124,18 +126,16 @@ public synchronized LottieTask<T> removeFailureListener(LottieListener<Throwable

private void notifyListeners() {
// Listeners should be called on the main thread.
handler.post(new Runnable() {
@Override public void run() {
if (result == null) {
return;
}
// Local reference in case it gets set on a background thread.
LottieResult<T> result = LottieTask.this.result;
if (result.getValue() != null) {
notifySuccessListeners(result.getValue());
} else {
notifyFailureListeners(result.getException());
}
handler.post(() -> {
// Local reference in case it gets set on a background thread.
LottieResult<T> result = LottieTask.this.result;
if (result == null) {
return;
}
if (result.getValue() != null) {
notifySuccessListeners(result.getValue());
} else {
notifyFailureListeners(result.getException());
}
});
}
Expand Down Expand Up @@ -178,7 +178,7 @@ protected void done() {
try {
setResult(get());
} catch (InterruptedException | ExecutionException e) {
setResult(new LottieResult<T>(e));
setResult(new LottieResult<>(e));
}
}
}
Expand Down