fix: Crashlytics defer API calls until async context initialization completes - #15879
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively resolves a race condition where Crashlytics API calls could be silently dropped if made immediately after initialization. The approach of deferring calls using a promise until the asynchronous initialization is complete is solid, and the changes correctly wrap several public API methods. While the fix is good, it appears that some other public methods that could also be affected by this race condition might have been missed. Methods like recordError:userInfo:, recordExceptionModel:, and recordOnDemandExceptionModel: also seem to perform actions that depend on the context being initialized, but they are not wrapped. To ensure a comprehensive fix, I recommend reviewing these methods and wrapping them with waitForContextInit: as well. On a minor note, the added warning log and the typo fix are good improvements.
Updated code to defer execution of |
ncooke3
left a comment
ncooke3
left a comment
There was a problem hiding this comment.
Nice fix!
One possible nit, if multiple key/value pairs are expected, then the logs should read Failed to [read|write] key/value pairs
Co-authored-by: Nick Cooke <36927374+ncooke3@users.noreply.github.com>
|
Failing tests are known flakes. |
Firebase 12.11.0 (firebase/firebase-ios-sdk#15879) made the record/log APIs defer their work onto an internal context-init promise instead of running synchronously. `changeExistingHangReport` recorded a non-fatal hang via `record(onDemandExceptionModel:)` and then synchronously removed the "previously-crashed" marker - but the deferred record now (re)writes that marker *after* our removal ran, so the marker survived and the next launch mis-reported a phantom app crash. Expose the private `waitForContextInit:callback:` helper in our category and chain the marker removal on the same promise, after the record call. FBLPromise invokes observers in registration order on the main queue and the record's marker write is synchronous within its own observer, so our removal is guaranteed to run after the marker has been (re)written. Also fix a latent ABI mismatch: `FIRCLSExceptionRecordOnDemandModel` and `FIRCLSExceptionRecordOnDemand` each gained a trailing `BOOL shouldSuspendThread` argument in Firebase 12.x. Update the declarations and call sites to pass `true` (Firebase's own default). Require FirebaseCrashlytics >= 12.13.0 (< 13.0.0) in Package.swift and the podspec, since the fix depends on these 12.x behaviors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Firebase 12.11.0 (firebase/firebase-ios-sdk#15879) made the record/log APIs defer their work onto an internal context-init promise instead of running synchronously. `changeExistingHangReport` recorded a non-fatal hang via `record(onDemandExceptionModel:)` and then synchronously removed the "previously-crashed" marker - but the deferred record now (re)writes that marker *after* our removal ran, so the marker survived and the next launch mis-reported a phantom app crash. Expose the private `waitForContextInit:callback:` helper in our category and chain the marker removal on the same promise, after the record call. FBLPromise invokes observers in registration order on the main queue and the record's marker write is synchronous within its own observer, so our removal is guaranteed to run after the marker has been (re)written. Also fix a latent ABI mismatch: `FIRCLSExceptionRecordOnDemandModel` and `FIRCLSExceptionRecordOnDemand` each gained a trailing `BOOL shouldSuspendThread` argument in Firebase 12.x. Update the declarations and call sites to pass `true` (Firebase's own default). Require FirebaseCrashlytics >= 12.13.0 (< 13.0.0) in Package.swift and the podspec, since the fix depends on these 12.x behaviors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Firebase 12.11.0 (firebase/firebase-ios-sdk#15879) made the record/log APIs defer their work onto an internal context-init promise instead of running synchronously. `changeExistingHangReport` recorded a non-fatal hang via `record(onDemandExceptionModel:)` and then synchronously removed the "previously-crashed" marker - but the deferred record now (re)writes that marker *after* our removal ran, so the marker survived and the next launch mis-reported a phantom app crash. Expose the private `waitForContextInit:callback:` helper in our category and chain the marker removal on the same promise, after the record call. FBLPromise invokes observers in registration order on the main queue and the record's marker write is synchronous within its own observer, so our removal is guaranteed to run after the marker has been (re)written. Also fix a latent ABI mismatch: `FIRCLSExceptionRecordOnDemandModel` and `FIRCLSExceptionRecordOnDemand` each gained a trailing `BOOL shouldSuspendThread` argument in Firebase 12.x. Update the declarations and call sites to pass `true` (Firebase's own default). Require FirebaseCrashlytics >= 12.13.0 (< 13.0.0) in Package.swift and the podspec, since the fix depends on these 12.x behaviors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Firebase 12.11.0 (firebase/firebase-ios-sdk#15879) made the record/log APIs defer their work onto an internal context-init promise instead of running synchronously. `changeExistingHangReport` recorded a non-fatal hang via `record(onDemandExceptionModel:)` and then synchronously removed the "previously-crashed" marker - but the deferred record now (re)writes that marker *after* our removal ran, so the marker survived and the next launch mis-reported a phantom app crash. Expose the private `waitForContextInit:callback:` helper in our category and chain the marker removal on the same promise, after the record call. FBLPromise invokes observers in registration order on the main queue and the record's marker write is synchronous within its own observer, so our removal is guaranteed to run after the marker has been (re)written. The removal closure captures `self` strongly so it still runs if the caller releases the reporter before the promise resolves; it is owned by the one-shot promise observer, so there is no retain cycle. Also fix a latent ABI mismatch: `FIRCLSExceptionRecordOnDemandModel` and `FIRCLSExceptionRecordOnDemand` each gained a trailing `BOOL shouldSuspendThread` argument in Firebase 11.9.0. Update the declarations and call sites to pass `true` (Firebase's own default). Require FirebaseCrashlytics >= 12.11.0 (< 13.0.0) in Package.swift and the podspec - 12.11.0 is the release where `recordOnDemandExceptionModel:` became deferred (and `waitForContextInit:` wraps it), which is the behavior this fix depends on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Bump FirebaseCrashlytics to 12.15.0 (newest < 13) Pin the test app to the latest 12.x Firebase to validate PerfSuite against it. This is expected to make the recovered-non-fatal-hang tests RED: 12.x defers recordOnDemandExceptionModel behind a context-init promise, so the previously-crashed marker is written after changeExistingHangReport's synchronous removeFirebaseCrashMarker() runs - the marker survives and the next launch would mis-report the recovered hang as a crash. The follow-up commit fixes changeExistingHangReport to record synchronously. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Remove FirebaseCrashlytics pod Removed FirebaseCrashlytics pod from the Podfile. * Wait for Crashlytics context init before clearing crash marker Firebase 12.11.0 (firebase/firebase-ios-sdk#15879) made the record/log APIs defer their work onto an internal context-init promise instead of running synchronously. `changeExistingHangReport` recorded a non-fatal hang via `record(onDemandExceptionModel:)` and then synchronously removed the "previously-crashed" marker - but the deferred record now (re)writes that marker *after* our removal ran, so the marker survived and the next launch mis-reported a phantom app crash. Expose the private `waitForContextInit:callback:` helper in our category and chain the marker removal on the same promise, after the record call. FBLPromise invokes observers in registration order on the main queue and the record's marker write is synchronous within its own observer, so our removal is guaranteed to run after the marker has been (re)written. The removal closure captures `self` strongly so it still runs if the caller releases the reporter before the promise resolves; it is owned by the one-shot promise observer, so there is no retain cycle. Also fix a latent ABI mismatch: `FIRCLSExceptionRecordOnDemandModel` and `FIRCLSExceptionRecordOnDemand` each gained a trailing `BOOL shouldSuspendThread` argument in Firebase 11.9.0. Update the declarations and call sites to pass `true` (Firebase's own default). Require FirebaseCrashlytics >= 12.11.0 (< 13.0.0) in Package.swift and the podspec - 12.11.0 is the release where `recordOnDemandExceptionModel:` became deferred (and `waitForContextInit:` wraps it), which is the behavior this fix depends on. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Gleb Tarasov <gleb.tarasov@booking.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 participants
Fixes a silent failure where Crashlytics API calls are dropped when invoked immediately after Firebase initialization.
Explanation
Recently, Crashlytics transitioned to asynchronous initialization to improve overall app startup performance. Because of this change, when
FirebaseApp.configure()finishes executing on the main thread, the Crashlytics SDK may still be actively initializing its core context on a background thread.If a developer calls a Crashlytics API method (such as
setUserID:) right after configuring Firebase, the method executes while the Crashlytics context is not yet fully ready. Previously, this resulted in the SDK simply dropping the request. Because there were no logs emitted when this occurred, it created a silent failure.Approach
To resolve this race condition, the affected public API methods were updated to wrap their internal logic inside the existing
waitForContextInit:callback:private helper.Instead of executing synchronously and failing, the API calls now check the
_contextInitPromise. If the background initialization is still in progress, the API execution block is chained to the promise and safely deferred until the setup completes.