From eddaf503fe7ab1997cdde53ec55486d1f6087eed Mon Sep 17 00:00:00 2001 From: ttok Date: Mon, 13 Feb 2023 12:17:58 +0300 Subject: [PATCH] fix NPE in DecodeBase64ImageTask fix NPE in DecodeBase64ImageTask --- .../yandex/div/core/DecodeBase64ImageTask.kt | 2 +- .../div/core/view2/DivPlaceholderLoader.kt | 19 +++++++-- .../div/core/view2/divs/DivGifImageBinder.kt | 15 +++++-- .../div/core/view2/divs/DivImageBinder.kt | 41 ++++++++++++++----- .../div/core/view2/divs/DivImageBinderTest.kt | 22 ++++++---- 5 files changed, 72 insertions(+), 27 deletions(-) diff --git a/client/android/div/src/main/java/com/yandex/div/core/DecodeBase64ImageTask.kt b/client/android/div/src/main/java/com/yandex/div/core/DecodeBase64ImageTask.kt index 97922464c..66fde6cb9 100644 --- a/client/android/div/src/main/java/com/yandex/div/core/DecodeBase64ImageTask.kt +++ b/client/android/div/src/main/java/com/yandex/div/core/DecodeBase64ImageTask.kt @@ -10,7 +10,7 @@ import com.yandex.div.internal.util.UiThreadHandler internal class DecodeBase64ImageTask( private var base64string: String, private val synchronous: Boolean, - private val onDecoded: (Bitmap) -> Unit + private val onDecoded: (Bitmap?) -> Unit ) : Runnable { @WorkerThread diff --git a/client/android/div/src/main/java/com/yandex/div/core/view2/DivPlaceholderLoader.kt b/client/android/div/src/main/java/com/yandex/div/core/view2/DivPlaceholderLoader.kt index 3cb60c19c..baff35614 100644 --- a/client/android/div/src/main/java/com/yandex/div/core/view2/DivPlaceholderLoader.kt +++ b/client/android/div/src/main/java/com/yandex/div/core/view2/DivPlaceholderLoader.kt @@ -8,10 +8,13 @@ import com.yandex.div.core.Div2ImageStubProvider import com.yandex.div.core.annotations.Mockable import com.yandex.div.core.dagger.DivScope import com.yandex.div.core.view2.divs.widgets.LoadableImage +import com.yandex.div.core.view2.errors.ErrorCollector import java.util.concurrent.ExecutorService import java.util.concurrent.Future import javax.inject.Inject +private const val PREVIEW_IS_NOT_BASE_64_IMAGE = "Preview doesn't contain base64 image" + @DivScope @Mockable internal class DivPlaceholderLoader @Inject constructor( @@ -22,14 +25,22 @@ internal class DivPlaceholderLoader @Inject constructor( @MainThread fun applyPlaceholder( imageView: LoadableImage, + errorCollector: ErrorCollector, currentPreview: String?, currentPlaceholderColor: Int, synchronous: Boolean, onSetPlaceholder: (Drawable?) -> Unit, - onSetPreview: (Bitmap) -> Unit + onSetPreview: (Bitmap?) -> Unit ) { currentPreview?.let { - enqueueDecoding(it, imageView, synchronous, onSetPreview) + enqueueDecoding(it, imageView, synchronous) { bitmap -> + if (bitmap == null) { + errorCollector.logWarning(Throwable(PREVIEW_IS_NOT_BASE_64_IMAGE)) + onSetPlaceholder(imageStubProvider.getImageStubDrawable(currentPlaceholderColor)) + } else { + onSetPreview(bitmap) + } + } } ?: onSetPlaceholder(imageStubProvider.getImageStubDrawable(currentPlaceholderColor)) } @@ -37,7 +48,7 @@ internal class DivPlaceholderLoader @Inject constructor( preview: String, loadableImage: LoadableImage, synchronous: Boolean, - onDecoded: (Bitmap) -> Unit + onDecoded: (Bitmap?) -> Unit ) { loadableImage.getLoadingTask()?.cancel(true) @@ -51,7 +62,7 @@ internal class DivPlaceholderLoader @Inject constructor( private fun String.decodeBase64ToBitmap( synchronous: Boolean, - onDecoded: (Bitmap) -> Unit + onDecoded: (Bitmap?) -> Unit ): Future<*>? { val decodeTask = DecodeBase64ImageTask(this, synchronous, onDecoded) diff --git a/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivGifImageBinder.kt b/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivGifImageBinder.kt index 90090c9a8..f65198cff 100644 --- a/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivGifImageBinder.kt +++ b/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivGifImageBinder.kt @@ -15,6 +15,8 @@ import com.yandex.div.core.view2.Div2View import com.yandex.div.core.view2.DivPlaceholderLoader import com.yandex.div.core.view2.DivViewBinder import com.yandex.div.core.view2.divs.widgets.DivGifImageView +import com.yandex.div.core.view2.errors.ErrorCollector +import com.yandex.div.core.view2.errors.ErrorCollectors import com.yandex.div.internal.KLog import com.yandex.div.json.expressions.Expression import com.yandex.div.internal.widget.AspectImageView @@ -35,13 +37,16 @@ private const val GIF_SUFFIX = ".gif" internal class DivGifImageBinder @Inject constructor( private val baseBinder: DivBaseBinder, private val imageLoader: DivImageLoader, - private val placeholderLoader: DivPlaceholderLoader + private val placeholderLoader: DivPlaceholderLoader, + private val errorCollectors: ErrorCollectors, ) : DivViewBinder { override fun bindView(view: DivGifImageView, div: DivGifImage, divView: Div2View) { val oldDiv = view.div if (div == oldDiv) return + val errorCollector = errorCollectors.getOrCreate(divView.dataTag, divView.divData) + val expressionResolver = divView.expressionResolver view.closeAllSubscription() @@ -57,7 +62,7 @@ internal class DivGifImageBinder @Inject constructor( ) view.observeContentAlignment(expressionResolver, div.contentAlignmentHorizontal, div.contentAlignmentVertical) view.addSubscription( - div.gifUrl.observeAndGet(expressionResolver) { view.applyGifImage(divView, expressionResolver, div) } + div.gifUrl.observeAndGet(expressionResolver) { view.applyGifImage(divView, expressionResolver, div, errorCollector) } ) } @@ -94,7 +99,10 @@ internal class DivGifImageBinder @Inject constructor( gravity = evaluateGravity(horizontalAlignment.evaluate(resolver), verticalAlignment.evaluate(resolver)) } - private fun DivGifImageView.applyGifImage(divView: Div2View, resolver: ExpressionResolver, div: DivGifImage) { + private fun DivGifImageView.applyGifImage(divView: Div2View, + resolver: ExpressionResolver, + div: DivGifImage, + errorCollector: ErrorCollector) { val newGifUrl = div.gifUrl.evaluate(resolver) if (isImageLoaded && newGifUrl == gifUrl) { return @@ -104,6 +112,7 @@ internal class DivGifImageBinder @Inject constructor( // if bitmap was already loaded for the same imageUrl, we don't load placeholders. placeholderLoader.applyPlaceholder( this, + errorCollector, div.preview?.evaluate(resolver), div.placeholderColor.evaluate(resolver), synchronous = false, diff --git a/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivImageBinder.kt b/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivImageBinder.kt index 3d4004664..ecc1d39e0 100644 --- a/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivImageBinder.kt +++ b/client/android/div/src/main/java/com/yandex/div/core/view2/divs/DivImageBinder.kt @@ -14,6 +14,8 @@ import com.yandex.div.core.view2.DivPlaceholderLoader import com.yandex.div.core.view2.DivViewBinder import com.yandex.div.core.view2.divs.widgets.DivImageView import com.yandex.div.core.view2.divs.widgets.applyFilters +import com.yandex.div.core.view2.errors.ErrorCollector +import com.yandex.div.core.view2.errors.ErrorCollectors import com.yandex.div.internal.widget.AspectImageView import com.yandex.div.json.expressions.Expression import com.yandex.div.json.expressions.ExpressionResolver @@ -29,13 +31,16 @@ import javax.inject.Inject internal class DivImageBinder @Inject constructor( private val baseBinder: DivBaseBinder, private val imageLoader: DivImageLoader, - private val placeholderLoader: DivPlaceholderLoader + private val placeholderLoader: DivPlaceholderLoader, + private val errorCollectors: ErrorCollectors, ) : DivViewBinder { override fun bindView(view: DivImageView, div: DivImage, divView: Div2View) { val oldDiv = view.div if (div == oldDiv) return + val errorCollector = errorCollectors.getOrCreate(divView.dataTag, divView.divData) + val expressionResolver = divView.expressionResolver val subscriber = view.expressionSubscriber view.closeAllSubscription() @@ -51,9 +56,9 @@ internal class DivImageBinder @Inject constructor( div.scale.observeAndGet(expressionResolver) { scale -> view.imageScale = scale.toImageScale() } ) view.observeContentAlignment(expressionResolver, div.contentAlignmentHorizontal, div.contentAlignmentVertical) - view.observePreview(divView, expressionResolver, div) + view.observePreview(divView, expressionResolver, errorCollector, div) view.addSubscription( - div.imageUrl.observeAndGet(expressionResolver) { view.applyImage(divView, expressionResolver, div) } + div.imageUrl.observeAndGet(expressionResolver) { view.applyImage(divView, expressionResolver, errorCollector, div) } ) view.observeTint(expressionResolver, div.tintColor, div.tintMode) view.observeFilters(div.filters, divView, subscriber, expressionResolver) @@ -115,8 +120,13 @@ internal class DivImageBinder @Inject constructor( divView: Div2View, resolver: ExpressionResolver, ) { - currentBitmapWithoutFilters?.applyFilters(this, filters, divView.div2Component, resolver) { - setImageBitmap(it) + val bitmap = currentBitmapWithoutFilters + if (bitmap != null) { + bitmap.applyFilters(this, filters, divView.div2Component, resolver) { + setImageBitmap(it) + } + } else { + setImageBitmap(null) } } @@ -126,11 +136,14 @@ internal class DivImageBinder @Inject constructor( return !isImageLoaded || newImageUrl != imageUrl } - private fun DivImageView.observePreview(divView: Div2View, resolver: ExpressionResolver, div: DivImage) { + private fun DivImageView.observePreview(divView: Div2View, + resolver: ExpressionResolver, + errorCollector: ErrorCollector, + div: DivImage) { div.preview?.let { val callback = { _: Any -> if (!isImageLoaded) { - applyPreview(divView, resolver, div, synchronous = true) + applyPreview(divView, resolver, div, errorCollector, synchronous = true) } } @@ -138,9 +151,14 @@ internal class DivImageBinder @Inject constructor( } } - private fun DivImageView.applyPreview(divView: Div2View, resolver: ExpressionResolver, div: DivImage, synchronous: Boolean) { + private fun DivImageView.applyPreview(divView: Div2View, + resolver: ExpressionResolver, + div: DivImage, + errorCollector: ErrorCollector, + synchronous: Boolean) { placeholderLoader.applyPlaceholder( this, + errorCollector, div.preview?.evaluate(resolver), div.placeholderColor.evaluate(resolver), synchronous = synchronous, @@ -160,7 +178,10 @@ internal class DivImageBinder @Inject constructor( ) } - private fun DivImageView.applyImage(divView: Div2View, resolver: ExpressionResolver, div: DivImage) { + private fun DivImageView.applyImage(divView: Div2View, + resolver: ExpressionResolver, + errorCollector: ErrorCollector, + div: DivImage) { if (!shouldReloadImage(resolver, div)) { applyTint(resolver, div.tintColor, div.tintMode) return @@ -172,7 +193,7 @@ internal class DivImageBinder @Inject constructor( val newImageUrl = div.imageUrl.evaluate(resolver) newImageUrl.applyIfNotEquals(imageUrl) { resetImageLoaded() } - applyPreview(divView, resolver, div, synchronous = isHighPriorityShowPreview) + applyPreview(divView, resolver, div, errorCollector, synchronous = isHighPriorityShowPreview) val reference = imageLoader.loadImage( newImageUrl.toString(), diff --git a/client/android/div/src/test/java/com/yandex/div/core/view2/divs/DivImageBinderTest.kt b/client/android/div/src/test/java/com/yandex/div/core/view2/divs/DivImageBinderTest.kt index 3ef795b83..5466205b2 100644 --- a/client/android/div/src/test/java/com/yandex/div/core/view2/divs/DivImageBinderTest.kt +++ b/client/android/div/src/test/java/com/yandex/div/core/view2/divs/DivImageBinderTest.kt @@ -8,6 +8,7 @@ import com.yandex.div.core.images.CachedBitmap import com.yandex.div.core.images.DivImageDownloadCallback import com.yandex.div.core.view2.DivPlaceholderLoader import com.yandex.div.core.view2.divs.widgets.DivImageView +import com.yandex.div.core.view2.errors.ErrorCollectors import com.yandex.div.json.expressions.ExpressionResolver import com.yandex.div2.DivImage import org.junit.Assert @@ -32,8 +33,11 @@ import org.robolectric.RobolectricTestRunner class DivImageBinderTest : DivBinderTest() { private val placeholderLoader = mock() + private val errorCollector = mock { + on { getOrCreate(anyOrNull(), anyOrNull()) } doReturn mock() + } private val binder = DivImageBinder( - baseBinder, imageLoader, placeholderLoader, + baseBinder, imageLoader, placeholderLoader, errorCollector ) @Before @@ -65,7 +69,7 @@ class DivImageBinderTest : DivBinderTest() { binder.bindView(view, divContainer, divView) - verify(placeholderLoader).applyPlaceholder(any(), anyOrNull(), any(), any(), any(), any()) + verify(placeholderLoader).applyPlaceholder(any(), any(), anyOrNull(), any(), any(), any(), any()) verify(imageLoader).loadImage(eq(divContainer.imageUrl.evaluate(ExpressionResolver.EMPTY).toString()), any()) } @@ -77,7 +81,7 @@ class DivImageBinderTest : DivBinderTest() { val (_, nextDivContainer) = createTestDiv("with_action.json") binder.bindView(view, nextDivContainer, divView) - verify(placeholderLoader, times(2)).applyPlaceholder(any(), anyOrNull(), any(), any(), any(), any()) + verify(placeholderLoader, times(2)).applyPlaceholder(any(), any(), anyOrNull(), any(), any(), any(), any()) verify(imageLoader, times(2)).loadImage(eq(divContainer.imageUrl.evaluate(ExpressionResolver.EMPTY).toString()), any()) } @@ -106,7 +110,7 @@ class DivImageBinderTest : DivBinderTest() { val nextDivContainer = DivImage(imageUrl = Uri.parse("https://foo.bar/foo.png").asExpression()) binder.bindView(view, nextDivContainer, divView) - verify(placeholderLoader, times(2)).applyPlaceholder(any(), anyOrNull(), any(), any(), any(), any()) + verify(placeholderLoader, times(2)).applyPlaceholder(any(), any(), anyOrNull(), any(), any(), any(), any()) verify(imageLoader).loadImage(eq(nextDivContainer.imageUrl.evaluate(ExpressionResolver.EMPTY).toString()), any()) } @@ -120,7 +124,7 @@ class DivImageBinderTest : DivBinderTest() { ) binder.bindView(view, divContainer, divView) verify(placeholderLoader).applyPlaceholder( - any(), anyOrNull(), any(), + any(), any(), anyOrNull(), any(), synchronous = eq(true), any(), any() ) @@ -133,7 +137,7 @@ class DivImageBinderTest : DivBinderTest() { binder.bindView(view, nextDivContainer, divView) verify(placeholderLoader).applyPlaceholder( - any(), anyOrNull(), any(), + any(), any(), anyOrNull(), any(), synchronous = eq(false), any(), any() ) } @@ -228,7 +232,7 @@ class DivImageBinderTest : DivBinderTest() { private fun whenImageLoaded(imageUrl: String) { val imageDownloadCallbackCaptor = argumentCaptor() - verify(placeholderLoader).applyPlaceholder(any(), anyOrNull(), any(), any(), any(), any()) + verify(placeholderLoader).applyPlaceholder(any(), any(), anyOrNull(), any(), any(), any(), any()) verify(imageLoader).loadImage(eq(imageUrl), imageDownloadCallbackCaptor.capture()) val cachedBitmap = mock { on { bitmap } doReturn mock() @@ -237,9 +241,9 @@ class DivImageBinderTest : DivBinderTest() { } private fun whenPreviewLoaded() { - val previewSetCallbackCaptor = argumentCaptor<(Bitmap) -> Unit>() + val previewSetCallbackCaptor = argumentCaptor<(Bitmap?) -> Unit>() val bitmap = mock() - verify(placeholderLoader).applyPlaceholder(any(), anyOrNull(), any(), any(), any(), previewSetCallbackCaptor.capture()) + verify(placeholderLoader).applyPlaceholder(any(), any(), anyOrNull(), any(), any(), any(), previewSetCallbackCaptor.capture()) previewSetCallbackCaptor.firstValue.invoke(bitmap) }