BIT-7817: Skeleton crash reports for native crashes on API level 30#934
BIT-7817: Skeleton crash reports for native crashes on API level 30#934
Conversation
…on API level 30 Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/1eea7ae0-62e8-43c2-92b3-c17d8583429c Co-authored-by: murki <216735+murki@users.noreply.github.com>
…port call Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/1eea7ae0-62e8-43c2-92b3-c17d8583429c Co-authored-by: murki <216735+murki@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/1eea7ae0-62e8-43c2-92b3-c17d8583429c Co-authored-by: murki <216735+murki@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds support for generating “skeleton” native-crash reports on Android API 30 when ApplicationExitInfo.getTraceInputStream() returns null, avoiding silently dropping those crashes.
Changes:
- Make
processAppExitReport(..., traceInputStream)nullable throughIIssueReporterProcessor/IssueReporterProcessor. - Update
IssueReporterto always invoke.use {}on the (nullable) trace stream and forward it to the processor. - Add
NativeCrashProcessorfallback to build a minimal native-crashReportwhen tombstone data is unavailable, plus a regression test and changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/IssueReporter.kt | Routes app-exit processing through a single .use {} path that can forward a null stream. |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/IIssueReporterProcessor.kt | Updates API contract to accept a nullable trace stream (defaulting to null). |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/IssueReporterProcessor.kt | Propagates nullable stream into native crash processing and ANR processing path. |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/NativeCrashProcessor.kt | Generates a skeleton native crash report when tombstone stream is null. |
| platform/jvm/capture/src/test/kotlin/io/bitdrift/capture/reports/processor/IssueReporterProcessorTest.kt | Adds test coverage for the new skeleton-report behavior. |
| CHANGELOG.md | Documents the API-30 native-crash reporting fix. |
Comments suppressed due to low confidence (1)
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/IssueReporterProcessor.kt:117
traceInputStreamis now nullable, but the ANR branch passes it directly toprocessAndPersistANR(stream: InputStream, ...), which requires a non-null stream. This is a compile-time type mismatch (and would also be a runtime issue if forced). Consider requiring a non-null stream whenfatalIssueType == ReportType.AppNotResponding(e.g.,val stream = requireNotNull(traceInputStream)with a clear error/log + early return) while keeping null allowed for native crashes.
traceInputStream: InputStream?,
) {
runCatching {
if (fatalIssueType == ReportType.AppNotResponding) {
streamingReportsProcessor.processAndPersistANR(
traceInputStream,
timestamp,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/IIssueReporterProcessor.kt
Outdated
Show resolved
Hide resolved
...rm/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/NativeCrashProcessor.kt
Outdated
Show resolved
Hide resolved
...rm/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/NativeCrashProcessor.kt
Outdated
Show resolved
Hide resolved
…tionExitInfo.status Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/cccb3fef-a897-4459-805c-cb471c9510c9 Co-authored-by: murki <216735+murki@users.noreply.github.com>
Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/cccb3fef-a897-4459-805c-cb471c9510c9 Co-authored-by: murki <216735+murki@users.noreply.github.com>
Size Comparison Report (x86_64)
|
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
...rm/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/NativeCrashProcessor.kt
Show resolved
Hide resolved
…nalInfo structure Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/50db0348-eb7c-4ee9-a316-0e58b0a96762 Co-authored-by: murki <216735+murki@users.noreply.github.com>
.../jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/IssueReporterProcessor.kt
Outdated
Show resolved
Hide resolved
…cessing Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/bfe00ca6-7591-401c-8c28-e20dafe46f6d Co-authored-by: murki <216735+murki@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
…erProcessor Agent-Logs-Url: https://github.com/bitdriftlabs/capture-sdk/sessions/a4bcefe9-ab00-4b1c-8eae-f00348a727cd Co-authored-by: murki <216735+murki@users.noreply.github.com>
Removed the Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/reports/processor/NativeCrashProcessor.kt:206
descriptionis nullable, butsignalName.ifEmpty { description }requires a non-null fallback string. As written, this won’t compile (or would risk passing null intocreateString). Use a non-null default (e.g.,description ?: "Native crash") for the empty-signal case so the error name/reason is always a valid string.
tombstone: Tombstone,
frameOffsets: IntArray,
): Int {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| processor.processAppExitReport( | ||
| ReportType.NativeCrash, | ||
| FAKE_TIME_STAMP, | ||
| description, | ||
| traceInputStream = null, | ||
| signalNumber = 11, // SIGSEGV | ||
| ) |
There was a problem hiding this comment.
Because IssueReporterProcessor.processAppExitReport(...) now requires signalNumber (no default on the concrete override), several existing calls in this same test file still invoke it with only 4 arguments (e.g., the ANR and existing native-crash tests). Those call sites will no longer compile unless you either (a) update them to pass signalNumber = 0, or (b) add a default value for signalNumber on the override as well (keeping in mind defaults are resolved by the static type).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| description: String?, | ||
| traceInputStream: InputStream, | ||
| traceInputStream: InputStream?, | ||
| signalNumber: Int, |
There was a problem hiding this comment.
signalNumber has no default value in the concrete IssueReporterProcessor.processAppExitReport override, but several call sites (including existing tests in this module) invoke processAppExitReport(...) without providing it. In Kotlin, default parameters from the interface are only applied when calling through the interface type, so these calls will fail to compile when the receiver is IssueReporterProcessor. Consider giving signalNumber a default of 0 in the override as well (or update all call sites to pass an explicit value).
| signalNumber: Int, | |
| signalNumber: Int = 0, |
| latestAppExitInfoProvider.convertExitReasonToFbsReportType(lastReason.reason)?.let { fatalIssueType -> | ||
| lastReason.traceInputStream.use { traceInputStream -> | ||
| issueReporterProcessor?.processAppExitReport( | ||
| fatalIssueType = fatalIssueType, | ||
| timestamp = lastReason.timestamp, | ||
| description = lastReason.description, | ||
| traceInputStream = it, | ||
| traceInputStream = traceInputStream, | ||
| signalNumber = lastReason.status, | ||
| ) | ||
| } |
There was a problem hiding this comment.
traceInputStream is only wrapped in .use {} after convertExitReasonToFbsReportType(...) succeeds. If the exit reason maps to null but traceInputStream is non-null, the stream will never be closed. To avoid leaking the underlying FD/stream, wrap the .use {} around the stream unconditionally (and then decide inside the block whether to call processAppExitReport).
| val signalName = tombstone.signalInfo.name | ||
| val reason = builder.createString(signalName.ifEmpty { description }) | ||
| val causeText = | ||
| tombstone.causesList.firstOrNull()?.humanReadable ?: tombstone.abortMessage.ifEmpty { | ||
| signalDescriptions[signalName] ?: "Native crash" | ||
| signalsByName[signalName]?.description ?: "Native crash" | ||
| } |
There was a problem hiding this comment.
createErrorOffset can pass null into FlatBufferBuilder.createString(...) when tombstone.signalInfo.name is empty and description is null (signalName.ifEmpty { description }). Because createString is a Java API (platform type), this compiles but can throw at runtime and cause the native crash report to be dropped. Ensure a non-null fallback string is always used (e.g., fall back to "Native crash" when both are missing).
| **Fixed** | ||
|
|
||
| - Fatal Issue Reports are now being sent on API level 30. Crash reports will have basic metadata but no stack traces. | ||
|
|
There was a problem hiding this comment.
Changelog entry says "Fatal Issue Reports are now being sent on API level 30", but the PR changes appear specific to native crashes where ApplicationExitInfo.getTraceInputStream() is null (skeleton native-crash reports). Consider rewording to avoid implying all fatal issue reports were previously not sent on API 30, and to call out that these are native crash reports without tombstone/stack traces.
Android Benchmark Results
Allocations
Timing
|
Adds support for generating "skeleton" native-crash reports on Android API 30 when
ApplicationExitInfo.getTraceInputStream()returnsnull, avoiding silently dropping those crashes.Test report on Android OS 11 in staging: https://explorations.bitdrift.dev/issues/2274956664977278374/2a26f7f0-c01c-4887-a17c-817fcee6dd50
Changes
processAppExitReport(..., traceInputStream)nullable throughIIssueReporterProcessor/IssueReporterProcessor, removing the= nulldefault so callers must pass the stream explicitly.IssueReporterto always invoke.use {}on the (nullable) trace stream and forward it to the processor.NativeCrashProcessorfallback to build a minimal native-crashReportwhen tombstone data is unavailable.SignalInfodata class inNativeCrashProcessorwith a singlesignalsmap (keyed by signal number) to resolve signal numbers (fromApplicationExitInfo.getStatus()) to signal names (e.g.,SIGSEGV) and descriptions (e.g., "Segmentation violation (invalid memory reference)") for skeleton report errorname/reasonfields. A derivedsignalsByNamemap supports lookups from tombstone data.signalNumberparameter throughIssueReporter→processAppExitReport→NativeCrashProcessor.createSkeletonReport.description/"Native crash" when the signal number is unknown.traceInputStreamis null, instead of using a non-null assertion.CHANGELOG.md's "Unreleased" section has been updated, if applicable.Original prompt