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.

Sign up for GitHub

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

Jump to bottom

MM-58818 fix android push notifications when app is closed #7989

Merged
merged 1 commit into from
Jun 10, 2024
Merged

MM-58818 fix android push notifications when app is closed #7989

merged 1 commit into from
Jun 10, 2024

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Jun 5, 2024

Summary

On Android, when a push notification is received while the app is fully closed we were getting a crash with the following message

FATAL EXCEPTION: DefaultDispatcher-worker-1
Process: com.mattermost.rnbeta, PID: 12800
java.lang.IllegalAccessError: Interface com.nozbe.watermelondb.WMDatabase$TransactionFunction implemented by class com.mattermost.helpers.database_extension.GeneralKt$$ExternalSyntheticLambda2 is inaccessible (declaration of 'com.mattermost.helpers.database_extension.GeneralKt$$ExternalSyntheticLambda2' appears in /data/app/~~vkUbZhRNsPjUP2iOhNthEA==/com.mattermost.rnbeta-hrAYzoiHtA2oAM2UBe28qw==/base.apk!classes19.dex)
at com.mattermost.helpers.database_extension.GeneralKt.getDatabaseForServer(Unknown Source:0)
at com.mattermost.helpers.PushNotificationDataRunnable$Companion.start(PushNotificationDataHelper.kt:43)
at com.mattermost.helpers.PushNotificationDataHelper$fetchAndStoreDataForPushNotification$job$1.invokeSuspend(PushNotificationDataHelper.kt:23)
at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)
    Suppressed: kotlinx.coroutines.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@4838933, Dispatchers.Default]

This is due to the fact that the WatermelonDB interface TransactionFunction was not explicitly set to public and for some reason not accessible while the app was not running, although it does not present the same problem when the app is in fact running either in the foreground or background.

Along with this fix, we migrated the CustomPushNotification class to kotlin for better interoperatibility ensuring we process only one notification at a time even when they arrive in quick succession.

Release Note

Fixed a crash when receiving a push notification on Android when the app is not running

enahum
enahum commented Jun 5, 2024
View reviewed changes
patches/@nozbe+watermelondb+0.27.1.patch
+ public void unsafeVacuum() {
+ execute("vacuum");
+ }
+
interface TransactionFunction {
+ public interface TransactionFunction {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the actual fix

android/app/src/main/java/com/mattermost/helpers/database_extension/General.kt
@@ -57,7 +57,7 @@ fun DatabaseHelper.getDatabaseForServer(context: Context?, serverUrl: String): W
if (cursor.count == 1) {
cursor.moveToFirst()
val databasePath = String.format("file://%s", cursor.getString(0))
return WMDatabase.getInstance(databasePath, context!!)
return WMDatabase.buildDatabase(databasePath, context!!, SQLiteDatabase.CREATE_IF_NECESSARY)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is to ensure we create a new database connection as when they arrive in quick succession the code complains that we are trying to open an already closed database.

android/app/src/main/java/com/mattermost/rnbeta/CustomPushNotification.kt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migrated to Kotlin

@enahum enahum requested a review from larkox June 5, 2024 03:41
larkox
larkox approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

@enahum enahum added 4: Reviews Complete All reviewers have approved the pull request CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jun 10, 2024
@enahum enahum added this to the v2.18.0 milestone Jun 10, 2024
@enahum
Copy link
Contributor Author

enahum commented Jun 10, 2024

I'd say this should even be cherry-picked for 2.17, thoughts?

cc: @larkox

@enahum enahum merged commit e5ce796 into main Jun 10, 2024
25 checks passed
@enahum enahum deleted the fix-android-notifications branch June 10, 2024 21:15
@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit that referenced this pull request Jun 10, 2024
@enahum @mattermost-build
fix android push notifications when app is closed (#7989)
614159a 
(cherry picked from commit e5ce796)
@mattermost-build mattermost-build mentioned this pull request Jun 10, 2024
Merged
@mattermost-build mattermost-build added CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone and removed CherryPick/Approved Meant for the quality or patch release tracked in the milestone labels Jun 10, 2024
enahum added a commit that referenced this pull request Jun 10, 2024
@mattermost-build @enahum
fix android push notifications when app is closed (#7989) (#8003)
38d3c7d 
(cherry picked from commit e5ce796)

Co-authored-by: Elias Nahum <[email protected]>
@nobuto-m
Copy link

I'd say this should even be cherry-picked for 2.17, thoughts?

Has this landed to 2.17 in the end? In the git history, it's slated for 2.18. However, the v2.17.0 tag somehow contains the commit.

$ git branch -rv --contains 38d3c7d
origin/build-release-528 b28cd2b Bump app build and version number (#8023) (#8024)
origin/release-2.18 b28cd2b Bump app build and version number (#8023) (#8024)

$ git tag --contains 38d3c7d
v2.17.0

I'm curious because I'm seeing instability of push notifications when the app is closed after upgrading from 2.16 to 2.17. If the patch is not included, I can test it out whenever the next beta gets available.

@saturninoabril saturninoabril changed the title fix android push notifications when app is closed MM-58818 fix android push notifications when app is closed Jun 19, 2024
@amyblais
Copy link
Member

/cherry-pick release-2.17

@mattermost-build
Copy link
Contributor

Cherry pick is scheduled.

mattermost-build pushed a commit that referenced this pull request Jun 19, 2024
@enahum @mattermost-build
fix android push notifications when app is closed (#7989)
674203f 
(cherry picked from commit e5ce796)
@mattermost-build mattermost-build mentioned this pull request Jun 19, 2024
Merged
larkox pushed a commit that referenced this pull request Jun 19, 2024
@mattermost-build @enahum
fix android push notifications when app is closed (#7989) (#8030)
5e3c6ee 
(cherry picked from commit e5ce796)

Co-authored-by: Elias Nahum <[email protected]>
@devkartik123
Copy link

It is still not working. I just tried with the latest code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larkox larkox larkox approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request CherryPick/Done Successfully cherry-picked to the quality or patch release tracked in the milestone release-note
Projects
None yet
Milestone
v2.18.0
Development

Successfully merging this pull request may close these issues.
7 participants
@enahum @mattermost-build @nobuto-m @amyblais @devkartik123 @larkox @mm-cloud-bot