Improve reliability of out-of-order reactions and DOE

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);

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 => {
+        // Do not await, since this can deadlock the queue
-          const messageContact = m.getContact();
+        fromContact.queueJob(async () => {
-
+          const messages = await window.Signal.Data.getMessagesBySentAt(
-          if (!messageContact) {
+            del.get('targetSentTimestamp'),
-            return false;
+            {
-          }
+              MessageCollection: Whisper.MessageCollection,
-
+            }
-          // 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;
+          const targetMessage = messages.find(m => {
-        }
+            const messageContact = m.getContact();
 
-        // Make sure the server timestamps for the DOE and the matching message
+            if (!messageContact) {
-        // are less than one day apart
+              return false;
-        const delta = Math.abs(
+            }
-          del.get('serverTimestamp') - targetMessage.get('serverTimestamp')
+
-        );
+            // Find messages which are from the same contact who sent the DOE
-        if (delta > ONE_DAY) {
+            return messageContact.get('id') === fromContact.get('id');
-          window.log.info('Received late DOE. Dropping.', {
-            fromId: del.get('fromId'),
-            targetSentTimestamp: del.get('targetSentTimestamp'),
-            messageServerTimestamp: message.get('serverTimestamp'),
-            deleteServerTimestamp: del.get('serverTimestamp'),
           });
+
+          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:',

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, {
+      if (shouldPersist) {
-        Message: Whisper.Message,
+        await window.Signal.Data.saveMessage(this.attributes, {
-      });
+          Message: Whisper.Message,
+        });
+      }
     },
 
     async handleDeleteForEveryone(del) {

js/reactions.js

@@ -46,59 +46,74 @@
     },
     async onReaction(reaction) {
       try {
-        const messages = await window.Signal.Data.getMessagesBySentAt(
+        const targetConversation = await ConversationController.getConversationForTargetMessage(
-          reaction.get('targetTimestamp'),
+          // Do not use ensureContactIds here since maliciously malformed
-          {
+          // reactions from clients could cause issues
-            MessageCollection: Whisper.MessageCollection,
+          reaction.get('targetAuthorE164') || reaction.get('targetAuthorUuid'),
-          }
+          reaction.get('targetTimestamp')
         );
-
+        if (!targetConversation) {
-        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));
-          }
-
           return;
         }
 
-        const message = MessageController.register(
+        // awaiting is safe since `onReaction` is never called from inside the queue
-          targetMessage.id,
+        await targetConversation.queueJob(async () => {
-          targetMessage
+          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:',

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"
   }
-]
+]