From 4289c28a38b5908684590482ff1c5eed523bec31 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Fri, 10 Jul 2020 11:24:58 -0700 Subject: [PATCH] Improve reliability of out-of-order reactions and DOE --- js/conversation_controller.js | 34 +++++++++++ js/deletes.js | 99 +++++++++++++++--------------- js/models/messages.js | 33 +++++----- js/reactions.js | 109 +++++++++++++++++++--------------- ts/util/lint/exceptions.json | 10 ++-- 5 files changed, 170 insertions(+), 115 deletions(-) diff --git a/js/conversation_controller.js b/js/conversation_controller.js index faa616f57dc1..490ea3de5d1a 100644 --- a/js/conversation_controller.js +++ b/js/conversation_controller.js @@ -186,6 +186,40 @@ const uuid = textsecure.storage.user.getUuid(); return this.getConversationId(e164 || uuid); }, + /** + * Given certain metadata about a message (an identifier of who wrote the + * message and the sent_at timestamp of the message) returns the + * conversation the message belongs to OR null if a conversation isn't + * found. + * @param {string} targetFrom The E164, UUID, or Conversation ID of the message author + * @param {number} targetTimestamp The sent_at timestamp of the target message + */ + async getConversationForTargetMessage(targetFrom, targetTimestamp) { + const targetFromId = this.getConversationId(targetFrom); + + const messages = await window.Signal.Data.getMessagesBySentAt( + targetTimestamp, + { + MessageCollection: Whisper.MessageCollection, + } + ); + const targetMessage = messages.find(m => { + const contact = m.getContact(); + + if (!contact) { + return false; + } + + const mcid = contact.get('id'); + return mcid === targetFromId; + }); + + if (targetMessage) { + return targetMessage.getConversation(); + } + + return null; + }, prepareForSend(id, options) { // id is any valid conversation identifier const conversation = this.get(id); diff --git a/js/deletes.js b/js/deletes.js index 9b7f76766053..95c06b0735cd 100644 --- a/js/deletes.js +++ b/js/deletes.js @@ -31,13 +31,6 @@ }, async onDelete(del) { try { - const messages = await window.Signal.Data.getMessagesBySentAt( - del.get('targetSentTimestamp'), - { - MessageCollection: Whisper.MessageCollection, - } - ); - // The contact the delete message came from const fromContact = ConversationController.get(del.get('fromId')); @@ -51,52 +44,62 @@ return; } - const targetMessage = messages.find(m => { - const messageContact = m.getContact(); - - if (!messageContact) { - return false; - } - - // Find messages which are from the same contact who sent the DOE - return messageContact.get('id') === fromContact.get('id'); - }); - - if (!targetMessage) { - window.log.info( - 'No message for DOE', - del.get('fromId'), - del.get('targetSentTimestamp') + // Do not await, since this can deadlock the queue + fromContact.queueJob(async () => { + const messages = await window.Signal.Data.getMessagesBySentAt( + del.get('targetSentTimestamp'), + { + MessageCollection: Whisper.MessageCollection, + } ); - return; - } + const targetMessage = messages.find(m => { + const messageContact = m.getContact(); - // Make sure the server timestamps for the DOE and the matching message - // are less than one day apart - const delta = Math.abs( - del.get('serverTimestamp') - targetMessage.get('serverTimestamp') - ); - if (delta > ONE_DAY) { - window.log.info('Received late DOE. Dropping.', { - fromId: del.get('fromId'), - targetSentTimestamp: del.get('targetSentTimestamp'), - messageServerTimestamp: message.get('serverTimestamp'), - deleteServerTimestamp: del.get('serverTimestamp'), + if (!messageContact) { + return false; + } + + // Find messages which are from the same contact who sent the DOE + return messageContact.get('id') === fromContact.get('id'); }); + + if (!targetMessage) { + window.log.info( + 'No message for DOE', + del.get('fromId'), + del.get('targetSentTimestamp') + ); + + return; + } + + // Make sure the server timestamps for the DOE and the matching message + // are less than one day apart + const delta = Math.abs( + del.get('serverTimestamp') - targetMessage.get('serverTimestamp') + ); + if (delta > ONE_DAY) { + window.log.info('Received late DOE. Dropping.', { + fromId: del.get('fromId'), + targetSentTimestamp: del.get('targetSentTimestamp'), + messageServerTimestamp: message.get('serverTimestamp'), + deleteServerTimestamp: del.get('serverTimestamp'), + }); + this.remove(del); + + return; + } + + const message = MessageController.register( + targetMessage.id, + targetMessage + ); + + await message.handleDeleteForEveryone(del); + this.remove(del); - - return; - } - - const message = MessageController.register( - targetMessage.id, - targetMessage - ); - - await message.handleDeleteForEveryone(del); - - this.remove(del); + }); } catch (error) { window.log.error( 'Deletes.onDelete error:', diff --git a/js/models/messages.js b/js/models/messages.js index cf3b021fbbd6..a3303211a9bd 100644 --- a/js/models/messages.js +++ b/js/models/messages.js @@ -2520,6 +2520,18 @@ window.Signal.Data.updateConversation(conversation.attributes); await message.queueAttachmentDownloads(); + + // Does this message have any pending, previously-received associated reactions? + const reactions = Whisper.Reactions.forMessage(message); + reactions.forEach(reaction => { + message.handleReaction(reaction, false); + }); + + // Does this message have any pending, previously-received associated + // delete for everyone messages? + const deletes = Whisper.Deletes.forMessage(message); + deletes.forEach(del => Whisper.Deletes.onDelete(del, false)); + await window.Signal.Data.saveMessage(message.attributes, { Message: Whisper.Message, forceSave: true, @@ -2531,17 +2543,6 @@ await conversation.notify(message); } - // Does this message have any pending, previously-received associated reactions? - const reactions = Whisper.Reactions.forMessage(message); - reactions.forEach(reaction => { - message.handleReaction(reaction); - }); - - // Does this message have any pending, previously-received associated - // delete for everyone messages? - const deletes = Whisper.Deletes.forMessage(message); - deletes.forEach(del => Whisper.Deletes.onDelete(del)); - Whisper.events.trigger('incrementProgress'); confirm(); } catch (error) { @@ -2557,7 +2558,7 @@ }); }, - async handleReaction(reaction) { + async handleReaction(reaction, shouldPersist = true) { if (this.get('deletedForEveryone')) { return; } @@ -2597,9 +2598,11 @@ `Done processing reaction for message ${messageId}. Went from ${count} to ${newCount} reactions.` ); - await window.Signal.Data.saveMessage(this.attributes, { - Message: Whisper.Message, - }); + if (shouldPersist) { + await window.Signal.Data.saveMessage(this.attributes, { + Message: Whisper.Message, + }); + } }, async handleDeleteForEveryone(del) { diff --git a/js/reactions.js b/js/reactions.js index 60d586424e45..c6da4d3cb4ea 100644 --- a/js/reactions.js +++ b/js/reactions.js @@ -46,59 +46,74 @@ }, async onReaction(reaction) { try { - const messages = await window.Signal.Data.getMessagesBySentAt( - reaction.get('targetTimestamp'), - { - MessageCollection: Whisper.MessageCollection, - } + const targetConversation = await ConversationController.getConversationForTargetMessage( + // Do not use ensureContactIds here since maliciously malformed + // reactions from clients could cause issues + reaction.get('targetAuthorE164') || reaction.get('targetAuthorUuid'), + reaction.get('targetTimestamp') ); - - const targetMessage = messages.find(m => { - const contact = m.getContact(); - - if (!contact) { - return false; - } - - const mcid = contact.get('id'); - const recid = ConversationController.getConversationId( - reaction.get('targetAuthorE164') || reaction.get('targetAuthorUuid') - ); - return mcid === recid; - }); - - if (!targetMessage) { - window.log.info( - 'No message for reaction', - reaction.get('targetAuthorE164'), - reaction.get('targetAuthorUuid'), - reaction.get('targetTimestamp') - ); - - // Since we haven't received the message for which we are removing a - // reaction, we can just remove those pending reaction - if (reaction.get('remove')) { - this.remove(reaction); - const oldReaction = this.where({ - targetAuthorE164: reaction.get('targetAuthorE164'), - targetAuthorUuid: reaction.get('targetAuthorUuid'), - targetTimestamp: reaction.get('targetTimestamp'), - emoji: reaction.get('emoji'), - }); - oldReaction.forEach(r => this.remove(r)); - } - + if (!targetConversation) { return; } - const message = MessageController.register( - targetMessage.id, - targetMessage - ); + // awaiting is safe since `onReaction` is never called from inside the queue + await targetConversation.queueJob(async () => { + const messages = await window.Signal.Data.getMessagesBySentAt( + reaction.get('targetTimestamp'), + { + MessageCollection: Whisper.MessageCollection, + } + ); + // Message is fetched inside the conversation queue so we have the + // most recent data + const targetMessage = messages.find(m => { + const contact = m.getContact(); - await message.handleReaction(reaction); + if (!contact) { + return false; + } - this.remove(reaction); + const mcid = contact.get('id'); + const recid = ConversationController.getConversationId( + reaction.get('targetAuthorE164') || + reaction.get('targetAuthorUuid') + ); + return mcid === recid; + }); + + if (!targetMessage) { + window.log.info( + 'No message for reaction', + reaction.get('targetAuthorE164'), + reaction.get('targetAuthorUuid'), + reaction.get('targetTimestamp') + ); + + // Since we haven't received the message for which we are removing a + // reaction, we can just remove those pending reactions + if (reaction.get('remove')) { + this.remove(reaction); + const oldReaction = this.where({ + targetAuthorE164: reaction.get('targetAuthorE164'), + targetAuthorUuid: reaction.get('targetAuthorUuid'), + targetTimestamp: reaction.get('targetTimestamp'), + emoji: reaction.get('emoji'), + }); + oldReaction.forEach(r => this.remove(r)); + } + + return; + } + + const message = MessageController.register( + targetMessage.id, + targetMessage + ); + + await message.handleReaction(reaction); + + this.remove(reaction); + }); } catch (error) { window.log.error( 'Reactions.onReaction error:', diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index f864fa540432..9dc72ba74104 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -164,17 +164,17 @@ "rule": "jQuery-load(", "path": "js/conversation_controller.js", "line": " async load() {", - "lineNumber": 218, + "lineNumber": 252, "reasonCategory": "falseMatch", - "updated": "2020-03-24T20:06:31.391Z" + "updated": "2020-06-19T18:29:40.067Z" }, { "rule": "jQuery-load(", "path": "js/conversation_controller.js", "line": " this._initialPromise = load();", - "lineNumber": 260, + "lineNumber": 294, "reasonCategory": "falseMatch", - "updated": "2020-03-24T20:06:31.391Z" + "updated": "2020-06-19T18:29:40.067Z" }, { "rule": "jQuery-$(", @@ -11784,4 +11784,4 @@ "reasonCategory": "falseMatch", "updated": "2020-04-05T23:45:16.746Z" } -] +] \ No newline at end of file