Crash reporter reliability, QR analyzer frame handling, and the unresolvable Send intent - #36
Crash reporter reliability, QR analyzer frame handling, and the unresolvable Send intent#36usr577 wants to merge 3 commits into
Conversation
0ed5d50 to
6848eb8
Compare
Opening the crash screen could kill the app a second time, so the user saw
the app vanish with no report at all:
java.lang.NullPointerException: Attempt to invoke virtual method
'java.lang.String java.lang.Object.toString()' on a null object reference
at java.lang.Throwable.printEnclosedStackTrace(Throwable.java:727)
at CrashReportData.toString(CrashReportData.kt:29)
CrashReportData held the live Throwable and printed it later, from
CrashReportActivity. But OmsUncaughtExceptionHandler passes the object as an
Intent extra and then lets the process die, so it has to survive Java
serialization and a process restart - the pids in logcat differ between the
original crash and the crash screen. A Throwable does not survive that
reliably: it can come back with a null entry in its frame array, and printing
it then throws.
The stack trace is now rendered to a String in the crashing process, where
the Throwable is still intact, and only that String is serialized. The
fallback in renderStackTrace is a constant rather than an interpolation,
because formatting the throwable would call toString() on the object that
just failed to print and could throw a second time.
Also bounds the logcat at read time via "logcat -t <n>" for the crash path.
The report crosses a Binder transaction with a ~1 MB budget, and an unbounded
"logcat -b all -d" can exceed it on a busy device - TransactionTooLargeException
thrown from inside the uncaught exception handler means no crash screen at all.
Bounding at read time rather than truncating afterwards also avoids
materialising a multi-MB string twice while the process is already failing,
which matters when the original crash was itself an OutOfMemoryError.
The bound is opt-in via a constructor parameter, so the diagnostic exports in
QRScreen, which write straight to a file and have no Binder limit, keep the
complete log.
…lled
Three paths through analyze() returned without closing the ImageProxy, and
mlTask was only ever cleared by the success and failure listeners.
Measured on a device before changing anything: across 309 analyze() calls the
"already busy" and "null mediaImage" branches were never taken once. With
STRATEGY_KEEP_ONLY_LATEST and the default image queue depth CameraX does not
re-enter analyze() until the current frame is closed, and the old code cleared
mlTask immediately before closing it, so neither branch is reachable as the use
case is configured today. They are guarded anyway because nothing enforces that
- a setImageQueueDepth() call or a different backpressure strategy makes them
live - and the failure mode is silent: leaked buffers starve the pool, the
analyzer stops receiving frames and scanning dies while the preview carries on
rendering normally.
The reachable gap is task cancellation. Both addOnSuccessListener and
addOnFailureListener skip a cancelled Task, so if ML Kit cancels one - tearing
the detector down on a lifecycle transition, for instance - mlTask stays
non-null forever and its frame is never closed, permanently killing scanning for
that Analyzer instance. mlTask is now cleared from addOnCompleteListener, which
also runs for cancellation.
Not reproduced: I have not observed a cancelled Task in the wild, this is from
the Tasks API contract that neither listener fires for one.
The foss analyzer is unaffected - it wraps the frame in imageProxy.use {}.
The ACTION_SEND intent carried a "mailto:" data URI and no MIME type. ACTION_SEND is matched on MIME type, so conventional mail filters miss that combination. Measured on a device with Gmail and K-9 installed: $ adb shell cmd package query-activities -a android.intent.action.SEND -d "mailto:" No activities found $ adb shell cmd package query-activities -a android.intent.action.SEND -t text/plain 126 matches Outlook does appear to declare a filter broad enough to match it, so on devices with Outlook installed the primary path worked. Everywhere else it threw ActivityNotFoundException and fell through to the ACTION_SENDTO fallback - which carries no attachment at all and inlines the entire report in EXTRA_TEXT instead, risking TransactionTooLargeException once the logcat is included. The black screen came from somewhere else: every branch called finish() + exitProcess(0) immediately after startActivity(). With two mail apps installed and no default set, ACTION_SENDTO raises a disambiguation chooser owned by the calling task - and killing the process takes the chooser down with it before anything can be picked. The same call also meant the "could not send" toast was never on screen long enough to be read. Changes: - ACTION_SEND sets type = "text/plain" and drops the data URI; "mailto:" moves to the ACTION_SENDTO fallback where it belongs. EXTRA_EMAIL still pre-fills the maintainer address when a mail app is picked. - Added a Copy button. This is how the crash reports behind the two fixes in stud0709#35 were retrieved from the device. - Nothing but Dismiss ends the process, so a cancelled or failed share no longer loses the report. - The report is written to cacheDir/tmp/crash/ instead of cacheDir/tmp. Six code paths call OmsFileProvider.purgeTmp unconditionally, and share targets such as Gmail resolve the content URI lazily at send time, so the attachment used to disappear as soon as the app was reopened. purgeTmp does not recurse into directories and filepaths.xml already exposes all of tmp/. - The clipboard write sets ClipDescription.EXTRA_IS_SENSITIVE, matching the other clipboard writes in the app, since the report can carry the logcat. - FLAG_SECURE on the window, matching MainActivity, for the same reason. - A missing report extra now finishes the activity instead of leaving a blank screen with no way out. - The report is inlined in EXTRA_TEXT only below 128 KB; above that the attachment carries it, so including the logcat cannot blow the transaction limit. - Removed R.string.could_not_send_email - its only reference was the deleted fallback, and Intent.createChooser always resolves. The report is written through OmsFileProvider.create with a new optional subdirectory argument rather than a direct FileProvider.getUriForFile call, so the authority string stays in one place as everywhere else in the app. One deliberate trade-off, called out so it is easy to reverse if you disagree: below 128 KB the report goes out both inline in EXTRA_TEXT and as the attachment, so a mail client shows the whole report in the body as well as attaching crash_report.txt. That is redundant to read, but a lot of share targets - chat apps, note apps, clipboard managers - silently ignore EXTRA_STREAM and would otherwise receive an empty report. Dropping the inline copy is a two-line change if you would rather the mail case stay clean.
6848eb8 to
2fb7a50
Compare
|
Two unrelated things I ran into while working on this: versionCode looks like it missed a bump. c85b336 is titled "0.43-beta", but it only touches CryptoCurrencyAddressScreen.kt, Output.kt, Theme.kt and a font file — app/build.gradle is untouched, and master still reads versionCode = 42 / versionName = "0.42-beta". If 0.43-beta was published, it shipped sharing a versionCode with 0.42-beta. Worth mentioning here specifically because of #35: R8's mapping.txt only deobfuscates the exact build it was generated from, so two releases sharing a versionCode make incoming crash reports hard to attribute to the right build. gradlew is committed as mode 100644. On a fresh clone ./gradlew fails with "Permission denied" and you need sh ./gradlew instead. |
|
I went back and checked against the published Reproduction - Then open the app and go to Settings -> Private Keys. Two crashes land 92 ms apart, in two different processes: The first is the Two things I think are worth knowing: It is not Play-specific and not ML Kit-specific. This is the Line numbers. Every frame above reads Flagging rather than pushing: Settings -> Private Keys is a hard crash on the current release across every channel according to my tests and the crash reporter cannot report it. Might be worth factoring into release timing once you have decided what to do with this PR. |
1 participant
Three independent fixes, one per commit, so any of them can be dropped without affecting the
others. They are ordered least to most invasive — the last one is the only one that changes
behaviour, so it is the easiest to leave out.
Commit 2 is the least certain of the three and is flagged as such below. If you only want
changes with a reproduction behind them, take 1 and 3.
1.
crash report: stop the crash reporter from crashing on its own reportTouches
CrashReportData.kt,OmsUncaughtExceptionHandler.kt.Opening the crash screen could kill the app a second time, so the app just disappeared and no report was ever shown:
CrashReportDataheld the liveThrowableand printed it later, fromCrashReportActivity. ButOmsUncaughtExceptionHandlerpasses the object as an Intent extra and then lets the process die — the pids in logcat differ between the original crash and the crash screen — so it has to survive Java serialization and a process restart. AThrowabledoes not survive that reliably; it can come back with a null entry in its frame array, and printing it throws.The stack trace is now rendered to a
Stringin the crashing process, where theThrowableis still intact, and only thatStringis serialized.Also bounds the logcat at read time (
logcat -t <n>) on the crash path. The report crosses a Binder transaction with a ~1 MB budget; an unboundedlogcat -b all -dcan exceed it on a busy device, and aTransactionTooLargeExceptionthrown from inside the uncaught exception handler means no crash screen at all. Bounding at read time also avoids materialising a multi-MB string while the process is already failing — relevant if the original crash was anOutOfMemoryError.The bound is opt-in via a constructor parameter, so the diagnostic exports in
QRScreen, which write straight to a file and have no Binder limit, still get the complete log.Verified by hand on a physical Samsung SM-S948B (Android 16): the double crash was originally observed there and the trace above is real device output. After the change, a synthetic uncaught exception produces a fully rendered crash report — the stack trace now survives serialization and the process restart intact, where before it took the reporter down with it.
2.
qr: close dropped camera frames and reset mlTask when a task is cancelledTouches
app/src/standard/.../qr/Analyzer.kt.fossis unaffected — it wraps the frame inimageProxy.use {}.This is the weakest of the three and I want to be upfront about why. Three paths through
analyze()returned without closing theImageProxy, andmlTaskwas only ever cleared by the success and failure listeners.I instrumented the analyzer on a device before changing anything, and across 309
analyze()calls the "already busy" and "null mediaImage" branches were never taken once. WithSTRATEGY_KEEP_ONLY_LATESTand the default image queue depth (QRScreen.kt:215-217), CameraX does not re-enteranalyze()until the current frame is closed, and the old code clearedmlTaskimmediately before closing it — so neither branch is reachable as the use case is configured today. They are guarded anyway because nothing enforces that: asetImageQueueDepth()call or a different backpressure strategy makes them live, and the failure mode is silent — leaked buffers starve the pool, the analyzer stops receiving frames, and scanning dies while the preview carries on rendering normally.The genuinely reachable gap is task cancellation. Both
addOnSuccessListenerandaddOnFailureListenerskip a cancelledTask, so if ML Kit cancels one — tearing the detector down on a lifecycle transition, for instance —mlTaskstays non-null forever and its frame is never closed, permanently killing scanning for thatAnalyzerinstance.mlTasknow gets cleared fromaddOnCompleteListener, which also runs for cancellation.Not reproduced: I have not observed a cancelled Task in the wild — that part rests on the Tasks API contract that neither listener fires for one. If you would rather not carry a change without a reproduction behind it, drop this commit; the other two stand on their own.
3.
crash report: replace the unresolvable mailto share with the share sheetTouches
CrashReportActivity.kt,strings.xml. This is the only commit that changes behaviour — happy to drop it or trim it to the intent fix alone.The Send button could never work. The
ACTION_SENDintent carried amailto:data URI and no MIME type, andACTION_SENDis matched on MIME type, so no filter can match. Measured on a real device:Every press therefore fell through to the
ACTION_SENDTOfallback, and on a device with nomailto:handler to the toast — except the toast never rendered either, because each branch calledfinish()+exitProcess(0)immediately afterstartActivity(). The process died before the target was up, leaving a black screen.Changes:
ACTION_SENDsetstype = "text/plain"and no data URI;mailto:moves to theACTION_SENDTOfallback.EXTRA_EMAILstill pre-fills your address when an email app is chosen, so reports keep arriving by mail wherever a mail app exists.cacheDir/tmp/crash/rather thancacheDir/tmp. Six code paths callOmsFileProvider.purgeTmpunconditionally, and Gmail resolves the content URI lazily at send time, so the attachment used to vanish as soon as the app was reopened.purgeTmpdoes not recurse into directories andfilepaths.xmlalready exposes all oftmp/.ClipDescription.EXTRA_IS_SENSITIVE, matchingOutput.ktandMsgPluginKeyRequest.kt— the report can contain this app's logcat.FLAG_SECUREon the window, matchingMainActivity, for the same reason.EXTRA_TEXTonly below 128 KB; above that the attachment carries it, so including the logcat cannot blow the Binder transaction limit.R.string.could_not_send_email— its only reference was the deleted fallback, andIntent.createChooseralways resolves.The report goes through
OmsFileProvider.createwith a new optional subdirectory argument rather than a directFileProvider.getUriForFilecall, so the authority string stays in one place as it is everywhere else in the app.One deliberate trade-off, flagged so it is easy to reverse if you disagree: below 128 KB the report is delivered twice — inline in
EXTRA_TEXTand as the attachment. In a mail client that means the full report appears in the body as well ascrash_report.txtbeing attached, which is redundant to read. It is done that way because a lot of share targets — chat apps, note apps, clipboard managers — silently ignoreEXTRA_STREAMand would otherwise deliver an empty report. If you would rather the mail case stay clean, dropping the inline copy is a two-line change; the cost is that the report then only survives on targets that honour attachments.Verified by hand on a physical Samsung SM-S948B (Android 16), driving a synthetic uncaught exception to bring the screen up. The report renders; Copy places it on the clipboard with no Android 13+ content preview, confirming
EXTRA_IS_SENSITIVEtakes effect; Share offers the full target list rather than mail only, and Gmail receives both the inline body and thecrash_report.txtattachment; the crash screen survives the share, so Dismiss stays the only thing that ends the process. Repeated with "Include Logcat" ticked. Theadbquery output above is from the same device, and the Copy button is also how the crash reports behind #35 were retrieved.Build
assembleStandardReleaseandassembleFossReleaseboth build clean on all three commits.Known issues, not addressed here
barcodeScanneris never closed.QRScreen.startCamera()builds a freshAnalyzeron every invocation, so nativeBarcodeScannerinstances accumulate for the process lifetime. Fixing it needs a teardown hook throughQRCodeAnalyzerandQRScreen, i.e. a new API rather than a bug fix — happy to follow up if you want it.Text. Compose text does not virtualise, so ticking "Include Logcat" lays out the entire string on the main thread. Much reduced now the logcat is bounded, but not free.