fix(firebase_messaging): ensure silent foreground messages for iOS are called via event channel.

[russellwheatley]

Description

see title.

Related Issues

fixes #8548

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
• All existing and new tests are passing.
• I updated/added relevant documentation (doc comments with ///).
• The analyzer (melos run analyze) does not report any problems on my PR.
• I read and followed the Flutter Style Guide.
• I signed the CLA.
• I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

• Yes, this is a breaking change.
• No, this is not a breaking change.

[hugovangalen]

Silent/data notifications while the app is foregrounded still did not work for me (tested with firebase_messaging 11.4.0 that has this patch merged).

I was able to trace the problem to the didReceiveRemoteNotification and it appears to me that the following line is flawed (line 489 in FLTFirebaseMessagingPlugin.m):

if (notificationDict[@"aps"] != nil && notificationDict[@"aps"][@"alert"] == nil) {

As I am logging the contents of the notiticationDict at that point, I see that it does not at all contain an aps property. So I wonder whether this is the intended logic there, as it wants to test for the presence (or rather, lack) of an alert property.

if (notificationDict[@"aps"] == nil || notificationDict[@"aps"][@"alert"] == nil) {

This seems to fix the issue for me.

[PapyElGringo]

11.4.0 doesn't work for me neither

[PapyElGringo]

notificationDict[@"aps"] seem to be always null
FCM

{
    ...,
    data: { toto: 'tata' },
    android: {
      priority: 'high',
    },
    apns: {
      payload: {
        aps: {
          'content-available': 1,
          alert: 'toto',
        },
      },
    },

Give

notificationDict	__NSDictionaryM *	4 key/value pairs	0x00000002804311a0
[0]	(null)	@"messageId" : @"1652813178933879"	
[1]	(null)	@"data" : 1 key/value pair	
[2]	(null)	@"contentAvailable" : YES	
[3]	(null)	@"notification" : 2 key/value pairs	

[PapyElGringo]

@hugovangalen well I believe it's more of a data model issue than just a condition issue
Because if notificationDict[@"aps"] is never populated then your condition is equivalent as if ( true || true )

[russellwheatley]

@PapyElGringo - How are you sending the message? (i.e. nodeJS admin SDK or REST API, etc)

[russellwheatley]

Silent/data notifications while the app is foregrounded still did not work for me (tested with firebase_messaging 11.4.0 that has this patch merged).

I was able to trace the problem to the didReceiveRemoteNotification and it appears to me that the following line is flawed (line 489 in FLTFirebaseMessagingPlugin.m):

if (notificationDict[@"aps"] != nil && notificationDict[@"aps"][@"alert"] == nil) {

As I am logging the contents of the notiticationDict at that point, I see that it does not at all contain an aps property. So I wonder whether this is the intended logic there, as it wants to test for the presence (or rather, lack) of an alert property.

if (notificationDict[@"aps"] == nil || notificationDict[@"aps"][@"alert"] == nil) {

This seems to fix the issue for me.

@hugovangalen - this won't work because it will fire twice on onMessage API if notification + data is present on the message. This code change is strictly firing for data only messages (i.e. when alert property is not present on aps dictionary).

[PapyElGringo]

@russellwheatley I use Nodejs Admin SDK in firebase functions

firebase-admin: ^10.0.2
firebase-function: ^3.18.1

[hugovangalen]

@hugovangalen - this won't work because it will fire twice on onMessage API if notification + data is present on the message. This code change is strictly firing for data only messages (i.e. when alert property is not present on aps dictionary).

Right okay, I see. I guess I got distracted by the comment above the condition that says "if alert is present, this will be called by the other "Messaging#onMessage" channel handler", which lead me to think that it only dives in this specific routine for data only messages. If the payload has a notification, it goes through another handler userNotificationCenter (where the 'aps' property is also missing, and the notification text data is in the "notification" property). I do not observe a double onMessage triggering though with my change.

Anyways, yeah, @PapyElGringo may be right in the 'aps' getting stripped somewhere down the line. (I also noticed that "to" field gets stripped from the "data" payload, but that is probably another issue altogether.)

The 'userInfo' dictionary does contain the 'aps' payload, so the 'aps' gets lost here:

  NSDictionary *notificationDict =
      [FLTFirebaseMessagingPlugin remoteMessageUserInfoToDict:userInfo];

(I am using the REST API by the way, with a similar payload as PapyElGringo posted above though without an 'alert' property because it's basically a silent control message to send to the UI.)

[hugovangalen]

@hugovangalen well I believe it's more of a data model issue than just a condition issue Because if notificationDict[@"aps"] is never populated then your condition is equivalent as if ( true || true )

I erroneously thought that the notificationDict is what gets delivered by the operating system to the app, yesterday I overlooked the call to [FLTFirebaseMessagingPlugin remoteMessageUserInfoToDict:userInfo] which is where the problem arises, 'aps' is present in userInfo, but not in the resulting dictionary.

In fact, in remoteMessageUserInfoToDict, this is happening:

    // build data dict from remaining keys but skip keys that shouldn't be included in data
    if ([key isEqualToString:@"aps"] || [key hasPrefix:@"gcm."] || [key hasPrefix:@"google."]) {
      continue;
    }

It seems that the 'aps' intensionally gets stripped. It kinda does make sense to strip it, as it is operating system specific data that doesn't need to end up in the Flutter app, but it does break the condition back in didReceiveRemoteNotification.

@russellwheatley I suspect that in stead of notificationDict, the userInfo should be tested in line 489:
if (userInfo[@"aps"] != nil && userInfo[@"aps"][@"alert"] == nil) {

[PapyElGringo]

@russellwheatley I use sendMulticast API

[hugovangalen]

@PapyElGringo Could you please cconfirm that for you, when you dump the userInfo dictionary, that does contain the 'aps' property?

[PapyElGringo]

My background notifications are blocked because of quotas and priority

[russellwheatley]

@hugovangalen well I believe it's more of a data model issue than just a condition issue Because if notificationDict[@"aps"] is never populated then your condition is equivalent as if ( true || true )

I erroneously thought that the notificationDict is what gets delivered by the operating system to the app, yesterday I overlooked the call to [FLTFirebaseMessagingPlugin remoteMessageUserInfoToDict:userInfo] which is where the problem arises, 'aps' is present in userInfo, but not in the resulting dictionary.

In fact, in remoteMessageUserInfoToDict, this is happening:

    // build data dict from remaining keys but skip keys that shouldn't be included in data
    if ([key isEqualToString:@"aps"] || [key hasPrefix:@"gcm."] || [key hasPrefix:@"google."]) {
      continue;
    }

It seems that the 'aps' intensionally gets stripped. It kinda does make sense to strip it, as it is operating system specific data that doesn't need to end up in the Flutter app, but it does break the condition back in didReceiveRemoteNotification.

@russellwheatley I suspect that in stead of notificationDict, the userInfo should be tested in line 489: if (userInfo[@"aps"] != nil && userInfo[@"aps"][@"alert"] == nil) {

@hugovangalen thanks for the investigation. I will double check and get back to you on this 🙏

[russellwheatley]

Hey @hugovangalen, you're correct, apologies for the mistake. I've created a PR which rectifies the issue #8759. Will get this released at some point this week 🙏