Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: iOS push subscription gets lost #1802

Closed

Conversation

Soxasora
Copy link
Member

@Soxasora Soxasora commented Jan 5, 2025

Description

Partial (final) fix of #756, 1: (push notifications continue to be lost #411)

Sometimes an await showNotification can be considered as an hidden push notification (like those spam websites), when 3 of those gets recognized as 'hidden' consecutively, Apple revokes the subscription.

We need to tell the service worker that the work is finished (sending a notification) and we could do this by using event.waitUntil1 instead of await

This PR aligns showNotification calls with Apple's recommended way, which is event.waitUntil(showNotification).

Screenshots

n/a

Additional Context

This measure aligns our service worker with Apple's vision of how push notifications should work and it should be cross-platform. Not having Android I would need some test on it to see if notifications get sent correctly (Chrome didn't have problems)

This is part of a series of fixes tracked on #1794

Edit: I just found this comment, which is correct but weirdly enough return await showNotification sometimes creates a notification without telling the service worker that it happened, causing a 'silent notification'.
event.waitUntil tells the service worker that work is happening and doesn't return a Promise1
I can be incorrect in evaluating what is happening here

Passing event from onPush to mergeAndShowNotification should ensure that showNotification gets done within the same context not violating Apple's requirement

Checklist

Are your changes backwards compatible? Please answer below:
Yesnt, I need to test with Android

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
6, tested only on iOS, so far no lost push subscription in a 2-day test environment.

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
n/a

Did you introduce any new environment variables? If so, call them out explicitly here:
No

Footnotes

  1. The ExtendableEvent.waitUntil() method tells the event dispatcher that work is ongoing. It can also be used to detect whether that work was successful. In service workers, waitUntil() tells the browser that work is ongoing until the promise settles, and it shouldn't terminate the service worker if it wants that work to complete.
    waitUntil specification 2

@Soxasora Soxasora added the bug label Jan 5, 2025
@Soxasora Soxasora marked this pull request as draft January 5, 2025 14:50
@Soxasora
Copy link
Member Author

Soxasora commented Jan 5, 2025

Sending too many notifications one after another causes push sub to be lost, investigating
edit: was on a different version of the worker but I'll continue stress testing

@Soxasora Soxasora marked this pull request as ready for review January 5, 2025 15:20
@ekzyis ekzyis self-requested a review January 5, 2025 18:04
@Soxasora Soxasora marked this pull request as draft January 6, 2025 09:27
@Soxasora
Copy link
Member Author

Soxasora commented Jan 6, 2025

Merging to #1794

@Soxasora Soxasora closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant