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

"This site has been updated in the background" #1836

Closed
1 task done
ekzyis opened this issue Jan 21, 2025 · 5 comments · Fixed by #1837
Closed
1 task done

"This site has been updated in the background" #1836

ekzyis opened this issue Jan 21, 2025 · 5 comments · Fixed by #1837
Labels

Comments

@ekzyis
Copy link
Member

ekzyis commented Jan 21, 2025

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

Yesterday and just now, I got "This site has been updated in the background" as a push notification on my laptop running NixOS and Brave.

Screenshots

Image

Steps To Reproduce

???

Expected behavior

Never receive "this site has been updated in the background"

Logs

No response

Device information

No response

Additional context

No response

@ekzyis ekzyis added the bug label Jan 21, 2025
@Soxasora
Copy link
Member

Soxasora commented Jan 21, 2025

I had something like this on Safari during tests and the culprit was this

    if (!payload) return // ignore push events without payload, like isTrusted events

At every onPush event there's an isTrusted that gets passed to it, signaling that the notification is permitted by the user and/or the site is valid. This was here on early code too, but if we don't return at empty payload we would get an empty isTrusted notification at every onPush.
Monitoring closely because this would also be a cause of push sub being lost (push notifs are a bit needy aren't they)

@ekzyis
Copy link
Member Author

ekzyis commented Jan 21, 2025

Shouldn't every notification have some payload?

I just received this again together with a "stacked" notification:

Image

@Soxasora
Copy link
Member

Soxasora commented Jan 21, 2025

If it happens only on "stacked" notifications we can think that event.waitUntil isn't really extending the time and the onPush event concludes earlier than expected.
Which is alarming. I'll monitor the onPush flow and see what comes up

Shouldn't every notification have some payload?

To reply to this, yes they have to have some payload, in case of isTrusted the payload is empty because it's the browser's job to handle it

@Soxasora Soxasora self-assigned this Jan 21, 2025
@ekzyis
Copy link
Member Author

ekzyis commented Jan 21, 2025

If it happens only on "stacked" notifications we can think that event.waitUntil isn't really extending the time and the onPush event concludes earlier than expected.

I also received it with a "X has replied to a post" notification (user subscription). It also only happens ocassionally.

we can think that event.waitUntil isn't really extending the time and the onPush event concludes earlier than expected.

Mhhh, but the documentation mentions the purpose of event.waitUntil is that this doesn't happen:

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.

I looked at the event.waitUntil calls and I noticed that a promise isn't returned in one case. Not sure if this will fix this issue, but it looks related:

// iOS requirement: wait for all promises to resolve before showing the notification
event.waitUntil(Promise.all(promises).then(() => {
sw.registration.showNotification(payload.title, payload.options)
}))
}

I think this should be this:

diff --git a/sw/eventListener.js b/sw/eventListener.js
index 0e8b6778..fcec05be 100644
--- a/sw/eventListener.js
+++ b/sw/eventListener.js
@@ -59,7 +59,7 @@ export function onPush (sw) {

     // iOS requirement: wait for all promises to resolve before showing the notification
     event.waitUntil(Promise.all(promises).then(() => {
-      sw.registration.showNotification(payload.title, payload.options)
+      return sw.registration.showNotification(payload.title, payload.options)
     }))
   }
 }

So I think it's more likely that the service worker is still running after the browser considered it already done. That's why the silent push warning shows up first before I see the actual notification. I think that means the service worker is not terminated immediately but still had some time to show the notification 🤔

in case of isTrusted the payload is empty because it's the browser's job to handle it

Do you mean the browser triggers onPush for some internal events that don't have a payload? I thought only events that we generated on the server will run the code in onPush.

@Soxasora Soxasora removed their assignment Jan 21, 2025
@Soxasora
Copy link
Member

Do you mean the browser triggers onPush for some internal events that don't have a payload? I thought only events that we generated on the server will run the code in onPush.

That what I thought, too. But if you remove the empty payload 'filter' you'll get isTrusted notifications

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 a pull request may close this issue.

2 participants