From 810430e80651a5a5113b74fcff98b854fa01188e Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 20 Jan 2023 09:02:59 +1100 Subject: [PATCH 1/7] Fixed a couple of issues with the OpenGroupDeleteJob Updated the OpenGroupDispatcher to have a thread limit of 8 (was previously unlimited which could result in the app getting flooded with threads under certain conditions) Updated the OpenGroupDeleteJob to do bulk deletions (instead of individual message deletions) Updated the OpenGroupDeleteJob to catch and report failures (wasn't previously happening) --- .../attachments/DatabaseAttachmentProvider.kt | 14 +++++ .../database/AttachmentDatabase.java | 23 ++++++++ .../database/GroupReceiptDatabase.java | 6 ++ .../securesms/database/LokiMessageDatabase.kt | 59 +++++++++++++++++++ .../securesms/database/MessagingDatabase.java | 1 + .../securesms/database/MmsDatabase.kt | 17 ++++++ .../securesms/database/SmsDatabase.java | 26 ++++++++ .../database/MessageDataProvider.kt | 2 + .../libsession/messaging/jobs/JobQueue.kt | 2 +- .../messaging/jobs/OpenGroupDeleteJob.kt | 27 ++++++--- 10 files changed, 169 insertions(+), 8 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/attachments/DatabaseAttachmentProvider.kt b/app/src/main/java/org/thoughtcrime/securesms/attachments/DatabaseAttachmentProvider.kt index fa0fce7bd3..6e9185094b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/attachments/DatabaseAttachmentProvider.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/attachments/DatabaseAttachmentProvider.kt @@ -176,6 +176,11 @@ class DatabaseAttachmentProvider(context: Context, helper: SQLCipherOpenHelper) return messageDB.getMessageID(serverId, threadId) } + override fun getMessageIDs(serverIds: List, threadId: Long): Pair, List> { + val messageDB = DatabaseComponent.get(context).lokiMessageDatabase() + return messageDB.getMessageIDs(serverIds, threadId) + } + override fun deleteMessage(messageID: Long, isSms: Boolean) { val messagingDatabase: MessagingDatabase = if (isSms) DatabaseComponent.get(context).smsDatabase() else DatabaseComponent.get(context).mmsDatabase() @@ -184,6 +189,15 @@ class DatabaseAttachmentProvider(context: Context, helper: SQLCipherOpenHelper) DatabaseComponent.get(context).lokiMessageDatabase().deleteMessageServerHash(messageID) } + override fun deleteMessages(messageIDs: List, threadId: Long, isSms: Boolean) { + val messagingDatabase: MessagingDatabase = if (isSms) DatabaseComponent.get(context).smsDatabase() + else DatabaseComponent.get(context).mmsDatabase() + + messagingDatabase.deleteMessages(messageIDs.toLongArray(), threadId) + DatabaseComponent.get(context).lokiMessageDatabase().deleteMessages(messageIDs) + DatabaseComponent.get(context).lokiMessageDatabase().deleteMessageServerHashes(messageIDs) + } + override fun updateMessageAsDeleted(timestamp: Long, author: String) { val database = DatabaseComponent.get(context).mmsSmsDatabase() val address = Address.fromSerialized(author) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentDatabase.java index 182f8536d9..45172e2f6f 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/AttachmentDatabase.java @@ -35,6 +35,7 @@ import com.bumptech.glide.Glide; import net.zetetic.database.sqlcipher.SQLiteDatabase; +import org.apache.commons.lang3.StringUtils; import org.json.JSONArray; import org.json.JSONException; import org.session.libsession.messaging.sending_receiving.attachments.Attachment; @@ -318,6 +319,28 @@ public class AttachmentDatabase extends Database { notifyAttachmentListeners(); } + @SuppressWarnings("ResultOfMethodCallIgnored") + void deleteAttachmentsForMessages(long[] mmsIds) { + SQLiteDatabase database = databaseHelper.getWritableDatabase(); + Cursor cursor = null; + String mmsIdString = StringUtils.join(mmsIds, ','); + + try { + cursor = database.query(TABLE_NAME, new String[] {DATA, THUMBNAIL, CONTENT_TYPE}, MMS_ID + " IN (?)", + new String[] {mmsIdString}, null, null, null); + + while (cursor != null && cursor.moveToNext()) { + deleteAttachmentOnDisk(cursor.getString(0), cursor.getString(1), cursor.getString(2)); + } + } finally { + if (cursor != null) + cursor.close(); + } + + database.delete(TABLE_NAME, MMS_ID + " IN (?)", new String[] {mmsIdString}); + notifyAttachmentListeners(); + } + public void deleteAttachment(@NonNull AttachmentId id) { SQLiteDatabase database = databaseHelper.getWritableDatabase(); diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupReceiptDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupReceiptDatabase.java index d4140910dd..a6fed5be83 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupReceiptDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupReceiptDatabase.java @@ -8,6 +8,7 @@ import androidx.annotation.NonNull; import net.zetetic.database.sqlcipher.SQLiteDatabase; +import org.apache.commons.lang3.StringUtils; import org.session.libsession.utilities.Address; import org.thoughtcrime.securesms.database.helpers.SQLCipherOpenHelper; @@ -109,6 +110,11 @@ public class GroupReceiptDatabase extends Database { db.delete(TABLE_NAME, MMS_ID + " = ?", new String[] {String.valueOf(mmsId)}); } + void deleteRowsForMessages(long[] mmsIds) { + SQLiteDatabase db = databaseHelper.getWritableDatabase(); + db.delete(TABLE_NAME, MMS_ID + " IN (?)", new String[] {StringUtils.join(mmsIds, ',')}); + } + void deleteAllRows() { SQLiteDatabase db = databaseHelper.getWritableDatabase(); db.delete(TABLE_NAME, null, null); diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/LokiMessageDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/LokiMessageDatabase.kt index 41a136caac..45184c2d23 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/LokiMessageDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/LokiMessageDatabase.kt @@ -77,6 +77,25 @@ class LokiMessageDatabase(context: Context, helper: SQLCipherOpenHelper) : Datab database.endTransaction() } + fun deleteMessages(messageIDs: List) { + val database = databaseHelper.writableDatabase + database.beginTransaction() + + database.delete( + messageIDTable, + "${Companion.messageID} IN (${messageIDs.map { "?" }.joinToString(",")})", + messageIDs.map { "$it" }.toTypedArray() + ) + database.delete( + messageThreadMappingTable, + "${Companion.messageID} IN (${messageIDs.map { "?" }.joinToString(",")})", + messageIDs.map { "$it" }.toTypedArray() + ) + + database.setTransactionSuccessful() + database.endTransaction() + } + /** * @return pair of sms or mms table-specific ID and whether it is in SMS table */ @@ -96,6 +115,37 @@ class LokiMessageDatabase(context: Context, helper: SQLCipherOpenHelper) : Datab } } + fun getMessageIDs(serverIDs: List, threadID: Long): Pair, List> { + val database = databaseHelper.readableDatabase + + // Retrieve the message ids + val messageIdCursor = database + .rawQuery( + """ + SELECT ${messageThreadMappingTable}.${messageID}, ${messageIDTable}.${messageType} + FROM ${messageThreadMappingTable} + JOIN ${messageIDTable} ON ${messageIDTable}.message_id = ${messageThreadMappingTable}.${messageID} + WHERE ( + ${messageThreadMappingTable}.${Companion.threadID} = $threadID AND + ${messageThreadMappingTable}.${Companion.serverID} IN (${serverIDs.joinToString(",")}) + ) + """ + ) + + val smsMessageIds: MutableList = mutableListOf() + val mmsMessageIds: MutableList = mutableListOf() + while (messageIdCursor.moveToNext()) { + if (messageIdCursor.getInt(1) == SMS_TYPE) { + smsMessageIds.add(messageIdCursor.getLong(0)) + } + else { + mmsMessageIds.add(messageIdCursor.getLong(0)) + } + } + + return Pair(smsMessageIds, mmsMessageIds) + } + override fun setServerID(messageID: Long, serverID: Long, isSms: Boolean) { val database = databaseHelper.writableDatabase val contentValues = ContentValues(3) @@ -183,6 +233,15 @@ class LokiMessageDatabase(context: Context, helper: SQLCipherOpenHelper) : Datab database.delete(messageHashTable, "${Companion.messageID} = ?", arrayOf(messageID.toString())) } + fun deleteMessageServerHashes(messageIDs: List) { + val database = databaseHelper.writableDatabase + database.delete( + messageHashTable, + "${Companion.messageID} IN (${messageIDs.map { "?" }.joinToString(",")})", + messageIDs.map { "$it" }.toTypedArray() + ) + } + fun migrateThreadId(legacyThreadId: Long, newThreadId: Long) { val database = databaseHelper.writableDatabase val contentValues = ContentValues(1) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MessagingDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/MessagingDatabase.java index bc0594df01..2471db1cb5 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MessagingDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MessagingDatabase.java @@ -42,6 +42,7 @@ public abstract class MessagingDatabase extends Database implements MmsSmsColumn public abstract void markAsDeleted(long messageId, boolean read); public abstract boolean deleteMessage(long messageId); + public abstract boolean deleteMessages(long[] messageId, long threadId); public abstract void updateThreadId(long fromId, long toId); diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.kt b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.kt index d82c6bb278..3f94dd6bcf 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/MmsDatabase.kt @@ -995,6 +995,23 @@ class MmsDatabase(context: Context, databaseHelper: SQLCipherOpenHelper) : Messa return threadDeleted } + override fun deleteMessages(messageIds: LongArray, threadId: Long): Boolean { + val attachmentDatabase = get(context).attachmentDatabase() + val groupReceiptDatabase = get(context).groupReceiptDatabase() + + queue(Runnable { attachmentDatabase.deleteAttachmentsForMessages(messageIds) }) + groupReceiptDatabase.deleteRowsForMessages(messageIds) + + val database = databaseHelper.writableDatabase + database!!.delete(TABLE_NAME, ID_IN, arrayOf(messageIds.joinToString(","))) + + val threadDeleted = get(context).threadDatabase().update(threadId, false) + notifyConversationListeners(threadId) + notifyStickerListeners() + notifyStickerPackListeners() + return threadDeleted + } + override fun updateThreadId(fromId: Long, toId: Long) { val contentValues = ContentValues(1) contentValues.put(THREAD_ID, toId) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java index 358518deac..320cee477c 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/SmsDatabase.java @@ -31,6 +31,7 @@ import com.annimon.stream.Stream; import net.zetetic.database.sqlcipher.SQLiteDatabase; import net.zetetic.database.sqlcipher.SQLiteStatement; +import org.apache.commons.lang3.StringUtils; import org.session.libsession.messaging.calls.CallMessageType; import org.session.libsession.messaging.messages.signal.IncomingGroupMessage; import org.session.libsession.messaging.messages.signal.IncomingTextMessage; @@ -52,6 +53,7 @@ import org.thoughtcrime.securesms.dependencies.DatabaseComponent; import java.io.IOException; import java.security.SecureRandom; +import java.util.Arrays; import java.util.Collections; import java.util.LinkedList; import java.util.List; @@ -596,6 +598,30 @@ public class SmsDatabase extends MessagingDatabase { return threadDeleted; } + @Override + public boolean deleteMessages(long[] messageIds, long threadId) { + String[] argsArray = new String[messageIds.length]; + String[] argValues = new String[messageIds.length]; + Arrays.fill(argsArray, "?"); + + for (int i = 0; i < messageIds.length; i++) { + argValues[i] = (messageIds[i] + ""); + } + + String combinedMessageIdArgss = StringUtils.join(messageIds, ','); + String combinedMessageIds = StringUtils.join(messageIds, ','); + Log.i("MessageDatabase", "Deleting: " + combinedMessageIds); + SQLiteDatabase db = databaseHelper.getWritableDatabase(); + db.delete( + TABLE_NAME, + ID + " IN (" + StringUtils.join(argsArray, ',') + ")", + argValues + ); + boolean threadDeleted = DatabaseComponent.get(context).threadDatabase().update(threadId, false); + notifyConversationListeners(threadId); + return threadDeleted; + } + @Override public void updateThreadId(long fromId, long toId) { ContentValues contentValues = new ContentValues(1); diff --git a/libsession/src/main/java/org/session/libsession/database/MessageDataProvider.kt b/libsession/src/main/java/org/session/libsession/database/MessageDataProvider.kt index eb40df6e09..9adf6b9327 100644 --- a/libsession/src/main/java/org/session/libsession/database/MessageDataProvider.kt +++ b/libsession/src/main/java/org/session/libsession/database/MessageDataProvider.kt @@ -20,7 +20,9 @@ interface MessageDataProvider { * @return pair of sms or mms table-specific ID and whether it is in SMS table */ fun getMessageID(serverId: Long, threadId: Long): Pair? + fun getMessageIDs(serverIDs: List, threadID: Long): Pair, List> fun deleteMessage(messageID: Long, isSms: Boolean) + fun deleteMessages(messageIDs: List, threadId: Long, isSms: Boolean) fun updateMessageAsDeleted(timestamp: Long, author: String) fun getServerHashForMessage(messageID: Long): String? fun getDatabaseAttachment(attachmentId: Long): DatabaseAttachment? diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt index 215d20834a..f4d71fadf5 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt @@ -26,7 +26,7 @@ class JobQueue : JobDelegate { private val jobTimestampMap = ConcurrentHashMap() private val rxDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() private val rxMediaDispatcher = Executors.newFixedThreadPool(4).asCoroutineDispatcher() - private val openGroupDispatcher = Executors.newCachedThreadPool().asCoroutineDispatcher() + private val openGroupDispatcher = Executors.newFixedThreadPool(8).asCoroutineDispatcher()//Executors.newCachedThreadPool().asCoroutineDispatcher() private val txDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() private val scope = CoroutineScope(Dispatchers.Default) + SupervisorJob() private val queue = Channel(UNLIMITED) diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/OpenGroupDeleteJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/OpenGroupDeleteJob.kt index c4180c0025..1fb2d0df22 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/OpenGroupDeleteJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/OpenGroupDeleteJob.kt @@ -23,14 +23,27 @@ class OpenGroupDeleteJob(private val messageServerIds: LongArray, private val th val dataProvider = MessagingModuleConfiguration.shared.messageDataProvider val numberToDelete = messageServerIds.size Log.d(TAG, "Deleting $numberToDelete messages") - var numberDeleted = 0 - messageServerIds.forEach { serverId -> - val (messageId, isSms) = dataProvider.getMessageID(serverId, threadId) ?: return@forEach - dataProvider.deleteMessage(messageId, isSms) - numberDeleted++ + + // FIXME: This entire process should probably run in a transaction (with the attachment deletion happening only if it succeeded) + try { + val messageIds = dataProvider.getMessageIDs(messageServerIds.toList(), threadId) + + // Delete the SMS messages + if (messageIds.first.isNotEmpty()) { + dataProvider.deleteMessages(messageIds.first, threadId, true) + } + + // Delete the MMS messages + if (messageIds.second.isNotEmpty()) { + dataProvider.deleteMessages(messageIds.second, threadId, false) + } + + Log.d(TAG, "Deleted ${messageIds.first.size + messageIds.second.size} messages successfully") + delegate?.handleJobSucceeded(this) + } + catch (e: Exception) { + delegate?.handleJobFailed(this, e) } - Log.d(TAG, "Deleted $numberDeleted messages successfully") - delegate?.handleJobSucceeded(this) } override fun serialize(): Data = Data.Builder() From afa42daab1f1c4091c22902fe1665289c83296a6 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 20 Jan 2023 09:19:29 +1100 Subject: [PATCH 2/7] Updated the 'scrollToBottom' behaviour to be more efficient --- .../conversation/v2/ConversationActivityV2.kt | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt index 9b985384f9..2a0d7a55ea 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/conversation/v2/ConversationActivityV2.kt @@ -315,11 +315,24 @@ class ConversationActivityV2 : PassphraseRequiredActionBarActivity(), InputBarDe restoreDraftIfNeeded() setUpUiStateObserver() binding!!.scrollToBottomButton.setOnClickListener { - val layoutManager = binding?.conversationRecyclerView?.layoutManager ?: return@setOnClickListener + val layoutManager = (binding?.conversationRecyclerView?.layoutManager as? LinearLayoutManager) ?: return@setOnClickListener + if (layoutManager.isSmoothScrolling) { binding?.conversationRecyclerView?.scrollToPosition(0) } else { - binding?.conversationRecyclerView?.smoothScrollToPosition(0) + // It looks like 'smoothScrollToPosition' will actually load all intermediate items in + // order to do the scroll, this can be very slow if there are a lot of messages so + // instead we check the current position and if there are more than 10 items to scroll + // we jump instantly to the 10th item and scroll from there (this should happen quick + // enough to give a similar scroll effect without having to load everything) + val position = layoutManager.findFirstVisibleItemPosition() + if (position > 10) { + binding?.conversationRecyclerView?.scrollToPosition(10) + } + + binding?.conversationRecyclerView?.post { + binding?.conversationRecyclerView?.smoothScrollToPosition(0) + } } } unreadCount = mmsSmsDb.getUnreadCount(viewModel.threadId) From 0ed5c5825d4c3f9932cdee6c1c9858ab651edf82 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 20 Jan 2023 15:24:14 +1100 Subject: [PATCH 3/7] Cleaned up some of the error logging --- .../securesms/ApplicationContext.java | 5 ++++ .../messaging/file_server/FileServerApi.kt | 6 ++++- .../messaging/jobs/BatchMessageReceiveJob.kt | 24 ++++++++++++----- .../messaging/jobs/MessageSendJob.kt | 26 ++++++++++++++----- .../messaging/open_groups/OpenGroupApi.kt | 6 ++++- .../libsession/snode/OnionRequestAPI.kt | 4 +-- .../libsession/utilities/DownloadUtilities.kt | 7 ++++- .../org/session/libsignal/utilities/HTTP.kt | 14 +++++++--- 8 files changed, 71 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java b/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java index 4f1270acc5..ef4f5c46a3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java +++ b/app/src/main/java/org/thoughtcrime/securesms/ApplicationContext.java @@ -47,6 +47,7 @@ import org.session.libsession.utilities.Util; import org.session.libsession.utilities.WindowDebouncer; import org.session.libsession.utilities.dynamiclanguage.DynamicLanguageContextWrapper; import org.session.libsession.utilities.dynamiclanguage.LocaleParser; +import org.session.libsignal.utilities.HTTP; import org.session.libsignal.utilities.JsonUtil; import org.session.libsignal.utilities.Log; import org.session.libsignal.utilities.ThreadUtils; @@ -67,6 +68,7 @@ import org.thoughtcrime.securesms.groups.OpenGroupMigrator; import org.thoughtcrime.securesms.home.HomeActivity; import org.thoughtcrime.securesms.jobmanager.JobManager; import org.thoughtcrime.securesms.jobmanager.impl.JsonDataSerializer; +import org.thoughtcrime.securesms.jobmanager.impl.NetworkConstraint; import org.thoughtcrime.securesms.jobs.FastJobStorage; import org.thoughtcrime.securesms.jobs.JobManagerFactories; import org.thoughtcrime.securesms.logging.AndroidLogger; @@ -237,6 +239,9 @@ public class ApplicationContext extends Application implements DefaultLifecycleO resubmitProfilePictureIfNeeded(); loadEmojiSearchIndexIfNeeded(); EmojiSource.refresh(); + + NetworkConstraint networkConstraint = new NetworkConstraint.Factory(this).create(); + HTTP.INSTANCE.setConnectedToNetwork(networkConstraint::isMet); } @Override diff --git a/libsession/src/main/java/org/session/libsession/messaging/file_server/FileServerApi.kt b/libsession/src/main/java/org/session/libsession/messaging/file_server/FileServerApi.kt index f4972080be..01fae1f503 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/file_server/FileServerApi.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/file_server/FileServerApi.kt @@ -77,7 +77,11 @@ object FileServerApi { OnionRequestAPI.sendOnionRequest(requestBuilder.build(), server, serverPublicKey).map { it.body ?: throw Error.ParsingFailed }.fail { e -> - Log.e("Loki", "File server request failed.", e) + when (e) { + // No need for the stack trace for HTTP errors + is HTTP.HTTPRequestFailedException -> Log.e("Loki", "File server request failed due to error: ${e.message}") + else -> Log.e("Loki", "File server request failed", e) + } } } else { Promise.ofFail(IllegalStateException("It's currently not allowed to send non onion routed requests.")) diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt index 07c104cfda..7bf330fe3b 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt @@ -94,12 +94,24 @@ class BatchMessageReceiveJob( threadMap[threadID]!! += parsedParams } } catch (e: Exception) { - Log.e(TAG, "Couldn't receive message.", e) - if (e is MessageReceiver.Error && !e.isRetryable) { - Log.e(TAG, "Message failed permanently",e) - } else { - Log.e(TAG, "Message failed",e) - failures += messageParameters + when (e) { + is MessageReceiver.Error.DuplicateMessage, MessageReceiver.Error.SelfSend -> { + Log.i(TAG, "Couldn't receive message, failed with error: ${e.message}") + failures += messageParameters + } + is MessageReceiver.Error -> { + if (!e.isRetryable) { + Log.e(TAG, "Couldn't receive message, failed permanently", e) + } + else { + Log.e(TAG, "Couldn't receive message, failed", e) + failures += messageParameters + } + } + else -> { + Log.e(TAG, "Couldn't receive message, failed", e) + failures += messageParameters + } } } } diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt index cdd9e0a3ac..8ce1adf481 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/MessageSendJob.kt @@ -11,6 +11,7 @@ import org.session.libsession.messaging.messages.visible.VisibleMessage import org.session.libsession.messaging.sending_receiving.MessageSender import org.session.libsession.messaging.utilities.Data import org.session.libsession.snode.OnionRequestAPI +import org.session.libsignal.utilities.HTTP import org.session.libsignal.utilities.Log class MessageSendJob(val message: Message, val destination: Destination) : Job { @@ -67,14 +68,25 @@ class MessageSendJob(val message: Message, val destination: Destination) : Job { val promise = MessageSender.send(this.message, this.destination).success { this.handleSuccess() }.fail { exception -> - Log.e(TAG, "Couldn't send message due to error: $exception.") - if (exception is MessageSender.Error) { - if (!exception.isRetryable) { this.handlePermanentFailure(exception) } + var logStacktrace = true + + when (exception) { + // No need for the stack trace for HTTP errors + is HTTP.HTTPRequestFailedException -> { + logStacktrace = false + + if (exception.statusCode == 429) { this.handlePermanentFailure(exception) } + else { this.handleFailure(exception) } + } + is MessageSender.Error -> { + if (!exception.isRetryable) { this.handlePermanentFailure(exception) } + else { this.handleFailure(exception) } + } + else -> this.handleFailure(exception) } - if (exception is OnionRequestAPI.HTTPRequestFailedAtDestinationException && exception.statusCode == 429) { - this.handlePermanentFailure(exception) - } - this.handleFailure(exception) + + if (logStacktrace) { Log.e(TAG, "Couldn't send message due to error", exception) } + else { Log.e(TAG, "Couldn't send message due to error: ${exception.message}") } } try { promise.get() diff --git a/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroupApi.kt b/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroupApi.kt index 51f7108f5a..46eff4b03b 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroupApi.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/open_groups/OpenGroupApi.kt @@ -383,7 +383,11 @@ object OpenGroupApi { } return if (request.useOnionRouting) { OnionRequestAPI.sendOnionRequest(requestBuilder.build(), request.server, publicKey).fail { e -> - Log.e("SOGS", "Failed onion request", e) + when (e) { + // No need for the stack trace for HTTP errors + is HTTP.HTTPRequestFailedException -> Log.e("SOGS", "Failed onion request: ${e.message}") + else -> Log.e("SOGS", "Failed onion request", e) + } } } else { Promise.ofFail(IllegalStateException("It's currently not allowed to send non onion routed requests.")) diff --git a/libsession/src/main/java/org/session/libsession/snode/OnionRequestAPI.kt b/libsession/src/main/java/org/session/libsession/snode/OnionRequestAPI.kt index f93a7b243e..bcce887a5a 100644 --- a/libsession/src/main/java/org/session/libsession/snode/OnionRequestAPI.kt +++ b/libsession/src/main/java/org/session/libsession/snode/OnionRequestAPI.kt @@ -78,8 +78,8 @@ object OnionRequestAPI { // endregion class HTTPRequestFailedBlindingRequiredException(statusCode: Int, json: Map<*, *>, destination: String): HTTPRequestFailedAtDestinationException(statusCode, json, destination) - open class HTTPRequestFailedAtDestinationException(val statusCode: Int, val json: Map<*, *>, val destination: String) - : Exception("HTTP request failed at destination ($destination) with status code $statusCode.") + open class HTTPRequestFailedAtDestinationException(statusCode: Int, json: Map<*, *>, val destination: String) + : HTTP.HTTPRequestFailedException(statusCode, json, "HTTP request failed at destination ($destination) with status code $statusCode.") class InsufficientSnodesException : Exception("Couldn't find enough snodes to build a path.") private data class OnionBuildingResult( diff --git a/libsession/src/main/java/org/session/libsession/utilities/DownloadUtilities.kt b/libsession/src/main/java/org/session/libsession/utilities/DownloadUtilities.kt index 0a61d1ede0..b850baa253 100644 --- a/libsession/src/main/java/org/session/libsession/utilities/DownloadUtilities.kt +++ b/libsession/src/main/java/org/session/libsession/utilities/DownloadUtilities.kt @@ -2,6 +2,7 @@ package org.session.libsession.utilities import okhttp3.HttpUrl import org.session.libsession.messaging.file_server.FileServerApi +import org.session.libsignal.utilities.HTTP import org.session.libsignal.utilities.Log import java.io.* @@ -40,7 +41,11 @@ object DownloadUtilities { outputStream.write(it) } } catch (e: Exception) { - Log.e("Loki", "Couldn't download attachment.", e) + when (e) { + // No need for the stack trace for HTTP errors + is HTTP.HTTPRequestFailedException -> Log.e("Loki", "Couldn't download attachment due to error: ${e.message}") + else -> Log.e("Loki", "Couldn't download attachment", e) + } throw e } } diff --git a/libsignal/src/main/java/org/session/libsignal/utilities/HTTP.kt b/libsignal/src/main/java/org/session/libsignal/utilities/HTTP.kt index aea1fce2d9..5eac7cecd4 100644 --- a/libsignal/src/main/java/org/session/libsignal/utilities/HTTP.kt +++ b/libsignal/src/main/java/org/session/libsignal/utilities/HTTP.kt @@ -12,6 +12,7 @@ import javax.net.ssl.SSLContext import javax.net.ssl.X509TrustManager object HTTP { + var isConnectedToNetwork: (() -> Boolean) = { false } private val seedNodeConnection by lazy { OkHttpClient().newBuilder() @@ -64,8 +65,12 @@ object HTTP { private const val timeout: Long = 120 - class HTTPRequestFailedException(val statusCode: Int, val json: Map<*, *>?) - : kotlin.Exception("HTTP request failed with status code $statusCode.") + open class HTTPRequestFailedException( + val statusCode: Int, + val json: Map<*, *>?, + message: String = "HTTP request failed with status code $statusCode" + ) : kotlin.Exception(message) + class HTTPNoNetworkException : HTTPRequestFailedException(0, null, "No network connection") enum class Verb(val rawValue: String) { GET("GET"), PUT("PUT"), POST("POST"), DELETE("DELETE") @@ -120,8 +125,11 @@ object HTTP { response = connection.newCall(request.build()).execute() } catch (exception: Exception) { Log.d("Loki", "${verb.rawValue} request to $url failed due to error: ${exception.localizedMessage}.") + + if (!isConnectedToNetwork()) { throw HTTPNoNetworkException() } + // Override the actual error so that we can correctly catch failed requests in OnionRequestAPI - throw HTTPRequestFailedException(0, null) + throw HTTPRequestFailedException(0, null, "HTTP request failed due to: ${exception.message}") } return when (val statusCode = response.code()) { 200 -> { From 8a4a9623ccac57f4e8eb1108593ff2d8183a5c76 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Fri, 20 Jan 2023 17:42:46 +1100 Subject: [PATCH 4/7] Fixed an edge case where an OpenGroup might not download it's image --- .../securesms/database/GroupDatabase.java | 13 +++++++++++++ .../thoughtcrime/securesms/database/Storage.kt | 4 ++++ .../libsession/database/StorageProtocol.kt | 1 + .../sending_receiving/pollers/OpenGroupPoller.kt | 16 ++++++++++++++-- 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java index 3e23f524f3..584bf3a71a 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/GroupDatabase.java @@ -318,6 +318,19 @@ public class GroupDatabase extends Database implements LokiOpenGroupDatabaseProt notifyConversationListListeners(); } + public boolean hasDownloadedProfilePicture(String groupId) { + try (Cursor cursor = databaseHelper.getReadableDatabase().query(TABLE_NAME, new String[]{AVATAR}, GROUP_ID + " = ?", + new String[] {groupId}, + null, null, null)) + { + if (cursor != null && cursor.moveToNext()) { + return !cursor.isNull(0); + } + + return false; + } + } + public void updateMembers(String groupId, List
members) { Collections.sort(members); 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 7daae9ef0c..cc31e71ecf 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/Storage.kt @@ -321,6 +321,10 @@ class Storage(context: Context, helper: SQLCipherOpenHelper) : Database(context, DatabaseComponent.get(context).groupDatabase().updateProfilePicture(groupID, newValue) } + override fun hasDownloadedProfilePicture(groupID: String): Boolean { + return DatabaseComponent.get(context).groupDatabase().hasDownloadedProfilePicture(groupID) + } + override fun getReceivedMessageTimestamps(): Set { return SessionMetaProtocol.getTimestamps() } 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 edca7cd15e..660b919c35 100644 --- a/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt +++ b/libsession/src/main/java/org/session/libsession/database/StorageProtocol.kt @@ -81,6 +81,7 @@ interface StorageProtocol { // Open Group Metadata fun updateTitle(groupID: String, newValue: String) fun updateProfilePicture(groupID: String, newValue: ByteArray) + fun hasDownloadedProfilePicture(groupID: String): Boolean fun setUserCount(room: String, server: String, newValue: Int) // Last Message Server ID 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 595f7d4dc1..3f4cbc3126 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 @@ -117,6 +117,7 @@ class OpenGroupPoller(private val server: String, private val executorService: S ) { val storage = MessagingModuleConfiguration.shared.storage val groupId = "$server.$roomToken" + val dbGroupId = GroupUtil.getEncodedOpenGroupID(groupId.toByteArray()) val existingOpenGroup = storage.getOpenGroup(roomToken, server) val publicKey = existingOpenGroup?.publicKey ?: return @@ -157,8 +158,19 @@ class OpenGroupPoller(private val server: String, private val executorService: S }) } - // Start downloading the room image (if we don't have one or it's been updated) - if (pollInfo.details?.imageId != null && pollInfo.details.imageId != existingOpenGroup.imageId) { + if ( + ( + pollInfo.details != null && + pollInfo.details.imageId != null && ( + pollInfo.details.imageId != existingOpenGroup.imageId || + !storage.hasDownloadedProfilePicture(dbGroupId) + ) + ) || ( + pollInfo.details == null && + existingOpenGroup.imageId != null && + !storage.hasDownloadedProfilePicture(dbGroupId) + ) + ) { JobQueue.shared.add(GroupAvatarDownloadJob(roomToken, server)) } } From 86b065203fdff832980d02a8f8d8609ec0604d62 Mon Sep 17 00:00:00 2001 From: 0x330a <92654767+0x330a@users.noreply.github.com> Date: Mon, 23 Jan 2023 15:43:43 +1100 Subject: [PATCH 5/7] fix: prevent very old messages (15 minutes ago) being processed to prevent endless crashes in some cases --- .../securesms/webrtc/CallMessageProcessor.kt | 12 ++++++++++++ .../java/org/session/libsession/snode/SnodeAPI.kt | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt index f007ace976..bace754843 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt @@ -12,6 +12,7 @@ import org.session.libsession.database.StorageProtocol import org.session.libsession.messaging.calls.CallMessageType import org.session.libsession.messaging.messages.control.CallMessage import org.session.libsession.messaging.utilities.WebRtcUtils +import org.session.libsession.snode.SnodeAPI import org.session.libsession.utilities.Address import org.session.libsession.utilities.TextSecurePreferences import org.session.libsession.utilities.recipients.Recipient @@ -29,6 +30,10 @@ import org.webrtc.IceCandidate class CallMessageProcessor(private val context: Context, private val textSecurePreferences: TextSecurePreferences, lifecycle: Lifecycle, private val storage: StorageProtocol) { + companion object { + private const val VERY_EXPIRED_TIME = 15 * 60 * 1000L + } + init { lifecycle.coroutineScope.launch(IO) { while (isActive) { @@ -53,6 +58,13 @@ class CallMessageProcessor(private val context: Context, private val textSecureP } continue } + + val isVeryExpired = (nextMessage.sentTimestamp?:0) + VERY_EXPIRED_TIME < SnodeAPI.nowWithOffset + if (isVeryExpired) { + Log.e("Loki", "Dropping very expired call message") + continue + } + when (nextMessage.type) { OFFER -> incomingCall(nextMessage) ANSWER -> incomingAnswer(nextMessage) diff --git a/libsession/src/main/java/org/session/libsession/snode/SnodeAPI.kt b/libsession/src/main/java/org/session/libsession/snode/SnodeAPI.kt index 9a17958952..8077594643 100644 --- a/libsession/src/main/java/org/session/libsession/snode/SnodeAPI.kt +++ b/libsession/src/main/java/org/session/libsession/snode/SnodeAPI.kt @@ -56,6 +56,10 @@ object SnodeAPI { * user's clock is incorrect. */ internal var clockOffset = 0L + + val nowWithOffset + get() = System.currentTimeMillis() + clockOffset + internal var forkInfo by observable(database.getForkInfo()) { _, oldValue, newValue -> if (newValue > oldValue) { Log.d("Loki", "Setting new fork info new: $newValue, old: $oldValue") From ebe8479e4c510d1479f63eaf7dfbd1547e0caca6 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 23 Jan 2023 16:14:50 +1100 Subject: [PATCH 6/7] Resolved PR comments --- app/src/main/res/values/strings.xml | 2 +- .../session/libsession/messaging/jobs/BatchMessageReceiveJob.kt | 1 - .../main/java/org/session/libsession/messaging/jobs/JobQueue.kt | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 81e6ae3251..50a1f04572 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -865,5 +865,5 @@ Navigate Back Close Dialog Database Upgrade Failed - Please contact support to report the error and then install an older version to continue using Session. + Please contact support to report the error. diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt index 7bf330fe3b..38a193b8d2 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/BatchMessageReceiveJob.kt @@ -97,7 +97,6 @@ class BatchMessageReceiveJob( when (e) { is MessageReceiver.Error.DuplicateMessage, MessageReceiver.Error.SelfSend -> { Log.i(TAG, "Couldn't receive message, failed with error: ${e.message}") - failures += messageParameters } is MessageReceiver.Error -> { if (!e.isRetryable) { diff --git a/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt b/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt index f4d71fadf5..8e46f275f2 100644 --- a/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt +++ b/libsession/src/main/java/org/session/libsession/messaging/jobs/JobQueue.kt @@ -26,7 +26,7 @@ class JobQueue : JobDelegate { private val jobTimestampMap = ConcurrentHashMap() private val rxDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() private val rxMediaDispatcher = Executors.newFixedThreadPool(4).asCoroutineDispatcher() - private val openGroupDispatcher = Executors.newFixedThreadPool(8).asCoroutineDispatcher()//Executors.newCachedThreadPool().asCoroutineDispatcher() + private val openGroupDispatcher = Executors.newFixedThreadPool(8).asCoroutineDispatcher() private val txDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() private val scope = CoroutineScope(Dispatchers.Default) + SupervisorJob() private val queue = Channel(UNLIMITED) From e0785c485472c9edd07c0745d0fe03c0c23254f3 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 24 Jan 2023 13:31:56 +1100 Subject: [PATCH 7/7] Added in fixes and defensive coding for the most frequent crashes Fixed a crash which could occur when dealing will calls in the background on Android 12 and newer Fixed a crash when we don't have permission to check the current call state Fixed a crash when the ScreenshotObserver receives an invalid Uri (just prevent the specific case) --- .../attachments/ScreenshotObserver.kt | 5 +++++ .../securesms/webrtc/CallManager.kt | 20 +++++++++++++++++-- .../securesms/webrtc/CallMessageProcessor.kt | 9 ++++----- 3 files changed, 27 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/attachments/ScreenshotObserver.kt b/app/src/main/java/org/thoughtcrime/securesms/attachments/ScreenshotObserver.kt index 94c7517eb0..84a9b6cfc3 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/attachments/ScreenshotObserver.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/attachments/ScreenshotObserver.kt @@ -13,6 +13,11 @@ class ScreenshotObserver(private val context: Context, handler: Handler, private override fun onChange(selfChange: Boolean, uri: Uri?) { super.onChange(selfChange, uri) uri ?: return + + // There is an odd bug where we can get notified for changes to 'content://media/external' + // directly which is a protected folder, this code is to prevent that crash + if (uri.scheme == "content" && uri.host == "media" && uri.path == "/external") { return } + if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { queryRelativeDataColumn(uri) } else { diff --git a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallManager.kt b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallManager.kt index 006da2b63e..b7a9b6fd65 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallManager.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallManager.kt @@ -1,7 +1,9 @@ package org.thoughtcrime.securesms.webrtc import android.content.Context +import android.content.pm.PackageManager import android.telephony.TelephonyManager +import androidx.core.content.ContextCompat import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asSharedFlow import kotlinx.serialization.json.Json @@ -176,8 +178,22 @@ class CallManager(context: Context, audioManager: AudioManagerCompat, private va _callStateEvents.value = newState } - fun isBusy(context: Context, callId: UUID) = callId != this.callId && (currentConnectionState != CallState.Idle - || context.getSystemService(TelephonyManager::class.java).callState != TelephonyManager.CALL_STATE_IDLE) + fun isBusy(context: Context, callId: UUID): Boolean { + // Make sure we have the permission before accessing the callState + if (ContextCompat.checkSelfPermission(context, android.Manifest.permission.READ_PHONE_STATE) == PackageManager.PERMISSION_GRANTED) { + return ( + callId != this.callId && ( + currentConnectionState != CallState.Idle || + context.getSystemService(TelephonyManager::class.java).callState != TelephonyManager.CALL_STATE_IDLE + ) + ) + } + + return ( + callId != this.callId && + currentConnectionState != CallState.Idle + ) + } fun isPreOffer() = currentConnectionState == CallState.RemotePreOffer diff --git a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt index f007ace976..a85b1bcbe1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/webrtc/CallMessageProcessor.kt @@ -78,7 +78,7 @@ class CallMessageProcessor(private val context: Context, private val textSecureP private fun incomingHangup(callMessage: CallMessage) { val callId = callMessage.callId ?: return val hangupIntent = WebRtcCallService.remoteHangupIntent(context, callId) - ContextCompat.startForegroundService(context, hangupIntent) + context.startService(hangupIntent) } private fun incomingAnswer(callMessage: CallMessage) { @@ -91,7 +91,7 @@ class CallMessageProcessor(private val context: Context, private val textSecureP sdp = sdp, callId = callId ) - ContextCompat.startForegroundService(context, answerIntent) + context.startService(answerIntent) } private fun handleIceCandidates(callMessage: CallMessage) { @@ -120,7 +120,7 @@ class CallMessageProcessor(private val context: Context, private val textSecureP callId = callId, callTime = callMessage.sentTimestamp!! ) - ContextCompat.startForegroundService(context, incomingIntent) + context.startService(incomingIntent) } private fun incomingCall(callMessage: CallMessage) { @@ -134,8 +134,7 @@ class CallMessageProcessor(private val context: Context, private val textSecureP callId = callId, callTime = callMessage.sentTimestamp!! ) - ContextCompat.startForegroundService(context, incomingIntent) - + context.startService(incomingIntent) } private fun CallMessage.iceCandidates(): List {