fix: preserve tombstone stack traces in Crashlytics - #3071
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📜 Recent review details🔇 Additional comments (2)
📝 WalkthroughWalkthroughCrash report handling now reconstructs JVM and Go stack traces from report content. It extracts failure headers, preserves parsed frames in ChangesCrash report reconstruction
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change preserves JVM and Go stack traces when saved tombstones are uploaded to Crashlytics without altering the existing upload flow. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant FirebaseUpload as sendFileToFirebase
participant ExceptionBuilder as buildReportException
participant CrashParsers as JVM and Go parsers
FirebaseUpload->>ExceptionBuilder: Pass report filename and content
ExceptionBuilder->>CrashParsers: Select parser and extract failure header
CrashParsers-->>ExceptionBuilder: Return message and StackTraceElement frames
ExceptionBuilder-->>FirebaseUpload: Return labeled RuntimeException
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The changes improve Crashlytics stack-trace reconstruction, but they do not investigate or fix the SQLiteFullException, address the Room/WorkManager failure path, or prevent that exception from crashing the application as required by issue Resolution Add or link implementation changes that address the SQLITE_FULL failure in RoomDatabase.endTransaction and the related WorkManager scheduling path. Ensure the exception no longer causes an application crash, or link the correct issue for this Crashlytics reporting work. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Reconstructs real stack traces for Crashlytics reports instead of dumping raw text: buildReportException now parses JVM frames from Kotlin tombstones and function/source-location pairs from Go crashes (including created by goroutines), sets them as the exception's stackTrace, and uses the detected exception/panic header as the message. Falls back to the previous bounded content preview (capped at MAX_EXCEPTION_PREVIEW_CHARS) when no header is found or the format is malformed, and extracts the report-type classification into a shared reportType helper. Adds EnhancedBugReportTest covering frame parsing, malformed-frame handling, and the fallback path.
No blocking issues surfaced. 4 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 65 functions depend on the 31 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 65 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 53 function(s) in the blast radius were not formally verified this run
|
@mvanhorn Thanks a lot for the pull request. Merging! 💯 |
3 participants
Saved tombstones are currently uploaded by
EnhancedBugReport.sendFileToFirebaseas text inside a newly createdRuntimeException. Crashlytics therefore attributes the event to the uploader and obscures the JVM or Go frames contained in the file, as seen in the reportedSQLiteFullException. Kotlin tombstones also prepend timestamp, token, and diagnostic headers before the exception text, while Go dumps pair function lines with subsequent.go:<line>locations. The supplied issue evidence contains no claim, competing PR, or closed-unmerged prior attempt.A Kotlin tombstone containing timestamp/token headers and the issue's
SQLiteFullExceptiontrace produces a report exception whose message identifies the saved failure and whose first frames point toSQLiteConnection,RoomDatabase, and the original source line rather thanEnhancedBugReport.sendFileToFirebase.Summary
Replace the message-only synthetic exception construction in
EnhancedBugReportwith a production-used builder that derives a concise exception message and assigns parsedStackTraceElemententries from the tombstone content before callingFirebaseErrorReporting.recordException. For Kotlin/JVM tombstones, locate the exception header and parse standardat package.Class.method(File.kt:line)frames despite the metadata prefix; for Go crash content, associate function records with their following.gofile/line records, includingcreated bysections. Keep the existing 64 KiB read cap, Crashlytics log chunking, file retry/deletion semantics, and a safe fallback for malformed or non-stacktrace log files so reporting never drops an otherwise uploadable tombstone.Fixes #2990
Summary by CodeRabbit
Bug Fixes
Tests