Repository navigation
fix(chat): read an image data URL's type from its header, not by copying it - #413
Merged
Merged
Conversation
…ing it Crashlytics (v1.2.27) has an OOM in ChatViewModel.imageMime: a 76 MB allocation failed inside String.toLowerCase on a file part's data URL. The heap was already nearly full (the 76 MB string is itself resident), so the extra copy is what tipped it over. Finding the MIME type of a `data:` URL copied the payload up to twice per call: - parseDataImageUri removed the "data:" prefix (a full copy) and then took the payload after the comma (a second copy), only for imageMime to keep the mime. - When it returned null - a part whose declared type is not an image, such as an application/octet-stream or PDF data URL - imageMime fell through to its file-extension fallback, whose lowercase() copied the payload again, after looking for ".png" inside base64 text. That is the allocation that failed. toChatPart runs for every part on every transcript load. Parts that declare an image type return before reaching this, so the failing path is non-image data URLs. dataImageMime reads only the first 1024 characters, so the type comes from the header and nothing larger than the header is copied. imageMime and the image viewer's fallback no longer sniff an extension out of a data URL, and parseDataImageUri now makes the one copy its callers need (the base64 to decode). Ordinary URLs behave as before. This removes the allocation that failed; it does not explain why the heap was nearly full. loadChatImageBytes still copies once before rejecting an oversized payload. DataImageMimeTest watches how far into the text the header check reads and whether it copies a large span; it did not compile before the change. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the Crashlytics OOM in
ChatViewModel.imageMime(v1.2.27, 3 events / 2 users):String.toLowerCasefailed to allocate 76 MB while converting a file part whose URL is a largedata:URL.Finding the MIME type of a
data:URL copied its payload, up to twice per call:parseDataImageUriremoved thedata:prefix (a full copy) and then took the payload after the comma (a second copy), only forimageMimeto keep the mime.application/octet-streamor PDF data URL)imageMimefell through to its file-extension fallback, whoselowercase()copied the payload again, after looking for.pnginside base64 text. That is the allocation that failed.toChatPartruns for every part on every transcript load. Parts that declare an image type return before reaching this, so the failing path is non-imagedata:URLs.What changed
dataImageMime(url: CharSequence)reads only the first 1024 characters and returns the type from the header. Nothing larger than the header is copied.imageMimeand the image viewer's fallback type no longer look for a file extension inside adata:URL.parseDataImageUrimakes the one copy its callers need (the base64 to decode).https://…/shot.png,/workspace/photo.jpg) are unchanged.One deliberate behaviour change: the header is now only searched for its comma within the first 1024 characters. The app's own producers (
AttachmentImporter,CodexItemParser,OpenCodeV2ApiClient) only build shortdata:<mime>;base64,headers, so this only affects external URLs with very long parameters; those parts now fall out of the display instead of being decoded. It cannot crash.What this does not claim
toChatPartpath. It does not find why the heap was already nearly full: the 76 MB string is itself resident,largeHeapis already on, and the same payload can be held several times over by the V2 client while a transcript loads. The other OOM issues in Crashlytics (Compose snapshot, coroutines, OEM framework frames) are victims of that same pressure and are not individually fixable here.Not in this PR (known follow-ups)
From the review, none blocking:
loadChatImageBytescopies the base64 once and only then checksMAX_IMAGE_BASE64_CHARS, so an oversized URL is rejected after a copy. Checkingsource.url.lengthfirst would reject it with no allocation.CodexItemParser.userInputImagePartstill doesurl.substringAfter("data:", "").substringBefore(';'), which copies a whole data URL on every Codex history read.dataImageMime(url) ?: "image/png"would replace it.decodeImageFromUrlOrPathanddecodeDataImageinChatMessageComponents.kthave no callers anywhere in the repo (checked across all source sets). They decode the whole base64 with no size limit, so they are a trap if reused; worth deleting separately.Test plan
./gradlew spotlessCheck detekt :app:testGithubDebugUnitTest— 1627 tests, 0 failures. New:DataImageMimeTest(11). It watches how far into the text the header check reads and whether it copies a large span, and it did not compile before the change../gradlew :app:assembleGithubDebug :app:lintGithubDebug— 0 errors, no findings in the changed filesPre-PR review
The
repo-reviewersubagent is defined for OpenCode (.opencode/agents/repo-reviewer.md) and is not available in the Claude Code session that produced this change. The review below was done by a read-only Claude Code subagent given that definition as its role and output format. It is a stand-in, not therepo-reviewersubagent itself.It reviewed commit
3f812d16. The amendedHEADdiffers from it only in KDoc and commit-message wording (the reviewer found the original claimed "three copies" and that every image part paid the cost, which overstated it; both are corrected above) and in three tests the reviewer asked for. No production logic differs.Pre-PR review
判定: APPROVE
対象: ブランチ fix-oom-large-image-payloads(コミット 3f812d16 の1件、
origin/mainの先頭を祖先に持つ)。ブロッカー
なし
提案(非ブロッキング)
removePrefix、substring(comma+1)の2回で、そこでimageMimeが return する。非画像・非 base64 ではremovePrefixと拡張子フォールバックのlowercase()の2回。3回すべてが同じ呼び出しで起きる経路はない。image/*の画像パートは、toChatPartが早期 return するのでコピーは0回(旧コードも)。OOM が出た経路(lowercase()に到達)は、parseDataImageUriが null を返した場合だけなので、「画像パート」ではなく、宣言 MIME が画像でない file パートの data URL(例:OpenCodeV2ApiClient.kt:1303-1304が組み立てるdata:application/octet-stream;base64,...や PDF)である。lowercase()が 76MB の確保に失敗した。コピーは不要なのでヘッダ1024文字だけを読む」。loadChatImageBytes)—parseDataImageUriで base64 をsubstringしてからMAX_IMAGE_BASE64_CHARSを検査している。過大な data URL は、1回の 76MB コピーを作ってから拒否される。旧コードは2回で、今回は1回に減っただけで、OOM の発生点が拒否前のコピーに残る。ヘッダ長を足した上限(source.url.lengthがMAX_IMAGE_BASE64_CHARSに1024を足した値以下)を先に確認すれば、拒否は割り当てなしでできる。userInputImagePart)—url.substringAfter("data:", "").substringBefore(';')は、data URL 全体を1回コピーしてから MIME を取り出す。Codex の履歴を読むたびに通る、同種のコピーが残る箇所。dataImageMime(url) ?: "image/png"で置き換えられる。同種の箇所は AttachmentImporter.kt:62 と AntigravityRuntime.kt:586 のsubstringAfter(";base64,")にもあるが、こちらは送信側で本件のロード経路ではない。indexOf(',')を無制限に走査する実装に戻しても通る。"data:image/png;base64" + "A".repeat(2_000_000)(カンマなし)をWatchedCharsで渡し、null が返りhighestIndexReadが1024付近で止まること。②ヘッダが1024文字を超える URL(例:data:image/png;name=に長い名前、;base64,AAAA)が null になる境界値。effectiveImageMime(ChatImageViewer.kt:267-270、private)の挙動変更(data URL かつ非画像ならimage/png)にテストがない。safeImageFilenameは public だが未テストなので、ChatImageSource("data:text/plain,notes.png", "image/*")などで、拡張子が.pngになり、ペイロード中の.で誤判定されないことを確認するテストを足せる。decodeImageFromUrlOrPath、decodeDataImage)は、本調査どおり未使用のデッドコード。別PRで削除したい。decodeDataImageは残る限り、base64 全体をデコードするパスとして将来の誤用を招く。チェック済み項目
parseDataImageUriとの同値性: 実コードで比較した。payload = url.removePrefix("data:")、comma = payload.indexOf(',')、meta = payload.substring(0, comma)と進み、endsWith(";base64")とstartsWith("image/")を判定し、payload.substring(comma+1)を返す。dataImageMimeは、インデックス5("data:"の直後)からmin(length, 1024)未満を走査し、最初のカンマでsubSequence(5, comma).toString()を取り、同じ判定をする。"data:"にカンマは含まれないので、新実装が見つけたカンマは、parseDataImageUri側のurl.indexOf(',')が返す位置と必ず同じ(dataImageMimeが null でない時点で先頭1024文字内にカンマがあり、それが URL 全体の最初のカンマ)。したがってsubstring(indexOf(',') + 1)はペイロードと完全一致する。空ペイロード、カンマ直後、metaが空、MIME パラメータ付きの扱いも同じ。data:<mime>;base64,の短いヘッダしか作らず、MIME もサーバ由来の短い文字列。実際に1024文字を超えるのは、;name=などで長いパラメータが付く外部由来の URL に限られる。影響は、toChatPartでそのパートが表示から落ちること(旧実装では表示された)と、loadChatImageBytesが「Invalid image data URL」で失敗してプレビューに落ちること。クラッシュにはならず、許容できる。KDoc に上限が書かれているのも妥当。imageMime/effectiveImageMimeの挙動変更:parseDataImageUriはloadChatImageBytesと未使用のdecodeDataImageのみ、imageMimeはtoChatPart、effectiveImageMimeはsafeImageFilenameとsaveChatImageToPicturesのみで、取りこぼしはない。.pngを拾うなど)が、base64 本文に.は含まれないので、通常はnull/image/pngに落ちていた。新実装はimageMimeで null、effectiveImageMimeでimage/pngを返す。画像として扱われていたケースは、どちらもparseDataImageUriが成功した(画像 MIME かつ base64 かつヘッダが短い)場合に限られ、そこは変わらない。URL エンコードされた SVG(data:image/svg+xml,...)は、旧実装でもparseDataImageUriが null で、拡張子フォールバックはほぼ null/image/pngに落ちており、実質的に退行はない。toChatPartのisNotBlank()(先頭の非空白で止まる)、ChatImageSourceの使い方に変化はない。通常の URL(https://...png、/workspace/photo.jpg)の拡張子判定は従来どおり。decodeImageFromUrlOrPath/decodeDataImageは未使用: リポジトリ全体を grep した(build、.git、.gradle以外)。ヒットは定義とdecodeImageFromUrlOrPathからdecodeDataImageへの1箇所だけで、app/src/test、androidTest、fdroid、githubの各ソースセット、Manifest、proguard-rules.proにも参照はない。単一モジュール(app)のためinternalの外部利用もなく、リフレクション参照もない。本当に未使用で、触らない判断は妥当。WatchedCharsは、CharSequence.startsWith(getを5回)、indexOf(indexOfAny経由のget)、subSequence、toStringの経路をすべて拾う。ヘッダだけを読むこと(highestIndexRead < 1024)と、1024文字超のsubSequenceやtoString()が呼ばれないこと(largeCopies == 0)は、実際に検証できている。imageMimeのテストは結果ベースで、$payload.pngを使った「データ URL から拡張子を拾わない」の検証と、通常 URL と宣言 MIME 優先の検証が揃っている(旧実装はdataImageMimeが無くコンパイルエラーになる)。parseDataImageUriの「ペイロード全体が一致する」検証も、既存の ChatImagePartTest と合わせて同値性を押さえている。imageMimeのlowercase())の割り当てが無くなり、toChatPart経路で data URL に対するコピーが0になること。トレース上、この3件・2ユーザーの再発は止まる見込みが高い。android:largeHeap="true"が既に有効(AndroidManifest.xml:58)なのに 76MB の確保が失敗しているので、ヒープはすでにかなり埋まっていた。1回の一時コピーを減らしても、別の割り当てで OOM が移るだけの可能性が残る。②同じペイロードは、V2 クライアントで JSON 本文、file.textOf("data")、"data:$mime;base64,$data"のテンプレート結果(OpenCodeV2ApiClient.kt:1303-1304)と、複数の文字列で同時に載る。ロード全体のピークを決めるのはこちらで、本PRはその一部にしか効かない。③toUiMessage(ChatViewModel.kt:2596 付近)のPromptAttachment(url = part.url)やreloadedImageUrls(:175)は参照保持とハッシュ計算だけでコピーはないが、文字列は会話の存続中ずっと保持される。④ビューア経路(loadChatImageBytes)は1回のコピーが残る(上記の提案)。origin/main基点、テストあり。spotless・detekt・単体テスト(1624件)のグリーンは依頼文の報告に依拠しており、再実行はしていない(読み取り専用のため)。assembleGithubDebug と lintGithubDebug は実行中とのことなので、マージ前に緑であることを確認してほしい。🤖 Generated with Claude Code