-
Notifications
You must be signed in to change notification settings - Fork 17
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
Fix app crashing when app is not yet started on some devices #215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just requested a comment removal
android/src/main/java/io/ably/flutter/plugin/push/FirebaseMessagingReceiver.java
Outdated
Show resolved
Hide resolved
a59d688
to
44b9b91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
android/src/main/java/io/ably/flutter/plugin/push/FirebaseMessagingReceiver.java
Outdated
Show resolved
Hide resolved
… future messages received when app is running (in foreground or background) will fail. This was because of retaining an old FlutterJNI and methodChannel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. I made some requests about some minor comment changes. There are also some places where nullable variables are accessed without any check or annotation.
android/src/main/java/io/ably/flutter/plugin/push/ManualFlutterApplicationRunner.java
Outdated
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/ManualFlutterApplicationRunner.java
Outdated
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/ManualFlutterApplicationRunner.java
Outdated
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/ManualFlutterApplicationRunner.java
Outdated
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/ManualFlutterApplicationRunner.java
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/PushMessagingEventHandlers.java
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/PushMessagingEventHandlers.java
Outdated
Show resolved
Hide resolved
android/src/main/java/io/ably/flutter/plugin/push/PushMessagingEventHandlers.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
There are actually three bugs:
activityManager.getRunningAppProcesses()
can return null when no processes are running. I was iterating over it, assuming it would be an empty list instead of null. This caused a NPE like crash. (3 line fix, excluding comments)BackgroundIsolateAndroidPlatform
. (one line fix)PushMessagingEventHandlers.load(applicationContext, methodChannel);
Tried to send a platform message to Flutter, but FlutterJNI was detached from native C++.
.See customer report: https://ably-real-time.slack.com/archives/C012DA29VM0/p1636555532089600?thread_ts=1636544116.088600&cid=C012DA29VM0