fix another crash in ShareShortcutHelper

[connyduck]

Glide sometimes calls the callback more than once (for the placeholder, then for the actual image), but a coroutine can only resume once.

Exception java.lang.IllegalStateException:
  at kotlinx.coroutines.CancellableContinuationImpl.alreadyResumedError (CancellableContinuationImpl.kt:555)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl (CancellableContinuationImpl.kt:520)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeImpl$default (CancellableContinuationImpl.kt:493)
  at kotlinx.coroutines.CancellableContinuationImpl.resumeWith (CancellableContinuationImpl.kt:364)
  at com.keylesspalace.tusky.util.GlideExtensionsKt$submitAsync$2$target$1.onResourceReady (GlideExtensions.kt:39)
  at com.bumptech.glide.request.SingleRequest.onResourceReady (SingleRequest.java:650)
  at com.bumptech.glide.request.SingleRequest.onResourceReady (SingleRequest.java:596)
  at com.bumptech.glide.request.SingleRequest.begin (SingleRequest.java:243)
  at com.bumptech.glide.manager.RequestTracker.resumeRequests (RequestTracker.java:115)
  at com.bumptech.glide.RequestManager.resumeRequests (RequestManager.java:339)
  at com.bumptech.glide.RequestManager.onStart (RequestManager.java:364)
  at com.bumptech.glide.manager.ApplicationLifecycle.addListener (ApplicationLifecycle.java:15)
  at com.bumptech.glide.RequestManager$1.run (RequestManager.java:84)
  at android.os.Handler.handleCallback (Handler.java:958)
  at android.os.Handler.dispatchMessage (Handler.java:99)
  at android.os.Looper.loopOnce (Looper.java:230)
  at android.os.Looper.loop (Looper.java:319)
  at android.app.ActivityThread.main (ActivityThread.java:8893)
  at java.lang.reflect.Method.invoke
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:608)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1103)

While removing the placeholder fixes the problem here, we should probably put some safeguards into submitAsync so that this can't happen again elsewhere. Any ideas how to do that, @cbeyls?

[cbeyls]

My bad, I should have looked closer at the Glide API!

I'll take a look later today as soon as I come back home.

[cbeyls]

Here's a recap of my struggles with the Glide API to retrieve an image outside of a View. My apologies for the long read.

submit() returns a FutureTarget which implements java.util.concurrent.Future. Ideally this should have implemented CompletableFuture or ListenableFuture so we could simply add a listener to turn it into a suspending function without blocking a Thread, but unfortunately this class only implements Future.

My next objective was to implement a custom Target that would do the same thing as RequestFutureTarget (the current implementation of FutureTarget) without the blocking part. It turns out RequestFutureTarget implements both Target and RequestListener, two interfaces with overlapping functionality but slightly different behavior. Implementing RequestListener is mandatory if you want to access the actual exception in case of error so I thought I could get away with simply registering a custom RequestListener and use the existing RequestFutureTarget to launch the request and cancel it. This is the current code in the app.

However, it turns out that both Target.onResourceReady() and RequestListener.onResourceReady(), which receive different arguments, can both end up being called multiple times for various reasons (even if I never noticed it myself during my tests). I was surprised to discover that RequestFutureTarget, despite using similar logic, doesn't crash like suspendCancellableCoroutine() when called with multiple results. And also to discover that RequestFutureTarget could return the placeholder instead of the actual result !

I dug up further into the Glide codebase and found out that it's possible to find out if the resource is the placeholder or the actual image by checking if the current Request is complete or not. The Request can be retrieved from the Target.

I also found out that Glide provides an experimental module containing methods to retrieve a Flow allowing to observe a request, that hooks into the private API as well. I briefly considered using that to reimplement submitAsync() in a simple way, but unfortunately that Flow API has 2 problems:

• It doesn't return the actual GlideException in case of error
• It clears all resources when the Flow stops being collected, which means that the retrieved Bitmap could be recycled. So this API can only be used if the Bitmap is processed within the Flow collection and not after it.

In conclusion: the Glide API is a mess but I still managed to find a way to properly detect if the image is the final result and return it once. I'll send the code later.

[cbeyls]

Here's the updated implementation:

/**
 * Allows waiting for a Glide request to complete without blocking a background thread.
 */
suspend fun <R> RequestBuilder<R>.submitAsync(
    width: Int = Target.SIZE_ORIGINAL,
    height: Int = Target.SIZE_ORIGINAL
): R {
    return suspendCancellableCoroutine { continuation ->
        val target = addListener(
            object : RequestListener<R> {
                override fun onLoadFailed(
                    e: GlideException?,
                    model: Any?,
                    target: Target<R>,
                    isFirstResource: Boolean
                ): Boolean {
                    continuation.resumeWithException(e ?: GlideException("Image loading failed"))
                    return false
                }

                override fun onResourceReady(
                    resource: R & Any,
                    model: Any,
                    target: Target<R>?,
                    dataSource: DataSource,
                    isFirstResource: Boolean
                ): Boolean {
                    if (target?.request?.isComplete == true) {
                        continuation.resume(resource)
                    }
                    return false
                }
            }
        ).submit(width, height)
        continuation.invokeOnCancellation { target.cancel(true) }
    }
}

I added a test in onResourceReady(). This is safer in theory, but since I was not able to reproduce the crash with the original implementation I cannot test if it behaves better in the same conditions.

[cbeyls]

Not related to the bug: to avoid converting the fallback vector drawable to a Bitmap then drawing that Bitmap into another Bitmap, we can simply draw a generic Drawable into a Bitmap:

                val drawable = try {
                    Glide.with(context)
                        .load(account.profilePictureUrl)
                        .submitAsync(innerSize, innerSize)
                } catch (e: GlideException) {
                    // https://github.com/bumptech/glide/issues/4672 :/
                    Log.w(TAG, "failed to load avatar ${account.profilePictureUrl}", e)
                    AppCompatResources.getDrawable(context, R.drawable.avatar_default) ?: return@mapNotNull null
                }

                // inset the loaded bitmap inside a 108dp transparent canvas so it looks good as adaptive icon
                val outBmp = Bitmap.createBitmap(outerSize, outerSize, Bitmap.Config.ARGB_8888)

                val canvas = Canvas(outBmp)
                val borderSize = (outerSize - innerSize) / 2
                drawable.setBounds(borderSize, borderSize, borderSize + innerSize, borderSize + innerSize)
                drawable.draw(canvas)

[connyduck]

Thanks @cbeyls, looks good, please send a PR!