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

Notifications delayed when app not running, on Android #342

Closed
gnprice opened this issue Oct 26, 2023 · 16 comments
Closed

Notifications delayed when app not running, on Android #342

gnprice opened this issue Oct 26, 2023 · 16 comments
Assignees
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-notifications beta feedback Things beta users have specifically asked for

Comments

@gnprice
Copy link
Member

gnprice commented Oct 26, 2023

In my draft implementation of #320, notifications appear just fine when the app is in the foreground, and also when it's running in the background. But when the app is not running at all — for example if you've swiped it away in the app switcher — and then get a message, it seems like the notification will usually not promptly appear.

On at least one occasion, the notification appeared several minutes later, after I'd gone and launched the app for the sake of other testing. I'm not sure how long it might be delayed if the user doesn't happen to independently launch the app; just now I tried it again, and it's been over 10 minutes and counting.

We don't have this problem in zulip-mobile, as far as I've ever noticed. So it should be entirely a client-side issue to solve.

The fix will probably involve taking over some of the work that the Java code in package:firebase_messaging does, for listening to the FCM service and acting on those messages. In zulip-mobile we do that work ourselves, in Kotlin, and then act on them within Kotlin as well. In a worst case we can always do the same here, but I'm hopeful that we can keep the application logic in Dart instead — we'd just need to be more eager in passing over to Dart than package:firebase_messaging is. But that will require investigation.

(Even in that worst case where we do end up keeping application logic in Kotlin for notifications, we'll be in a much better position to share its application data with our Dart code (cf zulip/zulip-mobile#5117) than we were with React Native in zulip-mobile. Our accounts data is already in a nice clean SQLite table; and when we start storing from-the-server data, that will also be in reasonable tabular form from the beginning. And a critical aspect of any such data sharing would be integration tests spanning both sides (cf zulip/zulip-mobile#5589), which Flutter supports well and RN just catastrophically didn't.)

@gnprice gnprice added a-Android Issues specific to Android, or requiring Android-specific work a-notifications labels Oct 26, 2023
@gnprice gnprice added this to the Beta milestone Oct 26, 2023
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Nov 1, 2023
This completes the core functionality of showing notifications on
Android, zulip#320.

Sadly this does not work very well if the app isn't running at all:
e.g., if you terminate the app by swiping it away in the app switcher.
In that case the notification can be quite a bit delayed.

But fixing that seems likely to require some deeper debugging, and
getting our hands into Java or Kotlin code for I think the first time
in zulip-flutter.  So we'll deal with that as a followup issue, zulip#342.
Details there.

Fixes: zulip#320
gnprice added a commit that referenced this issue Nov 1, 2023
This completes the core functionality of showing notifications on
Android, #320.

Sadly this does not work very well if the app isn't running at all:
e.g., if you terminate the app by swiping it away in the app switcher.
In that case the notification can be quite a bit delayed.

But fixing that seems likely to require some deeper debugging, and
getting our hands into Java or Kotlin code for I think the first time
in zulip-flutter.  So we'll deal with that as a followup issue, #342.
Details there.

Fixes: #320
@gnprice
Copy link
Member Author

gnprice commented Nov 1, 2023

It looks like the symptoms of this may vary between devices. My observations above were on my Pixel 5 running Android 13. @chrisbobbe reports at #352 (review) :

FWIW, in some brief testing on the office Android device (Samsung Galaxy S9 running Android 9), I don't notice long delays in the case where the app isn't running at all. After I swipe it away in the app switcher, I can send myself DMs from a test account and still get a notification for each one within a few seconds.

So that's something to bear in mind when investigating.

One thing I wonder is if the difference is in the app switcher UI — if maybe the one in Samsung's Android 9 leaves the app still in the background, in a way that the one in Google/Pixel's Android 13 doesn't.

@gnprice gnprice added the performance Smooth and responsive UI; fixing jank, stutters, and lag label Nov 18, 2023
@gnprice gnprice modified the milestones: Beta 1, Beta 2 Dec 19, 2023
@n-92
Copy link

n-92 commented Jan 3, 2024

Hello,

Just to let you know that I managed to get the notifications working by adding the following annotation before the function:

Another issue I am facing right now is, there is no sound during the notification and the notification itself disappears right after it shows up on the screen.

@pragma('vm:entry-point')
  static Future<void> _onBackgroundMessage(FirebaseRemoteMessage message) async {
    // This callback will run in a separate isolate from the rest of the app.
    // See docs:
    //   https://firebase.flutter.dev/docs/messaging/usage/#background-messages
    _initBackgroundIsolate();

    assert(debugLog("notif message in background: ${message.data}"));
    _onRemoteMessage(message);
  }

The file I edited is notifications.dart file. I got this tip from another developer who posted under firebase project.
Link

@gnprice
Copy link
Member Author

gnprice commented Jan 3, 2024

Interesting, thanks. If I understand you correctly, the change was that you added this line:

  @pragma('vm:entry-point')

It makes sense that that could be needed, so that's a good thing to try.

Another issue I am facing right now is, there is no sound during the notification and the notification itself disappears right after it shows up on the screen.

Let's discuss separate issues in separate threads. 🙂 I'd be glad to investigate these either in a new issue thread here on GitHub, or in a thread in #mobile on chat.zulip.org.

@Itsamanjain

This comment was marked as off-topic.

@n-92

This comment was marked as off-topic.

@gnprice

This comment was marked as off-topic.

@gnprice gnprice added beta feedback Things beta users have specifically asked for and removed performance Smooth and responsive UI; fixing jank, stutters, and lag labels Feb 9, 2024
@gnprice
Copy link
Member Author

gnprice commented Feb 13, 2024

@gnprice
Copy link
Member Author

gnprice commented Feb 13, 2024

I tried the @pragma('vm:entry-point') change, and it didn't fix the issue. I'll probably still add that line, since it shouldn't hurt and it does look like the package:firebase_messaging docs recommend it, but there's still something to debug here.

In particular one observation is that the issue reproduces for me even in debug mode, not only in release mode. That pragma only has an effect when Dart is compiled in AOT mode (see docs — not sure why that isn't in a more official location, but it's the personal domain of one of the Dart compiler's developers), and for Flutter apps that happens in release mode and profile mode but not debug mode. (Flutter debug mode uses the Dart VM's JIT mode, as part of how it accomplishes fast hot reload.) So whatever's causing the issue in debug mode, that pragma shouldn't be able to have any effect on at all.

Here's how the issue currently reproduces for me:

  • Build and run the app, in debug mode. (Likely it'd reproduce the same in release mode, but I haven't verified that.)
  • While the app is still in the foreground, cause a notification, by sending a DM from a different account and a different device (e.g., a desktop browser).
  • This should produce a notification. It should also produce a notification simultaneously from the zulip-mobile main app — this is a helpful control in the later steps, so if it doesn't then that's worth fixing by installing the main app, logging into the account, etc.
  • Now swipe to the home screen, and cause another notification. This should still appear from both apps simultaneously.
  • Now swipe to open the "recent apps" screen, and swipe the Flutter app away from there. (This stops the app, so that it's no longer running even in the background.) Then cause another notification. This one should also appear from both apps simultaneously — but the behavior I'm currently actually seeing is that it appears only from the main/legacy app, not the Flutter app.

Just now this reproduced for me on a few tries in a row; and it reproduced again after I added that pragma and then hit "run" in the IDE to build and install the updated app.

By contrast, if I open the main app and then quit it in the same way, subsequent notifications still arrive promptly.

(This was all on my Pixel 8 running Android 14.)

@n-92

This comment was marked as off-topic.

@gnprice

This comment was marked as off-topic.

@n-92

This comment was marked as off-topic.

@gnprice
Copy link
Member Author

gnprice commented Feb 14, 2024

I spent some time today debugging this, and found the failure seems to happening at a deeper, earlier layer than I'd hoped. I don't yet have a diagnosis.

The first key debugging step was to start using a local version of package:firebase_messaging, so that I could edit it to add logging and otherwise experiment:

diff --git a/pubspec.yaml b/pubspec.yaml
index 3693e3652..dad3e99b7 100644
--- a/pubspec.yaml
+++ b/pubspec.yaml
@@ -50 +50,2 @@ dependencies:
-  firebase_messaging: ^14.6.3
+  firebase_messaging:
+    path: ../firebase-flutter/packages/firebase_messaging/firebase_messaging

Then flutter pub get, and then — crucially — stop Gradle (android/gradlew -p android --stop) before starting again. (Without that last step, the Android build continues using the old version. This seems like a bug in Flutter's Gradle integration for loading native Android plugins. I haven't searched for it in the upstream tracker.)

Then I found that even FlutterFirebaseMessagingReceiver.onReceive isn't getting called when the app isn't running. This is surprising because that class is listed in the manifest in the same way as com.google.firebase.iid.FirebaseInstanceIdReceiver is in zulip-mobile, which I believe is the entry point through which notifications first reach that app's code. This step didn't actually yet require a local firebase_messaging; just rerun the repro steps above, watching logcat filtered to tag:FLTFire (in the Android Studio logcat UI), and look for the log line "broadcast received for message" which that function emits:

  public void onReceive(Context context, Intent intent) {
    Log.d(TAG, "broadcast received for message");

(where TAG is FLTFireMsgReceiver). That line does appear when the app is in the foreground or background, but not when it's not running.

Along the same lines, FlutterFirebaseMessagingService isn't getting started. The quick way to get evidence of this is to add a log line to the onMessageReceived implementation, and see that it's called when the app is in the foreground or background but not when it's not running. That method itself is called by some infrastructure provided by the firebase-messaging Maven module, so that evidence doesn't rule out that the service is getting started and that infrastructure just isn't calling the method… so for more definitive evidence, one can cut out that infrastructure entirely by replacing the service's definition thus, inheriting directly from android.app.Service:

public class FlutterFirebaseMessagingService extends Service {
  private static final String TAG = "FLTFire1MsgService";

  @Override
  public int onStartCommand(Intent intent, int flags, int startId) {
    Log.i(TAG, "onStartCommand: " + intent.toString());
    return super.onStartCommand(intent, flags, startId);
  }

  @Nullable
  @Override
  public IBinder onBind(Intent intent) {
    return null;
  }
}

The outcome of that experiment is that onStartCommand itself only gets called — i.e., the service is only getting started, at the Android SDK level — when a notification arrives while the app is in the foreground or background, not when it's not running.

This leaves me puzzled at what the variable is that causes the problem to arise here while things work great in zulip-mobile. It seems like the issue arises already before basically any of the code from package:firebase_messaging or its dependencies even runs. I went back through the Android manifests (the "merged" manifests, the versions that go into the built app, incorporating all the manifest fragments from dependencies) of the two apps, and didn't find any differences that looked relevant. So, more debugging needed.

One possible next step would be to do similar log-debugging on zulip-mobile, and confirm that the corresponding methods do get called there when the app wasn't running. On my current understanding of how notification delivery works, they have to be in order for the notifications to appear — which they do — but it'd be good to check that.

If that's confirmed, then possibly the right next step is to make a hello-world app that receives notifications with package:firebase_messaging and see if the problem reproduces the same way, and if so then make a hello-world pure Android app that receives notifications (serving as a stripped-down stand-in for zulip-mobile) and see if the problem reproduces there, in order to narrow things down to a minimal pair of working vs. not-working. For those, the most annoying part might be dealing with getting the FCM tokens and then getting the notifications sent through FCM; with the two actual Zulip clients, there's a bunch of code in the clients and the Zulip server that deals with that.

@gnprice
Copy link
Member Author

gnprice commented Feb 14, 2024

… Well, I can no longer reproduce this issue myself. I don't reproduce it on a fresh install:

I had been reproducing this issue consistently on my existing install, even through lots of reinstalls/updates (but without ever uninstalling the app first) as I tried various changes for debugging. But at one point around thirty minutes ago I thought that #521 had fixed the issue for fresh installs, and so I went and replaced my main install with a fresh install to test that. The issue is indeed gone there now, but then just to confirm things I tried a fresh install from main and discovered that that also cleared up the issue. As a side effect of that testing, I no longer have an install of the app where the issue reproduces (and I don't know a way to make one, since fresh installs work fine).

It's possible that this means this issue isn't a problem for normal users — that it was somehow a legacy of the many variations of the app, including all kinds of debugging code and other ad-hoc changes, that I'd installed on my device while developing notification support. We've had various reports of trouble with notifications, but I'm not aware of any that are clearly the same set of symptoms that I was seeing and that this thread is about; some I think were definitely #520, and others like "no sound" (above at #342 (comment)) remain undiagnosed but seem clearly distinct.

I'll leave this open for now while I ask around (thread here) to see if I can find anyone who's actually seeing this same set of symptoms. If that doesn't turn up any cases, then we'll close it.

@gnprice
Copy link
Member Author

gnprice commented Feb 23, 2024

I'll leave this open for now while I ask around (thread here) to see if I can find anyone who's actually seeing this same set of symptoms. If that doesn't turn up any cases, then we'll close it.

That didn't, so closing this issue.

From a user perspective, though, notifications on Android still won't work yet. After #521 (fixing #520) they work perfectly for me in a debug build… but they don't show up at all in a release build, even when the app is already running and even when it's in the foreground. Details in #528, which we'll track as a separate issue because the pattern of symptoms no longer matches the one this thread was about.

@gnprice gnprice closed this as completed Feb 23, 2024
gnprice added a commit to gnprice/zulip-flutter that referenced this issue Feb 23, 2024
This fixes part of zulip#528, following the suggestion n-92 made here:
  zulip#342 (comment)

Even with this change, notifications still don't yet work on Android
in release builds.  But after this change, when the app wasn't in
the foreground, they get farther than before.  Specifically they get
just as far as they get, with or without this change, when the app
was in the foreground.  Details here:
  zulip#528 (comment)
chrisbobbe pushed a commit that referenced this issue Feb 23, 2024
This fixes part of #528, following the suggestion n-92 made here:
  #342 (comment)

Even with this change, notifications still don't yet work on Android
in release builds.  But after this change, when the app wasn't in
the foreground, they get farther than before.  Specifically they get
just as far as they get, with or without this change, when the app
was in the foreground.  Details here:
  #528 (comment)
@gnprice
Copy link
Member Author

gnprice commented Mar 1, 2024

@n-92 @Itsamanjain This issue, and its siblings #520 and #528, are all fixed in today's new beta release v0.0.11. That means that notifications should now be working normally in the published release build of the beta app. Please try it out, and report issues or feedback with anything you notice!

Thanks in particular @n-92 for the pointer on @pragma('vm:entry-point'), which turned out to be one of the several fixes we needed, as #529.

@n-92
Copy link

n-92 commented Mar 4, 2024

You are welcome. I am happy to contribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-Android Issues specific to Android, or requiring Android-specific work a-notifications beta feedback Things beta users have specifically asked for
Projects
Status: Done
3 participants