From 156e48541bf7b7f88278662eef23eec4fe1b24fe Mon Sep 17 00:00:00 2001 From: Gabriel Peal Date: Tue, 2 Nov 2021 22:20:51 -0700 Subject: [PATCH] Fix a few potential NPE race conditions (#1917) Although me and the reporters are unable to reproduce these NPEs, it's not impossible that they would occur if either: LottieDrawable's composition is set multiple times from multiple threads Dynamic properties are set from multiple threads This adds some thread safety to variable access and should resolve these. Fixes #1794 --- .../com/airbnb/lottie/LottieDrawable.java | 13 ++-- .../keyframe/TransformKeyframeAnimation.java | 68 ++++++++++--------- 2 files changed, 45 insertions(+), 36 deletions(-) diff --git a/lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java b/lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java index b235b390e7..f51c37780d 100644 --- a/lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java +++ b/lottie/src/main/java/com/airbnb/lottie/LottieDrawable.java @@ -1194,7 +1194,7 @@ public void unscheduleDrawable(@NonNull Drawable who, @NonNull Runnable what) { * If the composition is larger than the canvas, we have to use a different method to scale it up. * See the comments in {@link #draw(Canvas)} for more info. */ - private float getMaxScale(@NonNull Canvas canvas) { + private float getMaxScale(@NonNull Canvas canvas, LottieComposition composition) { float maxScaleX = canvas.getWidth() / (float) composition.getBounds().width(); float maxScaleY = canvas.getHeight() / (float) composition.getBounds().height(); return Math.min(maxScaleX, maxScaleY); @@ -1202,6 +1202,7 @@ private float getMaxScale(@NonNull Canvas canvas) { @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) public void draw(Canvas canvas, Matrix matrix) { + CompositionLayer compositionLayer = this.compositionLayer; if (compositionLayer == null) { return; } @@ -1209,7 +1210,9 @@ public void draw(Canvas canvas, Matrix matrix) { } private void drawWithNewAspectRatio(Canvas canvas) { - if (compositionLayer == null) { + CompositionLayer compositionLayer = this.compositionLayer; + LottieComposition composition = this.composition; + if (compositionLayer == null || composition == null) { return; } @@ -1252,13 +1255,15 @@ private void drawWithNewAspectRatio(Canvas canvas) { } private void drawWithOriginalAspectRatio(Canvas canvas) { - if (compositionLayer == null) { + CompositionLayer compositionLayer = this.compositionLayer; + LottieComposition composition = this.composition; + if (compositionLayer == null || composition == null) { return; } float scale = this.scale; float extraScale = 1f; - float maxScale = getMaxScale(canvas); + float maxScale = getMaxScale(canvas, composition); if (scale > maxScale) { scale = maxScale; extraScale = this.scale / scale; diff --git a/lottie/src/main/java/com/airbnb/lottie/animation/keyframe/TransformKeyframeAnimation.java b/lottie/src/main/java/com/airbnb/lottie/animation/keyframe/TransformKeyframeAnimation.java index 4483fbb74d..5453d7b9d4 100644 --- a/lottie/src/main/java/com/airbnb/lottie/animation/keyframe/TransformKeyframeAnimation.java +++ b/lottie/src/main/java/com/airbnb/lottie/animation/keyframe/TransformKeyframeAnimation.java @@ -15,7 +15,6 @@ import android.graphics.Matrix; import android.graphics.PointF; -import androidx.annotation.NonNull; import androidx.annotation.Nullable; import com.airbnb.lottie.model.animatable.AnimatableTransform; @@ -33,11 +32,11 @@ public class TransformKeyframeAnimation { private final Matrix skewMatrix3; private final float[] skewValues; - @NonNull private BaseKeyframeAnimation anchorPoint; - @NonNull private BaseKeyframeAnimation position; - @NonNull private BaseKeyframeAnimation scale; - @NonNull private BaseKeyframeAnimation rotation; - @NonNull private BaseKeyframeAnimation opacity; + @Nullable private BaseKeyframeAnimation anchorPoint; + @Nullable private BaseKeyframeAnimation position; + @Nullable private BaseKeyframeAnimation scale; + @Nullable private BaseKeyframeAnimation rotation; + @Nullable private BaseKeyframeAnimation opacity; @Nullable private FloatKeyframeAnimation skew; @Nullable private FloatKeyframeAnimation skewAngle; @@ -167,25 +166,28 @@ public void setProgress(float progress) { public Matrix getMatrix() { matrix.reset(); + BaseKeyframeAnimation position = this.position; if (position != null) { - PointF position = this.position.getValue(); - if (position.x != 0 || position.y != 0) { - matrix.preTranslate(position.x, position.y); + PointF positionValue = position.getValue(); + if (positionValue.x != 0 || positionValue.y != 0) { + matrix.preTranslate(positionValue.x, positionValue.y); } } + BaseKeyframeAnimation rotation = this.rotation; if (rotation != null) { - float rotation; - if (this.rotation instanceof ValueCallbackKeyframeAnimation) { - rotation = this.rotation.getValue(); + float rotationValue; + if (rotation instanceof ValueCallbackKeyframeAnimation) { + rotationValue = rotation.getValue(); } else { - rotation = ((FloatKeyframeAnimation) this.rotation).getFloatValue(); + rotationValue = ((FloatKeyframeAnimation) rotation).getFloatValue(); } - if (rotation != 0f) { - matrix.preRotate(rotation); + if (rotationValue != 0f) { + matrix.preRotate(rotationValue); } } + FloatKeyframeAnimation skew = this.skew; if (skew != null) { float mCos = skewAngle == null ? 0f : (float) Math.cos(Math.toRadians(-skewAngle.getFloatValue() + 90)); float mSin = skewAngle == null ? 1f : (float) Math.sin(Math.toRadians(-skewAngle.getFloatValue() + 90)); @@ -216,17 +218,19 @@ public Matrix getMatrix() { matrix.preConcat(skewMatrix3); } + BaseKeyframeAnimation scale = this.scale; if (scale != null) { - ScaleXY scaleTransform = this.scale.getValue(); + ScaleXY scaleTransform = scale.getValue(); if (scaleTransform.getScaleX() != 1f || scaleTransform.getScaleY() != 1f) { matrix.preScale(scaleTransform.getScaleX(), scaleTransform.getScaleY()); } } + BaseKeyframeAnimation anchorPoint = this.anchorPoint; if (anchorPoint != null) { - PointF anchorPoint = this.anchorPoint.getValue(); - if (anchorPoint.x != 0 || anchorPoint.y != 0) { - matrix.preTranslate(-anchorPoint.x, -anchorPoint.y); + PointF anchorPointValue = anchorPoint.getValue(); + if (anchorPointValue.x != 0 || anchorPointValue.y != 0) { + matrix.preTranslate(-anchorPointValue.x, -anchorPointValue.y); } } @@ -271,13 +275,13 @@ public Matrix getMatrixForRepeater(float amount) { public boolean applyValueCallback(T property, @Nullable LottieValueCallback callback) { if (property == TRANSFORM_ANCHOR_POINT) { if (anchorPoint == null) { - anchorPoint = new ValueCallbackKeyframeAnimation(callback, new PointF()); + anchorPoint = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, new PointF()); } else { anchorPoint.setValueCallback((LottieValueCallback) callback); } } else if (property == TRANSFORM_POSITION) { if (position == null) { - position = new ValueCallbackKeyframeAnimation(callback, new PointF()); + position = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, new PointF()); } else { position.setValueCallback((LottieValueCallback) callback); } @@ -287,42 +291,42 @@ public boolean applyValueCallback(T property, @Nullable LottieValueCallback< ((SplitDimensionPathKeyframeAnimation) position).setYValueCallback((LottieValueCallback) callback); } else if (property == TRANSFORM_SCALE) { if (scale == null) { - scale = new ValueCallbackKeyframeAnimation(callback, new ScaleXY()); + scale = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, new ScaleXY()); } else { scale.setValueCallback((LottieValueCallback) callback); } } else if (property == TRANSFORM_ROTATION) { if (rotation == null) { - rotation = new ValueCallbackKeyframeAnimation(callback, 0f); + rotation = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, 0f); } else { rotation.setValueCallback((LottieValueCallback) callback); } } else if (property == TRANSFORM_OPACITY) { if (opacity == null) { - opacity = new ValueCallbackKeyframeAnimation(callback, 100); + opacity = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, 100); } else { opacity.setValueCallback((LottieValueCallback) callback); } - } else if (property == TRANSFORM_START_OPACITY && startOpacity != null) { + } else if (property == TRANSFORM_START_OPACITY) { if (startOpacity == null) { - startOpacity = new ValueCallbackKeyframeAnimation(callback, 100); + startOpacity = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, 100f); } else { startOpacity.setValueCallback((LottieValueCallback) callback); } - } else if (property == TRANSFORM_END_OPACITY && endOpacity != null) { + } else if (property == TRANSFORM_END_OPACITY) { if (endOpacity == null) { - endOpacity = new ValueCallbackKeyframeAnimation(callback, 100); + endOpacity = new ValueCallbackKeyframeAnimation<>((LottieValueCallback) callback, 100f); } else { endOpacity.setValueCallback((LottieValueCallback) callback); } - } else if (property == TRANSFORM_SKEW && skew != null) { + } else if (property == TRANSFORM_SKEW) { if (skew == null) { - skew = new FloatKeyframeAnimation(Collections.singletonList(new Keyframe(0f))); + skew = new FloatKeyframeAnimation(Collections.singletonList(new Keyframe<>(0f))); } skew.setValueCallback((LottieValueCallback) callback); - } else if (property == TRANSFORM_SKEW_ANGLE && skewAngle != null) { + } else if (property == TRANSFORM_SKEW_ANGLE) { if (skewAngle == null) { - skewAngle = new FloatKeyframeAnimation(Collections.singletonList(new Keyframe(0f))); + skewAngle = new FloatKeyframeAnimation(Collections.singletonList(new Keyframe<>(0f))); } skewAngle.setValueCallback((LottieValueCallback) callback); } else {