fix(android): [SDK-4407] deliver notification events when firebase_messaging is present (#1138)#1152
Conversation
|
@claude review |
|
@claude review |
613d864 to
f2c3f25
Compare
|
@claude review |
|
Would like to wait on OneSignal/OneSignal-Android-SDK#2655 |
…stence demo_fm is a copy of the demo app with firebase_core + firebase_messaging added and FCM listeners registered before OneSignal, matching the affected users' setup in issue #1138. Initializing Firebase registers a FirebaseMessagingService in the manifest, which is what triggers the FCM hijack + click-listener routing problems the fix addresses. Also gitignores google-services.json / GoogleService-Info.plist so per-dev Firebase config is not committed. Co-authored-by: Cursor <cursoragent@cursor.com>
42b31de to
f4acc38
Compare
55219da to
0a45f87
Compare
|
@claude review |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
ios/onesignal_flutter/Sources/onesignal_flutter/OSFlutterNotifications.m:241-249— 🟡 nit:displayNotification:(OSFlutterNotifications.m:233-242) still callsonesignalLog:ONE_S_LL_ERROR+ returns withoutresult(nil)when the cache is empty, while the symmetricproceedWithWillDisplay:path (lines 188-194) was correctly updated to handle the new post-eviction state silently. With the new eviction at lines 244-246, apps that register multipleaddForegroundWillDisplayListenercallbacks that each callevent.notification.display()(a common modular-app pattern — analytics module + custom-UI module — and the same pattern this PR's own demos use with a single listener) now log N-1 misleadingONE_S_LL_ERRORlines per foreground push and leak one Flutter result callback per error. Functional behavior is correct (single display via OS-level idempotency); fix is mechanical — mirror the sibling fix: return silently withresult(nil)when the cache is already consumed.Extended reasoning...
What the bug is
This PR added cache eviction inside
displayNotification:at OSFlutterNotifications.m:244-246:[event.notification display]; // Manual display ends this notification's lifecycle; drop both cache entries. [self.onWillDisplayEventCache removeObjectForKey:notificationId]; [self.preventedDefaultCache removeObjectForKey:notificationId]; result(nil);
The eviction is correct in isolation — pre-PR
[event.notification display]could be invoked twice for the same id (once here by an explicit listener call, then again byproceedWithWillDisplay:), so removing the cache entry eliminates that double-dispatch. The PR also correctly updated the sibling handlerproceedWithWillDisplay:(lines 188-194) to recognize the now-expected post-eviction empty-cache state and return silently:if (!event) { // Already consumed for this id, typically because a listener called // notification.display() (which displays and evicts) before this automatic // proceed ran. Nothing left to do. result(nil); return; }
But the matching update was not applied to
displayNotification:'s own!eventbranch (lines 233-242), which is unchanged from pre-PR and still callsonesignalLog:ONE_S_LL_ERRORand returns withoutresult(nil):if (!event) { [OneSignalLog onesignalLog:ONE_S_LL_ERROR message:[NSString stringWithFormat: @"OneSignal (objc): could not find notification " @"will display event for notification with id: %@", notificationId]]; return; }
Why this fires now and didn't pre-PR
Pre-PR the cache was never evicted by
displayNotification:, so multipledisplay()calls for the same id all hit the cached event and re-invoked[event.notification display](idempotent at the OneSignal-iOS SDK level — the OS completion handler can only fire once). The!eventbranch only triggered for ids that were neverwillDisplay-d. Post-PR, the firstdisplay()consumes the cache, so every subsequentdisplay()for the same id hits!event.Step-by-step proof
Dart-side trigger is verified in lib/src/notifications.dart:155-163.
_handleMethodforOneSignal#onWillDisplayNotificationiterates_willDisplayListenerssynchronously, then unconditionally invokesOneSignal#proceedWithWillDisplay. Each listener callingevent.notification.display()enqueuesOneSignal#displayNotificationfire-and-forget on the platform channel (lib/src/notifications.dart:193-196). With N display-calling listeners, iOS receives N x displayNotification then 1 x proceedWithWillDisplay in FIFO order — all sharing the same notificationId.Concrete scenario with the canonical modular pattern:
OneSignal.Notifications.addForegroundWillDisplayListener((event) { analyticsModule.logForegroundPush(event); event.notification.display(); }); OneSignal.Notifications.addForegroundWillDisplayListener((event) { customUiModule.augment(event); event.notification.display(); });
- Foreground push arrives. iOS
onWillDisplayNotification:(lines 144-172) caches the event and dispatchesOneSignal#onWillDisplayNotificationto Dart. - Dart loop fires both listeners. Each calls
event.notification.display()→ enqueuesOneSignal#displayNotificationon the platform channel. Loop ends → unconditionally enqueuesOneSignal#proceedWithWillDisplay. - Msg 1 (displayNotification): cache lookup at line 230 finds the event →
[event.notification display]→ evicts both cache entries (lines 245-246, NEW in this PR) →result(nil). ✓ - Msg 2 (displayNotification): cache lookup returns nil → hits
!eventbranch at lines 233-241 → logsONE_S_LL_ERROR: could not find notification will display event for notification with id: <UUID>→ returns at line 241 without callingresult(nil). - Msg 3 (proceedWithWillDisplay): cache lookup returns nil → hits the FIXED
!eventbranch at lines 188-194 → returns silently withresult(nil). ✓
Visible result: notification displays exactly once (correct functional behavior), but every foreground push with N display-calling listeners emits N-1 misleading
ONE_S_LL_ERRORlog lines and leaks N-1 Flutter result callbacks.Impact
ONE_S_LL_ERRORis at the default OneSignal log level in production. Apps reading their logs (and especially SDK consumers debugging issue reports) will see "OneSignal (objc): could not find notification will display event for notification with id: " for every foreground push that has multiple display-calling listeners, and conclude the SDK is broken or they're passing the wrong id. The misleading-log shape is exactly the failure mode the PR'sproceedWithWillDisplay:fix was filed about — the fix just was not extended to the siblingdisplayNotification:handler.The dominant single-listener-display pattern is unaffected: only one
displayNotificationmessage is queued, the cache is hit once, evicted, and the subsequentproceedWithWillDisplaytakes the FIXED silent path. Trigger requires the multi-listener-display setup, which is uncommon but realistic in modular apps that split feature concerns (analytics + custom UI augmentation + business logic) across separate listeners.How to fix
Mirror the
proceedWithWillDisplay:resolution intodisplayNotification:— return silently withresult(nil)when the cache is already consumed:- (void)displayNotification:(FlutterMethodCall *)call withResult:(FlutterResult)result { NSString *notificationId = call.arguments[@"notificationId"]; OSNotificationWillDisplayEvent *event = self.onWillDisplayEventCache[notificationId]; if (!event) { // Already consumed for this id (typically a sibling listener already // called display, which displays and evicts). Idempotent no-op. result(nil); return; } [event.notification display]; [self.onWillDisplayEventCache removeObjectForKey:notificationId]; [self.preventedDefaultCache removeObjectForKey:notificationId]; result(nil); }
This closes both the log-noise regression and the per-error Flutter result callback leak in a single edit, and matches the established convention the PR already applied to the symmetric handler.
- Foreground push arrives. iOS
d3f96b4 to
60ead83
Compare
Keep process-global Android channels bound to the UI engine while still registering Dart-to-native handlers on every engine, so FlutterFire background engines cannot steal notification callbacks. Add a bounded Dart cold-start click buffer and iOS willDisplay dedupe/cache handling for firebase_messaging coexistence without pulling in unrelated pre-existing callback cleanup. Co-authored-by: Cursor <cursoragent@cursor.com>
3926d91 to
64e9269
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
android/src/main/java/com/onesignal/flutter/OneSignalNotifications.java:159-163— On the new cache-eviction code path added in this PR, three null-event branches still logONE_S_LL_ERROR/Logging.errorand (on Android) return without callingreplySuccess(result, null)— symmetric to the three silent-return fixes the PR already applied in this same file. Affected: iOSpreventDefault:(OSFlutterNotifications.m:216–225), AndroiddisplayNotificationnull branch (OneSignalNotifications.java:156–159), and AndroidpreventDefaultnull branch (OneSignalNotifications.java:170–173). Functional behavior stays correct, but each affected interaction now emits a misleading ERROR log at OneSignal's default log level, and on Android the barereturn;also leaks the Flutter MethodChannelResulthandler. Mechanical fix mirroring the PR's existing pattern: replace each log+bare-return block withresult(nil); return;(iOS) orreplySuccess(result, null); return;(Android).Extended reasoning...
What the bug is
This PR added new cache eviction in two places that makes a previously-unreachable null-event branch newly reachable:
- iOS
displayNotification:(OSFlutterNotifications.m:246-248) now evicts bothonWillDisplayEventCacheandpreventedDefaultCacheafter[event.notification display]. - Android
displayNotification(OneSignalNotifications.java:162-163) now does the same onnotificationOnWillDisplayEventCacheandpreventedDefaultCache. - Android
proceedWithWillDisplay(line 149) also newly evicts the cache after display.
The PR additionally updated three null-branch handlers to return silently when the cache is empty (the canonical "already consumed" case):
- iOS
proceedWithWillDisplay:(lines 174-182) →result(nil); return;with comment "Already consumed for this id, typically because a listener called notification.display() (which displays and evicts)…" - iOS
displayNotification:(lines 237-244) →result(nil); return;with comment "Already consumed for this id, typically because a sibling willDisplay listener already called display()…" - Android
proceedWithWillDisplay(line 141) →replySuccess(result, null); return;
But three sibling null branches that the PR's new eviction can now drain into were left calling the original log+early-return code path:
// OSFlutterNotifications.m:216-225 — iOS preventDefault: if (!event) { [OneSignalLog onesignalLog:ONE_S_LL_ERROR message:[NSString stringWithFormat: @"OneSignal (objc): could not find notification will display " @"event for notification with id: %@", notificationId]]; return; // ← no result(nil); Flutter Result handler leaks }
// OneSignalNotifications.java:156-159 — Android displayNotification null branch if (event == null) { Logging.error("Could not find onWillDisplayNotification event for notification " + "with id: " + notificationId, null); return; // ← no replySuccess; Flutter Result handler leaks }
// OneSignalNotifications.java:170-173 — Android preventDefault null branch if (event == null) { Logging.error("Could not find onWillDisplayNotification event for notification " + "with id: " + notificationId, null); return; // ← no replySuccess; Flutter Result handler leaks }
How the new eviction makes these branches reachable
Dart's
_handleMethod(lib/src/notifications.dart:155-163) handles the willDisplay branch by iterating_willDisplayListenerssynchronously — each listener may callevent.notification.display()(OneSignal#displayNotification) orevent.preventDefault()(OneSignal#preventDefault) — then unconditionally invokesOneSignal#proceedWithWillDisplay. All three are fire-and-forgetinvokeMethodcalls on the same channel, so Flutter delivers them FIFO to the platform.Two realistic triggers reach the new null-cache state:
- Two willDisplay listeners with opposing actions — listener A calls
display(), listener B callspreventDefault(). iOS/Android process msg A (cache hit → display → evict both caches), then msg B reads an empty cache → hits the buggy null branch. OSNotificationClickEvent.preventDefault()on a foreground notification — lib/src/notification.dart:384-386 routes through the sameOneSignal#preventDefaultchannel method. Pre-PRdisplayNotificationdidn't evict, so a click handler firing afterdisplay()would still find the cached event; post-PR (lines 162-163 / 247-248 evict on display) the same call now hits the null branch.
Step-by-step proof using
examples/demo_fmshipped IN THIS PR- App registers a foreground willDisplay listener that calls
event.notification.display()and a separate analytics listener that callsevent.preventDefault()on a non-actionable subset (a common pattern). - A foreground OneSignal push arrives. Native auto-
preventDefaults, caches the event, and invokesOneSignal#onWillDisplayNotificationon the channel. - Dart
_handleMethodruns:- listener A →
event.notification.display()→ enqueues msg AOneSignal#displayNotification. - listener B →
event.preventDefault()→ enqueues msg BOneSignal#preventDefault. - loop ends → enqueues msg C
OneSignal#proceedWithWillDisplay.
- listener A →
- Native (Android) handles msg A: cache hit →
event.getNotification().display()→ evicts both caches (lines 162-163, NEW in this PR). ✓ - Native handles msg B (
preventDefault): cache miss → lines 170-173 fireLogging.error("Could not find onWillDisplayNotification event for notification with id: <id>")→ barereturn;→ noreplySuccess. Flutter logs a "reply was never sent" warning per affected interaction, and the production log shows a misleading ERROR for a notification that displayed correctly. - Native handles msg C (
proceedWithWillDisplay): cache miss → line 141replySuccess(result, null); return;silently. ✓
The iOS path is symmetric: msg B reaches
preventDefault:(lines 216-225), logsONE_S_LL_ERROR, andreturn;s withoutresult(nil).Why this is nit, not normal
All verifiers (six independent confirmations across the three sub-bugs) classified this as nit and agreed on the rationale:
- Functional behavior is correct. The notification displays exactly once; the eviction itself is a real improvement over pre-PR double-dispatch. Only side effect is misleading log noise + a bounded Flutter
Resultleak per interaction. - Trigger is narrow but real. Scenario 1 (display + preventDefault on the same event) is uncommon — display and preventDefault are contradictory. Scenario 2 (
clickEvent.preventDefaulton a foreground notification) is unusual API usage. But both fire from supported API combinations and surface a misleadingERRORat OneSignal's default log level. - The Dart-side
preventDefaultis fire-and-forget (lib/src/notifications.dart:191-193 — noawait), so the leaked FlutterResultdoesn't block any Future. Impact is bounded to MethodChannel "reply was never sent" warnings and the misleading log line. - The PR's own comments on the three sibling fixes anticipate exactly this scenario ("typically because a sibling willDisplay listener already called display() (which displays and evicts)") — so applying the silent-return treatment here is direct precedent, not a stretch.
How to fix
Mechanical, three small diffs that mirror the PR's existing pattern. Each replaces the log+bare-return block with the silent-success early return:
// iOS preventDefault: — OSFlutterNotifications.m:216-225 if (!event) { // Already consumed for this id, typically because a sibling willDisplay // listener already called display() (which displays and evicts). result(nil); return; }
// Android displayNotification null branch — OneSignalNotifications.java:156-159 if (event == null) { replySuccess(result, null); return; }
// Android preventDefault null branch — OneSignalNotifications.java:170-173 if (event == null) { replySuccess(result, null); return; }
This closes both the misleading-ERROR-log half and (on Android) the Flutter
Resulthandler leak in one shot. - iOS
64e9269 to
44639f9
Compare
|
@claude review |
|
@claude review I simplified the branch and moved non-blocking cleanup items to SDK-4752. Please review only for issues that directly affect SDK-4407 / #1138: firebase_messaging coexistence with OneSignal notification event delivery. Please treat the following as out of scope for this PR unless they directly break the SDK-4407 fix:
Recent local changes also:
Please focus on regressions introduced by this PR, missing test coverage for the SDK-4407 path, and any correctness issues in the Android channel rebind / click replay / iOS willDisplay dedupe logic. |
One Line Summary
Fix Android notification click & foreground events being silently dropped when
firebase_messaging(FlutterFire) is present in the same app.Motivation
GitHub #1138: since 5.4.0, users running
onesignal_flutteralongsidefirebase_messagingreportOneSignal.Notifications.addClickListenernever firing on Android background/killed notification taps.Root cause:
OneSignalNotificationsowns a process-global staticMethodChannel, butregisterWithruns once per Flutter engine. FlutterFire spins up a headless backgroundFlutterEngineforonBackgroundMessage, andGeneratedPluginRegistrantre-registers OneSignal against it — rebinding the shared channel to the background isolate, which never ranmain()and has no listeners. Native click / willDisplay callbacks were then routed to that dead isolate and dropped. An additional engine-swap case (back out ofMainActivity, then tap a notification) replayed queued clicks into a channel whose Dart end wasn't ready yet.Scope
firebase_messagingcoexistence example (examples/demo_fm/).Changes
OneSignalNotifications.registerWith: bind the shared channel only on the first engine; ignore later engines (e.g. FlutterFire's background engine) so they can't clobber it.OneSignalNotificationsnow tracks whether Dart requested a click listener and toggles the native-SDK subscription across engine/activity lifecycles, so clicks delivered while the channel is detached get queued by the native SDK instead of dispatched into a dead JNI.onAttachedToActivity(messenger): authoritatively rebind the channel to the activity-hosting engine's messenger. On a new engine, defer re-adding the native click listener and let Dart'sregisterClickListenerdrain the queue once the isolate is ready; on the same engine, re-add immediately.OneSignalPlugin: wire theActivityAwarelifecycle (attach/detach + config-change variants) into the hooks above.examples/demo_fm/: new example mirroring the affected setup (OneSignal +firebase_core+firebase_messaging), with a README documenting reproduction and direct-FCM testing.Testing
Manual, on a real device + emulator, OneSignal push tapped in each app state:
Notification clickedfiresNotification clickedfiresNotification clickedfiresForeground
willDisplaylistener also confirmed firing withfirebase_messagingpresent. Verified with runtime instrumentation (since removed) that the channel is bound to the UI isolate's messenger and that queued clicks drain after Dart re-registers.Affected code checklist
Checklist
flutter build apk --release)Made with Cursor