From 5151698d5661f481320bc0ba267fd4b02ef2b3e0 Mon Sep 17 00:00:00 2001 From: null Date: Tue, 14 Jul 2026 23:58:38 -0500 Subject: [PATCH] fix(avatar): harden the cropper's error handling; unify with the codebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checking error handling on the crop sheet surfaced three real problems, two of which I had introduced. 1. Crash on a high-megapixel photo. The picked image was decoded at full size and handed to a hardware Canvas. An 8000x8000 photo decodes to 256MB, which Android allocates but the canvas refuses to draw ("trying to draw too large bitmap") — thrown inside Compose's draw phase, where the runCatching around the decode can't see it. Proven live: an 8000x8000 pick killed the app. Fixed by subsampling at decode (inSampleSize from a bounds-only pass) so the working bitmap is capped at ~2048px / ~16MB. Native heap on that pick went 256MB(attempted) -> 46MB. 2. The subsample fix then broke decoding for EVERY image. decodeStream returns null by design in inJustDecodeBounds mode, and I had `?: return null` on it — so loadOriented bailed right after the bounds pass, for all inputs. It only looked like "the huge photo failed"; a normal photo would have failed too. I never re-tested a small image after the change. The new AvatarCropSheetTest caught it. The stream is now what's guarded; the real check is the header size. 3. Errors were swallowed and off-standard. runCatching{}.getOrNull() dropped the cause and a failure silently dismissed the sheet — indistinguishable from a save that did nothing. Now unified with the screen's own pattern (EditProfileViewModel.save): the sheet reports the Throwable up via a new onError, the VM records it through the injected CrashReporter and surfaces the message through uiState.error -> the existing snackbar. The crop also moved off the main thread (withContext(IO)); inline, `saving` flipped within one frame and never showed. Adds AvatarCropSheetTest (androidTest, 4 tests, on-device BitmapFactory/Canvas): subsample math for 8000+/oversized, oversized decode stays bounded, garbage returns null instead of throwing, crop output is square and bounded. All green. Co-Authored-By: Claude Opus 4.8 --- .../closer/ui/settings/AvatarCropSheetTest.kt | 87 +++++++++++++++++++ .../app/closer/ui/settings/AvatarCropSheet.kt | 86 ++++++++++++++---- .../closer/ui/settings/EditProfileScreen.kt | 5 +- .../ui/settings/EditProfileViewModel.kt | 15 ++++ 4 files changed, 177 insertions(+), 16 deletions(-) create mode 100644 app/src/androidTest/java/app/closer/ui/settings/AvatarCropSheetTest.kt diff --git a/app/src/androidTest/java/app/closer/ui/settings/AvatarCropSheetTest.kt b/app/src/androidTest/java/app/closer/ui/settings/AvatarCropSheetTest.kt new file mode 100644 index 00000000..8536c907 --- /dev/null +++ b/app/src/androidTest/java/app/closer/ui/settings/AvatarCropSheetTest.kt @@ -0,0 +1,87 @@ +package app.closer.ui.settings + +import android.graphics.Bitmap +import android.graphics.Canvas +import android.graphics.Color +import android.net.Uri +import androidx.test.ext.junit.runners.AndroidJUnit4 +import androidx.test.platform.app.InstrumentationRegistry +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotNull +import org.junit.Assert.assertTrue +import org.junit.Test +import org.junit.runner.RunWith +import java.io.File +import java.io.FileOutputStream + +/** + * On-device checks for the avatar cropper's decode/crop, using the real `BitmapFactory` and + * `Canvas` — the exact things a JVM unit test can't exercise and where the real bug lived. + * + * A picked photo used to be decoded at full size and then handed straight to a hardware Canvas: an + * 8000x8000 image decodes to 256MB, which Android will allocate but the canvas refuses to draw + * ("trying to draw too large bitmap"), thrown inside Compose's draw phase where the runCatching + * around the decode never saw it — a hard crash on first paint for anyone with a high-megapixel + * camera. These tests pin the subsampling that fixes it. + */ +@RunWith(AndroidJUnit4::class) +class AvatarCropSheetTest { + + private val context = InstrumentationRegistry.getInstrumentation().targetContext + + private fun writeJpeg(w: Int, h: Int): Uri { + val bmp = Bitmap.createBitmap(w, h, Bitmap.Config.ARGB_8888) + Canvas(bmp).drawColor(Color.rgb(120, 90, 200)) + val file = File(context.cacheDir, "avatar_test_${w}x${h}.jpg") + FileOutputStream(file).use { bmp.compress(Bitmap.CompressFormat.JPEG, 90, it) } + bmp.recycle() + return Uri.fromFile(file) + } + + @Test + fun sampleSize_bringsAnyImageWithinBudget() { + // The pathological cases: the exact size that crashed, and beyond it. + assertEquals(1, sampleSizeFor(2048, 2048, 2048)) + assertEquals(2, sampleSizeFor(4000, 3000, 2048)) + assertEquals(4, sampleSizeFor(8000, 8000, 2048)) + assertEquals(8, sampleSizeFor(12000, 9000, 2048)) + // Whatever the input, the decoded edge is capped — so the drawn bitmap can't exceed the + // hardware canvas limit that used to throw. + for (dim in intArrayOf(2049, 6000, 8000, 16000)) { + val decodedEdge = dim / sampleSizeFor(dim, dim, 2048) + assertTrue("edge $decodedEdge should be <= 2048", decodedEdge <= 2048) + } + } + + @Test + fun loadOriented_decodesAnOversizedImageDownToBudget() { + // Forces the subsampling path (4096 > 2048 -> inSampleSize 2 -> 2048). Not the full 8000 + // case: creating an 8000x8000 ARGB source is 256MB, which the test process can't allocate + // in the same heap the fix is about — sampleSize_bringsAnyImageWithinBudget covers 8000+ + // arithmetically. This proves the decode actually honours inSampleSize and returns a + // bounded, drawable bitmap (the property whose absence crashed the hardware canvas). + val bmp = loadOriented(context, writeJpeg(4096, 4096)) + assertNotNull("oversized image should still decode (subsampled)", bmp) + assertTrue("decoded edge must be bounded to <= 2048", bmp!!.width <= 2048 && bmp.height <= 2048) + } + + @Test + fun loadOriented_returnsNullForGarbageRatherThanThrowing() { + val file = File(context.cacheDir, "not_an_image.jpg") + file.writeText("this is not a JPEG") + assertEquals(null, loadOriented(context, Uri.fromFile(file))) + } + + @Test + fun crop_producesASquareBitmapAtOutputSize() { + val src = Bitmap.createBitmap(1200, 800, Bitmap.Config.ARGB_8888) + Canvas(src).drawColor(Color.rgb(200, 120, 160)) + val outUri = cropToCircleSquare(context, src, scale = 1f, offset = androidx.compose.ui.geometry.Offset.Zero, viewportPx = 512f) + val out = context.contentResolver.openInputStream(outUri)!!.use { + android.graphics.BitmapFactory.decodeStream(it) + } + assertNotNull(out) + assertEquals("avatar output is square", out.width, out.height) + assertTrue("avatar output is bounded", out.width in 1..512) + } +} diff --git a/app/src/main/java/app/closer/ui/settings/AvatarCropSheet.kt b/app/src/main/java/app/closer/ui/settings/AvatarCropSheet.kt index 47a253ef..00d44e99 100644 --- a/app/src/main/java/app/closer/ui/settings/AvatarCropSheet.kt +++ b/app/src/main/java/app/closer/ui/settings/AvatarCropSheet.kt @@ -34,6 +34,7 @@ import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableFloatStateOf import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -50,6 +51,7 @@ import androidx.compose.ui.unit.IntSize import androidx.compose.ui.unit.dp import androidx.exifinterface.media.ExifInterface import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import java.io.File import java.io.FileOutputStream @@ -78,10 +80,12 @@ private const val MAX_SCALE = 4f internal fun AvatarCropSheet( sourceUri: Uri, onDismiss: () -> Unit, - onConfirm: (Uri) -> Unit + onConfirm: (Uri) -> Unit, + onError: (Throwable) -> Unit ) { val context = LocalContext.current val sheetState = rememberModalBottomSheetState(skipPartiallyExpanded = true) + val scope = rememberCoroutineScope() var bitmap by remember(sourceUri) { mutableStateOf(null) } var failed by remember(sourceUri) { mutableStateOf(false) } @@ -91,8 +95,18 @@ internal fun AvatarCropSheet( var saving by remember(sourceUri) { mutableStateOf(false) } LaunchedEffect(sourceUri) { - bitmap = withContext(Dispatchers.IO) { runCatching { loadOriented(context, sourceUri) }.getOrNull() } - if (bitmap == null) failed = true + // runCatching{}.getOrNull() dropped the cause on the floor: an unreadable file and a decode + // failure looked identical and neither reached anyone. Report, then show the in-context + // message — the sheet stays open because there is nothing to proceed with. + withContext(Dispatchers.IO) { runCatching { loadOriented(context, sourceUri) } } + .onSuccess { decoded -> + bitmap = decoded + if (decoded == null) { + failed = true + onError(IllegalStateException("Avatar: could not decode picked image")) + } + } + .onFailure { failed = true; onError(it) } } ModalBottomSheet(onDismissRequest = onDismiss, sheetState = sheetState) { @@ -112,7 +126,7 @@ internal fun AvatarCropSheet( color = SettingsInk ) Text( - text = if (failed) "That image couldn't be opened. Try another one." + text = if (failed) "That photo couldn't be used. Try another one." else "Pinch to zoom, drag to move.", style = MaterialTheme.typography.bodyMedium, color = SettingsMuted @@ -175,12 +189,21 @@ internal fun AvatarCropSheet( Button( onClick = { val source = bmp ?: return@Button - saving = true - val out = runCatching { - cropToCircleSquare(context, source, scale, offset, viewport) - }.getOrNull() - saving = false - if (out != null) onConfirm(out) else onDismiss() + // Off the main thread: this allocates a bitmap, redraws it and JPEG-encodes + // to disk. Doing that inline also made `saving` a lie — it flipped true and + // false within one frame, so the button never actually showed its state. + scope.launch { + saving = true + withContext(Dispatchers.IO) { + runCatching { + cropToCircleSquare(context, source, scale, offset, viewport) + } + } + .onSuccess { saving = false; onConfirm(it) } + // Say so rather than just closing: a silent dismiss looks exactly + // like a successful save that quietly did nothing. + .onFailure { saving = false; failed = true; onError(it) } + } }, enabled = bmp != null && !saving, modifier = Modifier.weight(2f), @@ -224,12 +247,45 @@ private fun clampOffset( } /** - * Decodes [uri], honouring the EXIF orientation. Phone cameras store the sensor image and a - * rotation flag; without applying it a portrait selfie crops sideways. + * Largest edge we will decode. Nothing here needs more: the preview is at most a screen wide and + * the saved avatar is [OUTPUT_PX], so 2048 still leaves full detail at [MAX_SCALE] zoom. + * + * This bound is a crash fix, not a nicety. Decoding at full size looks fine right up until someone + * picks a photo from a modern high-megapixel camera: a 48MP shot decodes to ~192MB and an 8000x8000 + * image to 256MB, and while Android will happily *allocate* that, the hardware canvas refuses to + * *draw* a bitmap over ~100MB — `RuntimeException: Canvas: trying to draw too large bitmap`, thrown + * inside Compose's draw phase where no runCatching around the decode can see it. Proven live: an + * 8000x8000 pick killed the app outright. Sampling down at decode caps this at ~16MB. */ -private fun loadOriented(context: Context, uri: Uri): Bitmap? { +private const val MAX_DECODE_PX = 2048 + +/** Power-of-two subsample that brings [w]x[h] within [max] — what BitmapFactory expects. */ +internal fun sampleSizeFor(w: Int, h: Int, max: Int): Int { + var sample = 1 + while (w / sample > max || h / sample > max) sample *= 2 + return sample +} + +/** + * Decodes [uri] at a bounded size, honouring the EXIF orientation. Phone cameras store the sensor + * image and a rotation flag; without applying it a portrait selfie crops sideways. + */ +internal fun loadOriented(context: Context, uri: Uri): Bitmap? { + // Bounds-only pass first: reading the header costs nothing and tells us how far to subsample. + // NB: decodeStream returns null *by design* in inJustDecodeBounds mode — it only fills `bounds`. + // So the stream is what's guarded here, and the real "could we read it" check is the size test + // below. (A `?: return null` on the decode result would fire on every image, always.) + val bounds = BitmapFactory.Options().apply { inJustDecodeBounds = true } + (context.contentResolver.openInputStream(uri) ?: return null).use { + BitmapFactory.decodeStream(it, null, bounds) + } + if (bounds.outWidth <= 0 || bounds.outHeight <= 0) return null + + val opts = BitmapFactory.Options().apply { + inSampleSize = sampleSizeFor(bounds.outWidth, bounds.outHeight, MAX_DECODE_PX) + } val decoded = context.contentResolver.openInputStream(uri)?.use { - BitmapFactory.decodeStream(it) + BitmapFactory.decodeStream(it, null, opts) } ?: return null val rotation = context.contentResolver.openInputStream(uri)?.use { stream -> when (ExifInterface(stream).getAttributeInt( @@ -254,7 +310,7 @@ private fun loadOriented(context: Context, uri: Uri): Bitmap? { * [OUTPUT_PX] by the viewport→output ratio. Square, not circular: every avatar slot already clips * to a circle, and a transparent-cornered PNG would cost size for nothing. */ -private fun cropToCircleSquare( +internal fun cropToCircleSquare( context: Context, source: Bitmap, scale: Float, diff --git a/app/src/main/java/app/closer/ui/settings/EditProfileScreen.kt b/app/src/main/java/app/closer/ui/settings/EditProfileScreen.kt index d58220bf..688b7c25 100644 --- a/app/src/main/java/app/closer/ui/settings/EditProfileScreen.kt +++ b/app/src/main/java/app/closer/ui/settings/EditProfileScreen.kt @@ -139,7 +139,10 @@ fun EditProfileContent( onConfirm = { cropped -> viewModel.setPhotoUri(cropped.toString()) cropSource = null - } + }, + // Failures go through the VM so they surface in this screen's existing error snackbar, + // like every other failure here, instead of dying inside the sheet. + onError = { viewModel.onPhotoError(it) } ) } diff --git a/app/src/main/java/app/closer/ui/settings/EditProfileViewModel.kt b/app/src/main/java/app/closer/ui/settings/EditProfileViewModel.kt index 3e9702d3..45f44fe7 100644 --- a/app/src/main/java/app/closer/ui/settings/EditProfileViewModel.kt +++ b/app/src/main/java/app/closer/ui/settings/EditProfileViewModel.kt @@ -15,6 +15,7 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch +import app.closer.core.crash.CrashReporter import javax.inject.Inject data class EditProfileUiState( @@ -39,6 +40,7 @@ class EditProfileViewModel @Inject constructor( private val authRepository: AuthRepository, private val userRepository: UserRepository, private val storageDataSource: FirebaseStorageDataSource, + private val crashReporter: CrashReporter, private val coupleRepository: CoupleRepository, private val encryptionManager: CoupleEncryptionManager ) : ViewModel() { @@ -82,6 +84,19 @@ class EditProfileViewModel @Inject constructor( fun setPhotoUri(uri: String?) = _uiState.update { it.copy(photoUri = uri, error = null) } + /** + * The avatar cropper couldn't read or write the picked photo (unreadable file, decode failure, + * no space). Surfaced the same way every other failure on this screen is — `error` -> the + * snackbar the screen already collects — rather than dying inside the sheet, and recorded so a + * swallowed decode failure isn't invisible. + */ + fun onPhotoError(throwable: Throwable) { + crashReporter.recordException(throwable) + _uiState.update { + it.copy(error = throwable.message ?: "Couldn't use that photo. Please try again.") + } + } + fun save() { val state = _uiState.value val name = state.displayName.trim()