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

onPush not awaiting onBackgroundMessage event handler #3725

Closed
niyoko opened this issue Sep 2, 2020 · 9 comments
Closed

onPush not awaiting onBackgroundMessage event handler #3725

niyoko opened this issue Sep 2, 2020 · 9 comments
Assignees

Comments

@niyoko
Copy link

niyoko commented Sep 2, 2020

  • Operating System version: ArchLinux 5.8.5
  • Browser version: Chromium 85
  • Firebase SDK version: 7.19.1
  • Firebase Product: messaging

There's always additional notification when there is background push message coming, saying something like "The site has been updated in the background". I suspect the culprit is code below

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

As you can see, this.bgMessageHandler is called without await, so when I return promise from onBackgroundMessage callback to display custom notification, it is not await-ed.

@niyoko niyoko changed the title onPush not awaiting onBackgroundMessage event handler onPush not awaiting onBackgroundMessage event handler Sep 3, 2020
@zwu52
Copy link
Member

zwu52 commented Sep 3, 2020

Hi niyoko,
thanks for reaching out. Can you share your send curl or payloads?

I am guessing you are sending a FM data notification without a notification payload. For privacy/security reason the browser notifies the end user that some site is receiving background push and processing if no display notification is shown. Below are some solutions to this behavior:

  1. Call ​serviceWorkerRegistration.showNotification in onBackgroundMessage hook. You can customize a display message that is less scary than the force-shown message.

  2. Leave as it is. The "The site has been updated in the background" would appear once every 10 (IIRC) background messages.

  3. Add a notification payload to your pushes. This might be a good practice in informing the user that your site is doing something in background.

But these should probably be documented in the API doc.
🍻

@niyoko
Copy link
Author

niyoko commented Sep 4, 2020

Yes, it is data only payload.

Here is our payload format

{
  "to": "our_token",
  "data": {
    "eventName": "BusinessEvent\\Order\\Created",
    "actionUrlPlatform": "/relative/url",
    "title": "Title",
    "body": "Body",
    "notificationId": "123",
    "notificationRecipientId": "456"
  }
}

We already call showNotification from onBackgroundMessage and return the resulting Promise. The browser correctly show our notification but it also shows "The site has been updated..." notification. We are not passing notification payload because we want the notifications to merge when there are a lot of successive notifications coming in short timespan as described here.

messaging.onBackgroundMessage(function(payload) {
  var data = payload.data || {};
  var title = data.title || '';
  var options = {
    body: data.body || '',
    icon: 'https://domain/link/to/our/logo',
    badge: 'https://domain/link/to/our/logo',
    lang: 'id',
    data: { cta: data.actionUrlPlatform },
  };

  if (!data.eventName) {
    return self.registration.showNotification(title, options);
  } else {
    options.data.eventName = data.eventName;
    return self.registration
      .getNotifications()
      .then(function(notifications) {
        var currentNotification, i;
        for (i = 0; i < notifications.length; i++) {
          if (notifications[i].data && notifications[i].data.eventName === data.eventName)
            currentNotification = notifications[i];
        }

        return currentNotification;
      })
      .then(function(currentNotification) {
        var messageCount;
        if (currentNotification) {
          messageCount = (currentNotification.data.newMessageCount || 0) + 1;
          options.data.newMessageCount = messageCount;
          title = '[' + messageCount + '] ' + title;
          currentNotification.close();
        } else {
          options.data.newMessageCount = 1;
        }

        return registration.showNotification(title, options);
      });
  }
});

@zwu52
Copy link
Member

zwu52 commented Sep 11, 2020

niyoko,
thanks for sharing. I am able to reproduce. I think this is because the onBackgroundMessage is doing a "long-running" task and not being await as you suspected.

@zwu52
Copy link
Member

zwu52 commented Oct 14, 2020

@niyoko niyoko,
The solution to fix this might mean blocking briefly for each background message. I am doing some experiments and I found that looping though 1000 messages (arbitrary messages stored in notification box) and check for collapsing takes ~0.03s to complete and no silent push warning would be shown (on my device). I'd like to know more info on the devices that you seeing this issue:

  • How often do you see the warning
  • if possible, do you mind running some experiments on devices with the issue and get a delta t for running the onBackgroundMessage logic (I am using performance.now())

@tristan-morris
Copy link

tristan-morris commented Oct 16, 2020

Thanks for getting to the bottom of this issue @zwu52 and @niyoko. Not to ditto..

My code exhibits similar behaviour. On a Pixel 2 XL running as a Trusted Web App, the code below will throw the updated in background warning for maybe every second message throughout the day. It's more consistently shown when the phone hasn't been recently used, so I've assumed there's been more state to wake up/restore which takes longer to execute. As far as speed goes to execute the block of code, it's been pretty quick! But there's more going on.

This screenshot is from console on my Pixel 2 XL. Each "run" maybe two seconds apart. That 6.4 ms (I'm assuming the unit is actually ms) for the first execution, then followed by 0.70ms is solid difference.

image

Code within my service worker:

messaging.onBackgroundMessage(function(message) {
  console.log("onBackgroundMessage", message);

  let notificationOptions = {
    body: message.data.body,
    icon: message.data.icon,
    badge: message.data.badge,
    data: message.data
  };
  return self.registration.showNotification(
    message.data.title,
    notificationOptions
  );
});

(Removing the console.log dramatically improves the execution speed - down to 0.09ms and occasionally "0").

@zwu52
Copy link
Member

zwu52 commented Nov 6, 2020

Hi Triston,
thanks for taking the time to run experiments and sharing. Fixing PR was created and checked in. The solution was to create a timeout to allow showNotification async function to complete. Fix will be releasesd next week. Thanks for the input.

I am going to keep this thread open for a while before closing it. Feel free to post any question if any.

Edit: released. There will be an upcoming doc update. But make sure to call showNotification first in the onBackgroundMessage hook before any long running task.

@zwu52 zwu52 closed this as completed Nov 10, 2020
@corysimmons
Copy link

What if you need to show a notification with data collected from a fetch?

@zwu52
Copy link
Member

zwu52 commented Dec 10, 2020

@corysimmons
thanks for reaching out. yes, that would be unsupported by the SDK ATM. I am not sure what is your exact use case, but one thing you could consider doing is to include whatever data in the data payload if it's applicable. Please file a feature request ticket If fetching data and display is an absolute need, we likely need a new API for that since the current one is non-async.

@corysimmons
Copy link

I ended up just passing notification data along with client.postMessage and then in the service worker triggering a notification to focus the tab in question onclick. Worked nicely.

@firebase firebase locked and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants