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

[firebase_messaging] better handling on background isolate initialization #1917

Conversation

spideythewebhead
Copy link

@spideythewebhead spideythewebhead commented Jan 31, 2020

Description

Handles the creation of the background isolate/view/channel by adding a guard if its already available.

Reproducing steps:

  1. Launch activity and add an background handler for firebase message.
  2. Press the back button.
  3. Launch again the activity.
  4. Press the home button.
  5. Send FCM and as you will see you won't see your method being called in the background.

Related Issues

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.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • If the pull request affects only one plugin, the PR title starts with the name of the plugin in brackets (e.g. [cloud_firestore])
  • 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 (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@spideythewebhead
Copy link
Author

spideythewebhead commented Jan 31, 2020

unfortunately when running =>
pub.bat global run flutter_plugin_tools format -- plugins firebase_messaging

i get this error..
Formatting all .dart files...
CreateProcessW failed 2
Unhandled exception:
ProcessException: The system cannot find the file specified.

Edit: I have managed to get this working for Windows (havent tested it on linux or mac, as i think its working fine, reading some related issues, flutter/flutter#15381, flutter/flutter#33964).

@spideythewebhead spideythewebhead requested review from collinjackson and kroikie and removed request for kroikie February 4, 2020 03:20
@ivanchaukn
Copy link

ivanchaukn commented Apr 3, 2020

Is this related to #1754 ?

@spideythewebhead
Copy link
Author

Is this related to #1754 ?

I think not. Its a bug i have found when pressing the back button and going to home and then reopening the app, you wont receive background notifications anymore

@Fbrusca
Copy link

Fbrusca commented May 20, 2020

Reproducing steps:

Launch activity and add an background handler for firebase message.
Press the back button.
Launch again the activity.
Press the home button.
Send FCM and as you will see you won't see your method being called in the background.

I received: "Service took too long to process intent: com.google.android.c2dm.intent.RECEIVE App may get closed."

Is this related to #2196?

@spideythewebhead
Copy link
Author

I don't know exactly because it never happened to me , or at least I didn't see that error. It has to do more, not been able to receive background notifications, after following the reproducing steps. But I don't know if this problem is solved with newer updates on the plugin... Because it's been like 3 months and I got no responses

@franco-brusca
Copy link

I don't know exactly because it never happened to me , or at least I didn't see that error. It has to do more, not been able to receive background notifications, after following the reproducing steps. But I don't know if this problem is solved with newer updates on the plugin... Because it's been like 3 months and I got no responses

The problem persists. I have tried it in the Beta and Dev channel. I am not sure why it happens ... currently the only test I have not done is using embedding v1., Since I use plugins that I use and they benefit from embedding v2

@spideythewebhead
Copy link
Author

i can try with the new embedding and and inform u, i need some time tho, because i have been in touch with flutter in 2months :(

@spideythewebhead
Copy link
Author

spideythewebhead commented May 23, 2020

hi @fbrus92 i've tried it on a new project with the latest version of messaging 6.0.15, and it seems that it works fine without the error you showed me. (i tested it on a xiaomi phone).

But something else got my attention.

Screenshot_11

If u kill the activity (by pressing back for example) and reopen, it creates a new background thread without terminating / reusing the old one, i think i am not 100% percent or maybe i have messed up the registration of the background registrant.

class App : FlutterApplication(), PluginRegistry.PluginRegistrantCallback {
  override fun onCreate() {
    super.onCreate()
    FlutterFirebaseMessagingService.setPluginRegistrant(this)
  }

  override fun registerWith(registry: PluginRegistry) {
    if (!registry.hasPlugin("io.flutter.plugins.firebasemessaging.FirebaseMessagingPlugin")) {
      FirebaseMessagingPlugin.registerWith(registry.registrarFor("io.flutter.plugins.firebasemessaging.FirebaseMessagingPlugin"))
    }
  }
}

i have tried both with and without the if part. (flutter embedding 2)

@spideythewebhead
Copy link
Author

I also tried this version of the plugin (from this commit, i have locally on my pc) and seems to work fine. without the multiple threads problem that is occuring on the previous comment.

@Fbrusca
Copy link

Fbrusca commented May 23, 2020

I also tried this version of the plugin (from this commit, i have locally on my pc) and seems to work fine. without the multiple threads problem that is occuring on the previous comment.

The problem also occurs with the Firebase Messaging version 6.0.15., But this only happens with the messages interpreted by "onBackgroundMessage"

{
  "to": "{firebase_token}",
  "collapse_key": "type_a",
  "id": 4,
"data": {
         "extra_data": "some data",
         "click_action": "FLUTTER_NOTIFICATION_CLICK"
       }
}

Note that using the following configuration:
firebase_messaging:
     git:
       url: git: //github.com/spideythewebhead/flutterfire.git
       path: packages / firebase_messaging
       ref: prevent_multiple_background_views_and_channels_creation_for_background_isolate

The number of references to "main" decreased substantially compared to version 6.0.15

and the error doesn't happen anymore !!!

I don't see the time that your changes are integrated. :P

Hopefully the google team can review it soon @Salakar

@spideythewebhead
Copy link
Author

@Fbrusca i am glad it works for you 👍

@matanshukry
Copy link

Just had an issue and @spideythewebhead repo worked for me as well.

To me the issue started once I've created another background channel. Not sure if it's truly the source though since I've added few other things as well.

Either way this PR should get in.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants