Skip to content

[FIAM] Serialize display callback state and impression reservation - #16763

Open
dajiaohuang wants to merge 4 commits into
firebase:mainfrom
dajiaohuang:fix/fiam-display-callback-state
Open

dajiaohuang wants to merge 4 commits into
firebase:mainfrom
dajiaohuang:fix/fiam-display-callback-state

Conversation

@dajiaohuang

@dajiaohuang dajiaohuang commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

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.

@gemini-code-assist

Copy link
Copy Markdown
Contributor
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

@paulb777

paulb777 commented Oct 1, 2026

Copy link
Copy Markdown
Member

Thanks for the contribution! A few housekeeping items:

  • CHANGELOG: The entry in FirebaseInAppMessaging/CHANGELOG.md is under # 13.0.0, which has already shipped. Please move it under a # Unreleased heading at the top of the file (add the heading if it isn't there yet).
  • PR number: end the entry with this PR's number, e.g. - [fixed] Description. (#16763).
  • Formatting: after updating, please run scripts/style.sh so the infra.check job passes. It needs clang-format 23 and swiftformat (see scripts/setup_check.sh). If you can't run it locally, the infra.check log lists the files that need formatting.

@paulb777 paulb777 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this. There are really two fixes here:

  1. Releasing the display gate when displayMessageWithMessageDefinition returns nil. This is a real, deterministic bug: once it happens, isMsgBeingDisplayed stays YES and no in-app message is shown again until the app relaunches.
  2. 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?

Comment thread FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m
Comment thread FirebaseInAppMessaging/Tests/Unit/FIRIAMDisplayExecutorTests.m Outdated
Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m Outdated
Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m Outdated
Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m Outdated
@dajiaohuang

Copy link
Copy Markdown
Contributor Author

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.

@paulb777

paulb777 commented Oct 2, 2026

Copy link
Copy Markdown
Member

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
Comment thread FirebaseInAppMessaging/Sources/Flows/FIRIAMDisplayExecutor.m
@paulb777 paulb777 added the api: inappmessaging Firebase In App Messaging label Oct 2, 2026
@dajiaohuang

Copy link
Copy Markdown
Contributor Author

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.

@paulb777 paulb777 added this to the 13.1.0 - M188 milestone Oct 3, 2026
@paulb777 paulb777 closed this Oct 3, 2026
@paulb777 paulb777 reopened this Oct 3, 2026
@paulb777

paulb777 commented Oct 3, 2026

Copy link
Copy Markdown
Member

Closed and reopened to reset the CLA check

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: inappmessaging Firebase In App Messaging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants