Repository navigation
[FIAM] Serialize display callback state and impression reservation - #16763
dajiaohuang wants to merge 4 commits into
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 the 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 counterproductive. 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. |
|
Thanks for the contribution! A few housekeeping items:
|
paulb777
left a comment
There was a problem hiding this comment.
Thanks for digging into this. There are really two fixes here:
- Releasing the display gate when
displayMessageWithMessageDefinitionreturns nil. This is a real, deterministic bug: once it happens,isMsgBeingDisplayedstaysYESand no in-app message is shown again until the app relaunches. - Serializing the delegate callbacks with the display checks. The race is real but very narrow, and taking
@synchronized(self)on the main thread adds some risk (see inline).
Would you be willing to split (1) into its own PR with a test? That part could land quickly while we work through (2).
CI confirms the new test fails (FIRIAMDisplayExecutorTests.m:1169, :1170 and :1176 in the tvOS pod-lib-lint job). The cause is inline.
One more gap: self.impressionRecorded = NO (L704) is still written outside the lock, in the image-load block. Could it move into displayForMessage:, next to _currentMsgBeingDisplayed and isMsgBeingDisplayed, so the new message's state is set up under the same lock?
|
Split the nil-construction fix into #16819 with a card-without-images / next-message regression and removal of the unusable cached message. That return-path change is removed from this PR. For the callback work, state now uses a separate private monitor that is released before invoking display components, app delegates, or bookkeeping. New-message initialization resets impression state under that monitor. Callback state is matched by campaign ID, and the reentrant test uses timestamp 2000 plus direct cache replacement to avoid both earlier OCMock stubs. It also covers a late previous-message dismissal. Added a bounded worker/main callback test for a custom display component waiting on main. Moved the changelog to Unreleased/#16763 and formatted with clang-format 23.1.2. Apple XCTest is unrun locally, so I have not yet confirmed the requested native before/after result. Native CI execution is still needed. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request serializes display completion and impression reservation in FIRIAMDisplayExecutor using a new callback state lock to prevent overlapping triggers from losing impression tracking. It also adds unit tests to verify reentrant display behavior and main thread callbacks. The review feedback suggests guarding against a potential nil pointer crash in currentMessageForDelegateCallbackForMessageID and directly accessing instance variables instead of properties within synchronized blocks to avoid recursive locking overhead.
|
The requested native main-fails / PR-passes comparison now ran on macOS26/Xcode26.4.1 and an actual iOS simulator: https://github.com/dajiaohuang/firebase-ios-sdk/actions/runs/37091567286 . Using the PR tests with the main executor at8d7687f915e31b84ec3dfc816034c3fc571be5e6, the reentrant-dismissal case fails because the next message loses its display state, consumes its impression reservation and never records m4; these are three expected assertion failures. The callback/main-thread control passes. With the exact PR executor at0dff8aafde63b978fa240834d4f62fa28363f1a4 restored, all31 executor XCTest cases pass, including both new regressions. The comparison commit c653f86 adds only a separate workflow on my fork and does not change production or test sources. The run retains before/after logs and xcresult bundles. This verifies the executor unit behavior on the simulator; full FIAM integration/device behavior and the pending CLA remain separate. |
|
Closed and reopened to reset the CLA check |
Delegate callbacks previously cleared the display flag and reserved impressions separately from new-message initialization. A bookkeeping or app-delegate re-entry could start the next message and then overwrite its impression state.
Use a private monitor for the current message, display flag, and impression reservation. Initialize all three before loading images; snapshot/reserve callback state before invoking app delegates or bookkeeping, and match the callback's campaign ID before changing state. The monitor is released before calling display components or delegates. Image-load errors use the originating definition's ID even when constructing an error display object returns nil.
The reentrant test now uses timestamp 2000 and removes the previous message from the cache directly, avoiding both earlier OCMock stubs. It also checks that a late dismissal cannot end the new display or consume its impression. A bounded worker/main test checks that a custom display component can wait for a main-thread callback without the callback taking the executor monitor.
The independent nil-construction gate/cache fix and card-without-images regression are now in #16819, as requested. This PR no longer changes the final nil-construction return path. Changelog is under Unreleased with #16763.
Validation: clang-format 23.1.2 and git diff --check pass. The upstream exact-head FIAM workflow succeeds for CocoaPods/SPM/sample builds. Separate real Apple XCTest comparison also succeeds: https://github.com/dajiaohuang/firebase-ios-sdk/actions/runs/37091567286 . On macOS26/Xcode26.4.1 with a real iOS simulator, the same PR tests against main executor 8d7687f reproduce three assertions in the reentrant-dismissal test (2 tests executed; the main-thread callback control passes). Restoring the exact PR executor 0dff8aa passes all 31 FIRIAMDisplayExecutorTests with 0 failures, including both new tests. The fork comparison commit c653f86 adds only its validation workflow; production and test sources match the PR. Logs and both xcresult bundles are retained as run artifacts. This is executor-unit/simulator evidence, not full FIAM integration or device acceptance. CLA remains pending; no full PR-check pass is claimed.
Current-main integration
Merged current main c276eca normally at 4c7eb04 after #16819 landed. Retained the callback regressions and the upstream invalid-card regression as three separate, exact-parent test methods, plus both changelog entries. The merged executor retains the nil-construction gate release/cache removal from main. git diff --check and exact-parent method comparisons pass. The historical 31-test callback and 30-test invalid-card runs are separate component evidence; the combined executor/test fixture has not been executed locally. Xcode/Apple SDKs and formatter tools are unavailable on this Windows host; native combined-suite validation and CLA remain required.