Skip to content

fix: [SDK-4852] harden iOS click listener registration to match Android pattern#883

Merged
sherwinski merged 1 commit into
mainfrom
sherwin/sdk-4852
Jul 8, 2026
Merged

fix: [SDK-4852] harden iOS click listener registration to match Android pattern#883
sherwinski merged 1 commit into
mainfrom
sherwin/sdk-4852

Conversation

@sherwinski

@sherwinski sherwinski commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Description

One Line Summary

Apply the same click-listener registration safeguards added to the Android bridge in #881 to the iOS bridge, so both platforms share the identical pattern.

Details

Motivation

#881 hardened the Android Clicked event accessor based on review feedback. The iOS bridge (iOSNotificationsManager) already used lazy native registration (which is why iOS never had the cold-start click drop), but had none of the safeguards:

  1. The add/remove accessors were unsynchronized, so two threads subscribing concurrently could both pass the _clickNativeListenerSet check and register the native callback twice.
  2. _clickNativeListenerSet = true was set before the native call, so a transient native-call failure left the flag stuck true with no listener registered — clicks silently lost for the app's lifetime with no retry.
  3. A failed native call left the managed handler subscribed, so a caller retrying the subscription would add the same handler twice.

This PR applies the identical fixes: a private lock around both accessors, the flag set only after the native call returns, and a rollback (_clicked -= value) before rethrowing on failure.

It also adds the null-handler guard in _onClicked that the Android bridge already has — without it, a click arriving after all handlers are removed (the native listener is never unregistered) would throw a NullReferenceException on the Unity main thread.

Scope

iOS only, notification click events only. No public API changes and no behavior change on any success path — the lazy registration timing is exactly as before. Android is untouched (already done in #881).

Testing

Unit testing

None added — the repo has no unit test infrastructure; this mirrors the reviewed and manually verified Android change from #881.

Manual testing

Verified with the demo app built from this branch (Unity 6000.3.6f1, IL2CPP) on an iPhone 16 Pro simulator (iOS 18.1), using xcrun simctl push with a OneSignal-format payload containing additional data:

  • App initializes normally; the first Clicked += subscription registers the native callback through the new locked accessor without issues.
  • Foreground receive path unchanged: C# ForegroundWillDisplay fired for each pushed notification and the notification displayed.
  • Click path verified: tapping a pushed notification (app backgrounded) fired the C# Clicked handler, and the native SDK recorded the notification as a DIRECT influence.

The failure-path changes (concurrent first subscription, native call throwing) are not reachable in a normal run; correctness argued by symmetry with the reviewed Android implementation from #881.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

@sherwinski sherwinski requested a review from a team as a code owner July 8, 2026 20:43
@sherwinski

Copy link
Copy Markdown
Contributor Author

@claude review once

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM — mechanical mirror of the reviewed Android hardening from #881.

Extended reasoning...

Overview

Single-file change to com.onesignal.unity.ios/Runtime/iOSNotificationsManager.cs that applies the same three safeguards to the iOS Clicked event accessor that PR #881 applied to the Android bridge: a _clickRegistrationLock around both add/remove, setting _clickNativeListenerSet = true only after the native P/Invoke returns successfully, a try/catch rollback of _clicked -= value on native-call failure, and a null-handler guard in _onClicked before dispatching to the Unity main thread.

Security risks

None. This is a thread-safety and error-handling hardening on the notification click-event registration path. No auth, crypto, permissions, network I/O, or data-handling code is touched. The public API surface is unchanged.

Level of scrutiny

Low. The change is a line-for-line port of an already-reviewed and merged pattern (confirmed by grepping AndroidNotificationsManager.cs — structure, lock placement, rollback semantics, and null-guard all match). Success-path timing of lazy registration is unchanged, and the failure paths being hardened (concurrent first subscription, native call throwing, click arriving after unsubscribe) are not reachable in normal operation.

Other factors

The bug hunting system found no issues. The comment explaining the lazy-registration rationale is preserved. Manual testing is called out as in-progress in the PR description, but given the symmetry with a manually verified Android implementation and the lack of any behavioral change on the success path, this does not warrant blocking on human review.

@sherwinski sherwinski requested review from fadi-george and nan-li July 8, 2026 21:08
@sherwinski sherwinski merged commit 0dbe281 into main Jul 8, 2026
5 checks passed
@sherwinski sherwinski deleted the sherwin/sdk-4852 branch July 8, 2026 21:21
This was referenced Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants