Skip to content

Set 1s timeout for onBackgroundMessage Hook #3780

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 29, 2020
Merged

Set 1s timeout for onBackgroundMessage Hook #3780

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a83b949
await onBackgroundMessage hook
zwu52 Sep 14, 2020
0139a05
Create fluffy-panthers-hide.md
zwu52 Sep 15, 2020
013741d
block in onPush to let onBackgroundMessage to execute
zwu52 Oct 2, 2020
369f2a0
polish wording
zwu52 Oct 27, 2020
8c86b7f
Update changeset
zwu52 Oct 28, 2020
e61f8d5
Update fluffy-panthers-hide.md
zwu52 Oct 28, 2020
ea74a00
Clarify PR is about a bug fix
zwu52 Oct 29, 2020
ba34515
Update fluffy-panthers-hide.md
zwu52 Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fluffy-panthers-hide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@firebase/messaging': patch
---

Awaits onBackgroundMessage hook to avoid false-positive silent pushes warnings
4 changes: 2 additions & 2 deletions packages/messaging/src/controllers/sw-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,9 +206,9 @@ export class SwController implements FirebaseMessaging, FirebaseService {
const payload = externalizePayload(internalPayload);

if (typeof this.bgMessageHandler === 'function') {
this.bgMessageHandler(payload);
await this.bgMessageHandler(payload);
} else {
this.bgMessageHandler.next(payload);
await this.bgMessageHandler.next(payload);
Copy link
Member

Choose a reason for hiding this comment

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

This is not compatible with Observer signature where next() should return void. Perhaps we should only take a callback instead of an observer in onBackgroundMessage() ?

Copy link
Member

Choose a reason for hiding this comment

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

@zxu123

Copy link
Member Author

@zwu52 zwu52 Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think the issue is caused by the fact that the silent push check (which might be a timer or run till the end of the function) races the onBackgroundMessage. The only thing we are during differently from previous API is the wrapping and transformation operations on the payload (which is necessary for format and style).

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's a bug on Chrome side? Are we supposed to wait on the promise returned by ServiceWorkerRegistration.showNotification() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure if blocking ServiceWorkerRegistration.showNotification would work. I tried a await sleep(10s) at the end of onPush. The update in background would still show up. So I am suspecting there is a timer on this. I am going to verify it in Chrome code and possibly reach out to them to figure a solution if that's the case

Copy link
Member

Choose a reason for hiding this comment

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

Does waiting on the promise returned by ServiceWorkerRegistration.showNotification() work? Chrome will always show a notification if we don't show a notification ourselves.

Copy link
Member Author

@zwu52 zwu52 Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

onBackgroundMessage is a non-async function so can't await in that hook.

However, I think I as mistaken. The showNotification check is ran after the onPush is completed with event.waitUntil(onPush).

The solution I can think of is to block n seconds at the end of onPush here to let the onBackgroundMessage code to complete. pros and cons of doing so:
pro: - limit how much time a piece of code can run in the background. Imagine a malicious code snippet just keeps running in the background and never completes. The site has been updated in background message will never show (but there is probably a max execution time enforcer somewhere).
con: - blocking messages for every n seconds even if the onBackgroundMessage logic runs less than that. - It's kind of arbitrary on how to pick that n seconds.

WDYT of this approach or is there a better way to put one an execution time limit in js?

Copy link
Member

Choose a reason for hiding this comment

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

As you noted, I'm not sure how to pick the n seconds. And I'm not sure if we should make the decision on developer's behalf as there might be valid use cases that needs longer running time.

await onBackgroundMessage() is the right thing to do IMO. Can we change its signature to void | Promise<void>, so that it's backward compatible? WDTY

Copy link
Member Author

@zwu52 zwu52 Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think changing it to Promise<void> is reasonable. Having it going through a whole API review process seems cumbersome. Is there a recommended way to deal with these changes?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can avoid the API review, but we can try to get it approved by email.

}
}
}
Expand Down