From cd3b8f357199fcc47ef90111e244a7ccce010cd0 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Wed, 8 Feb 2023 13:42:35 +1100 Subject: [PATCH] Fixed a few issues related to the GroupAvatarDownloadJob Added logic to avoid scheduling a GroupAvatarDownloadJob if there is already an existing one with the same parameters Added logic to avoid trying to download an old avatar for a group if there is a new one Added logic to prevent an old GroupAvatarDownloadJob from overriding a valid avatar with an old one --- .../securesms/database/SessionJobDatabase.kt | 4 +-- .../securesms/database/Storage.kt | 4 +-- .../libsession/database/StorageProtocol.kt | 2 +- .../messaging/jobs/BackgroundGroupAddJob.kt | 4 +-- .../messaging/jobs/GroupAvatarDownloadJob.kt | 29 +++++++++++++++++-- .../pollers/OpenGroupPoller.kt | 8 +++-- 6 files changed, 38 insertions(+), 13 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt index 4425e3d85d..66497d9da8 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/SessionJobDatabase.kt @@ -83,11 +83,11 @@ class SessionJobDatabase(context: Context, helper: SQLCipherOpenHelper) : Databa } } - fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? { + fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? { val database = databaseHelper.readableDatabase return database.getAll(sessionJobTable, "$jobType = ?", arrayOf(GroupAvatarDownloadJob.KEY)) { jobFromCursor(it) as GroupAvatarDownloadJob? - }.filterNotNull().find { it.server == server && it.room == room } + }.filterNotNull().find { it.server == server && it.room == room && (imageId == null || it.imageId == imageId) } } fun cancelPendingMessageSendJobs(threadID: Long) { diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt index f7e2d9d06b..33aad17d1d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt @@ -226,8 +226,8 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context, return DatabaseComponent.get(context).sessionJobDatabase().getMessageReceiveJob(messageReceiveJobID) } - override fun getGroupAvatarDownloadJob(server: String, room: String): GroupAvatarDownloadJob? { - return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room) + override fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): GroupAvatarDownloadJob? { + return DatabaseComponent.get(context).sessionJobDatabase().getGroupAvatarDownloadJob(server, room, imageId) } override fun resumeMessageSendJobIfNeeded(messageSendJobID: String) { diff --git a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt index c80ef24642..ee124aa0e1 100644 --- a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt +++ b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt @@ -49,7 +49,7 @@ interface StorageProtocol { fun getAttachmentUploadJob(attachmentID: Long): AttachmentUploadJob? fun getMessageSendJob(messageSendJobID: String): MessageSendJob? fun getMessageReceiveJob(messageReceiveJobID: String): Job? - fun getGroupAvatarDownloadJob(server: String, room: String): Job? + fun getGroupAvatarDownloadJob(server: String, room: String, imageId: String?): Job? fun resumeMessageSendJobIfNeeded(messageSendJobID: String) fun isJobCanceled(job: Job): Boolean diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt index ef67408fb3..5154101328 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/BackgroundGroupAddJob.kt @@ -43,8 +43,8 @@ class BackgroundGroupAddJob(val joinUrl: String): Job { storage.setOpenGroupPublicKey(openGroup.server, openGroup.serverPublicKey) val info = storage.addOpenGroup(openGroup.joinUrl()) val imageId = info?.imageId - if (imageId != null) { - JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.room, openGroup.server)) + if (imageId != null && storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId) == null) { + JobQueue.shared.add(GroupAvatarDownloadJob(openGroup.server, openGroup.room, imageId)) } Log.d(KEY, "onOpenGroupAdded(${openGroup.server})") storage.onOpenGroupAdded(openGroup.server) diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt index 6429d760ae..e176046361 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/GroupAvatarDownloadJob.kt @@ -5,7 +5,7 @@ import org.session.libsession.messaging.open_groups.OpenGroupApi import org.session.libsession.messaging.utilities.Data import org.session.libsession.utilities.GroupUtil -class GroupAvatarDownloadJob(val room: String, val server: String) : Job { +class GroupAvatarDownloadJob(val server: String, val room: String, val imageId: String?) : Job { override var delegate: JobDelegate? = null override var id: String? = null @@ -13,10 +13,30 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { override val maxFailureCount: Int = 10 override fun execute(dispatcherName: String) { + if (imageId == null) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob now requires imageId")) + return + } + val storage = MessagingModuleConfiguration.shared.storage - val imageId = storage.getOpenGroup(room, server)?.imageId ?: return + val storedImageId = storage.getOpenGroup(room, server)?.imageId + + if (storedImageId == null || storedImageId != imageId) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId does not match the OpenGroup")) + return + } + try { val bytes = OpenGroupApi.downloadOpenGroupProfilePicture(server, room, imageId).get() + + // Once the download is complete the imageId might no longer match, so we need to fetch it again just in case + val postDownloadStoredImageId = storage.getOpenGroup(room, server)?.imageId + + if (postDownloadStoredImageId == null || postDownloadStoredImageId != imageId) { + delegate?.handleJobFailedPermanently(this, dispatcherName, Exception("GroupAvatarDownloadJob imageId no longer matches the OpenGroup")) + return + } + val groupId = GroupUtil.getEncodedOpenGroupID("$server.$room".toByteArray()) storage.updateProfilePicture(groupId, bytes) storage.updateTimestampUpdated(groupId, System.currentTimeMillis()) @@ -30,6 +50,7 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { return Data.Builder() .putString(ROOM, room) .putString(SERVER, server) + .putString(IMAGE_ID, imageId) .build() } @@ -40,6 +61,7 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { private const val ROOM = "room" private const val SERVER = "server" + private const val IMAGE_ID = "imageId" } class Factory : Job.Factory { @@ -47,7 +69,8 @@ class GroupAvatarDownloadJob(val room: String, val server: String) : Job { override fun create(data: Data): GroupAvatarDownloadJob { return GroupAvatarDownloadJob( data.getString(ROOM), - data.getString(SERVER) + data.getString(SERVER), + if (data.hasString(IMAGE_ID)) { data.getString(IMAGE_ID) } else { null } ) } } diff --git a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt index 353b4e1bf5..4f270206be 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/sending_receiving/pollers/OpenGroupPoller.kt @@ -165,14 +165,16 @@ class OpenGroupPoller(private val server: String, private val executorService: S pollInfo.details.imageId != null && ( pollInfo.details.imageId != existingOpenGroup.imageId || !storage.hasDownloadedProfilePicture(dbGroupId) - ) + ) && + storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, pollInfo.details.imageId) == null ) || ( pollInfo.details == null && existingOpenGroup.imageId != null && - !storage.hasDownloadedProfilePicture(dbGroupId) + !storage.hasDownloadedProfilePicture(dbGroupId) && + storage.getGroupAvatarDownloadJob(openGroup.server, openGroup.room, existingOpenGroup.imageId) == null ) ) { - JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server)) + JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server, existingOpenGroup.imageId)) } }