wordpress-mobile / wordpress-mobile/WordPress-iOS

`handleNotification` can drop the remote-notification completion handler, throttling background push delivery

Open Beginner friendly
#25,657 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Swift
Stars
3.9k
Forks
1.2k
Avg merge
23h 51m
Merged PRs (30d)
58

Description

Summary

PushNotificationsManager.handleNotification(_:userInteraction:completionHandler:) can return without ever invoking the OS-supplied fetchCompletionHandler. For a content-available (silent/background) push, the app then never signals iOS that it finished its background work — wasting the background assertion and, over repeated occurrences, leading iOS to throttle the app's future background/silent-push wakeups and background-fetch budget.

Pre-existing — not introduced by any recent change. Surfaced while reviewing #25643 (UIScene adoption), which makes windowless background launches a routinely-exercised path but does not touch this dispatch loop.

Root Cause

The OS entry point hands handleNotification a non-optional escaping handler (application(_:didReceiveRemoteNotification:fetchCompletionHandler:)). But handleNotification only calls it inside the four sub-handlers' success paths, with no terminal call when nothing handles the push — plus an early return on the badge path:

// Badge: Reset
guard let type = userInfo.string(forKey: Notification.typeKey), type != Notification.badgeResetValue else {
    return                       // ← completionHandler never called
}

let handlers = [
    handleSupportNotification,
    handleAuthenticationNotification,
    handleInactiveNotification,
    handleBackgroundNotification
]
for handler in handlers {
    if handler(userInfo, userInteraction, completionHandler) {
        break
    }
}
// ← if every handler returned false, completionHandler is never called

https://github.com/wordpress-mobile/WordPress-iOS/blob/f039b671ae917e5c13a4e748d98b70859183d908/WordPress/Classes/Utility/Notifications/PushNotificationsManager.swift#L208-L224

Trigger

A remote push delivered in the .background state with userInteraction == false that:

  • has a type (so it survives the badge-reset guard),
  • is not a Zendesk/support type → handleSupportNotification returns false,
  • is not an auth notification → handleAuthenticationNotification returns false,
  • handleInactiveNotification returns false (.background && !userInteraction),
  • carries no note_idhandleBackgroundNotification returns false (it requires Notification.identifierKey).

All four decline; completionHandler is never called. The badge-reset early-return at L208 is the same drop, though a badge-reset push isn't usually a content-available push the OS waits on.

In practice, real WP.com notification pushes (comment / like / follow / …) all carry note_id, so they land in handleBackgroundNotification and do call the handler. The drop therefore needs a malformed or future/unknown payload shape — latent and low-frequency, but unbounded by anything in the client.

Impact

  • A wasted ~30s background assertion per occurrence (the OS waits out the watchdog, then suspends the app).
  • iOS meters completion-handler reliability; repeated drops contribute to throttling of future silent-push delivery and background-fetch budget — i.e. degraded background responsiveness (share-extension upload completion, silent syncs).

No crash, no data loss — a robustness gap.

Fix

Guarantee the handler is called exactly once on every path:

guard let type = userInfo.string(forKey: Notification.typeKey), type != Notification.badgeResetValue else {
    completionHandler?(.noData)
    return
}

for handler in handlers {
    if handler(userInfo, userInteraction, completionHandler) {
        return                       // a handler took it (and already called completionHandler)
    }
}
completionHandler?(.noData)          // nothing handled it — still fulfil the OS contract

The return-on-handled keeps it single-call (no double invocation when a handler already called it).

Notes

  • Confirmed pre-existing: the dispatch loop and badge-reset return are identical on trunk and predate #25643. #25643's only change to this file (widening handleInactiveNotification to accept .background && userInteraction) shrinks the set of dropped pushes — it neither introduces nor worsens this.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in WordPress/Classes/Utility/Notifications/PushNotificationsManager.swift around the badge-reset guard and handler dispatch loop. Trace each handler's completion-handler behavior, then ensure the OS-supplied handler is invoked exactly once for early returns and when no handler accepts the notification; done means every path fulfills the completion contract without double-calling it.

Written by the indexing model from the issue text.

Assessment

Tech stack
ios, swift
Domain
mobile
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.