Drop group messages that don't change group

js/background.js

@@ -1614,6 +1614,7 @@
       mySignalingKey,
       options
     );
+    window.textsecure.messageReceiver = messageReceiver;
 
     function addQueuedEventListener(name, handler) {
       messageReceiver.addEventListener(name, (...args) =>

js/models/messages.js

@@ -1881,22 +1881,6 @@
         };
       }
 
-      let group = this.get('group_update');
-      if (group && group.avatar) {
-        window.log.info(
-          `Queueing group avatar download for message ${this.idForLogging()}`
-        );
-        count += 1;
-        group = {
-          ...group,
-          avatar: await window.Signal.AttachmentDownloads.addJob(group.avatar, {
-            messageId,
-            type: 'group-avatar',
-            index: 0,
-          }),
-        };
-      }
-
       let sticker = this.get('sticker');
       if (sticker) {
         window.log.info(
@@ -1957,7 +1941,6 @@
           preview,
           contact,
           quote,
-          group_update: group,
           sticker,
         });
 
@@ -2259,7 +2242,7 @@
               ...conversation.attributes,
             };
             if (dataMessage.group) {
-              let groupUpdate = null;
+              const pendingGroupUpdate = [];
               const memberConversations = await Promise.all(
                 (
                   dataMessage.group.members || dataMessage.group.membersE164
@@ -2289,20 +2272,83 @@
                   members: _.union(members, conversation.get('members')),
                 };
 
-                groupUpdate = {};
                 if (dataMessage.group.name !== conversation.get('name')) {
-                  groupUpdate.name = dataMessage.group.name;
+                  pendingGroupUpdate.push(['name', dataMessage.group.name]);
                 }
 
-                // Note: used and later cleared by background attachment downloader
+                const avatarAttachment = dataMessage.group.avatar;
-                groupUpdate.avatar = dataMessage.group.avatar;
+
+                let downloadedAvatar;
+                let hash;
+                if (avatarAttachment) {
+                  try {
+                    downloadedAvatar = await window.Signal.Util.downloadAttachment(
+                      avatarAttachment
+                    );
+
+                    if (downloadedAvatar) {
+                      const loadedAttachment = await Signal.Migrations.loadAttachmentData(
+                        downloadedAvatar
+                      );
+
+                      hash = await Signal.Types.Conversation.computeHash(
+                        loadedAttachment.data
+                      );
+                    }
+                  } catch (err) {
+                    window.log.info(
+                      'handleDataMessage: group avatar download failed'
+                    );
+                  }
+                }
+
+                const existingAvatar = conversation.get('avatar');
+
+                if (
+                  // Avatar added
+                  !existingAvatar ||
+                  // Avatar changed
+                  (existingAvatar && existingAvatar.hash !== hash) ||
+                  // Avatar removed
+                  avatarAttachment === null
+                ) {
+                  // Removes existing avatar from disk
+                  if (existingAvatar && existingAvatar.path) {
+                    await Signal.Migrations.deleteAttachmentData(
+                      existingAvatar.path
+                    );
+                  }
+
+                  let avatar = null;
+                  if (downloadedAvatar && avatarAttachment !== null) {
+                    const onDiskAttachment = await window.Signal.Types.Attachment.migrateDataToFileSystem(
+                      downloadedAvatar,
+                      {
+                        writeNewAttachmentData:
+                          window.Signal.Migrations.writeNewAttachmentData,
+                      }
+                    );
+                    avatar = {
+                      ...onDiskAttachment,
+                      hash,
+                    };
+                  }
+
+                  attributes.avatar = avatar;
+
+                  pendingGroupUpdate.push(['avatarUpdated', true]);
+                } else {
+                  window.log.info(
+                    'handleDataMessage: Group avatar hash matched; not replacing group avatar'
+                  );
+                }
 
                 const difference = _.difference(
                   members,
                   conversation.get('members')
                 );
                 if (difference.length > 0) {
-                  groupUpdate.joined = difference;
+                  pendingGroupUpdate.push(['joined', difference]);
                 }
                 if (conversation.get('left')) {
                   window.log.warn('re-added to a left group');
@@ -2324,9 +2370,9 @@
 
                 if (sender.isMe()) {
                   attributes.left = true;
-                  groupUpdate = { left: 'You' };
+                  pendingGroupUpdate.push(['left', 'You']);
                 } else {
-                  groupUpdate = { left: sender.get('id') };
+                  pendingGroupUpdate.push(['left', sender.get('id')]);
                 }
                 attributes.members = _.without(
                   conversation.get('members'),
@@ -2334,7 +2380,14 @@
                 );
               }
 
-              if (groupUpdate !== null) {
+              if (pendingGroupUpdate.length) {
+                const groupUpdate = pendingGroupUpdate.reduce(
+                  (acc, [key, value]) => {
+                    acc[key] = value;
+                    return acc;
+                  },
+                  {}
+                );
                 message.set({ group_update: groupUpdate });
               }
             }

js/modules/attachment_downloads.js

@@ -1,5 +1,4 @@
 /* global
-  ConversationController,
   Whisper,
   Signal,
   setTimeout,
@@ -8,7 +7,6 @@
 */
 
 const { isFunction, isNumber, omit } = require('lodash');
-const { computeHash } = require('./types/conversation');
 const getGuid = require('uuid/v4');
 const {
   getMessageById,
@@ -19,6 +17,7 @@ const {
   saveMessage,
   setAttachmentDownloadJobPending,
 } = require('../../ts/sql/Client').default;
+const { downloadAttachment } = require('../../ts/util/downloadAttachment');
 const { stringFromBytes } = require('../../ts/Crypto');
 
 module.exports = {
@@ -186,38 +185,28 @@ async function _runJob(job) {
     const pending = true;
     await setAttachmentDownloadJobPending(id, pending);
 
-    let downloaded;
     const messageReceiver = getMessageReceiver();
     if (!messageReceiver) {
       throw new Error('_runJob: messageReceiver not found');
     }
 
-    try {
+    const downloaded = await downloadAttachment(attachment);
-      if (attachment.id) {
-        // eslint-disable-next-line no-param-reassign
-        attachment.cdnId = attachment.id;
-      }
-      downloaded = await messageReceiver.downloadAttachment(attachment);
-    } catch (error) {
-      // Attachments on the server expire after 30 days, then start returning 404
-      if (error && error.code === 404) {
-        logger.warn(
-          `_runJob: Got 404 from server for CDN ${
-            attachment.cdnNumber
-          }, marking attachment ${attachment.cdnId ||
-            attachment.cdnKey} from message ${message.idForLogging()} as permanent error`
-        );
 
-        await _finishJob(message, id);
+    if (!downloaded) {
-        await _addAttachmentToMessage(
+      logger.warn(
-          message,
+        `_runJob: Got 404 from server for CDN ${
-          _markAttachmentAsError(attachment),
+          attachment.cdnNumber
-          { type, index }
+        }, marking attachment ${attachment.cdnId ||
-        );
+          attachment.cdnKey} from message ${message.idForLogging()} as permanent error`
+      );
 
-        return;
+      await _finishJob(message, id);
-      }
+      await _addAttachmentToMessage(
-      throw error;
+        message,
+        _markAttachmentAsError(attachment),
+        { type, index }
+      );
+      return;
     }
 
     const upgradedAttachment = await Signal.Migrations.processNewAttachment(
@@ -425,50 +414,6 @@ async function _addAttachmentToMessage(message, attachment, { type, index }) {
     return;
   }
 
-  if (type === 'group-avatar') {
-    const conversationId = message.get('conversationId');
-    const conversation = ConversationController.get(conversationId);
-    if (!conversation) {
-      logger.warn("_addAttachmentToMessage: conversation didn't exist");
-      return;
-    }
-
-    const loadedAttachment = await Signal.Migrations.loadAttachmentData(
-      attachment
-    );
-    const hash = await computeHash(loadedAttachment.data);
-    const existingAvatar = conversation.get('avatar');
-
-    if (existingAvatar) {
-      if (existingAvatar.hash === hash) {
-        logger.info(
-          '_addAttachmentToMessage: Group avatar hash matched; not replacing group avatar'
-        );
-        return;
-      }
-
-      await Signal.Migrations.deleteAttachmentData(existingAvatar.path);
-    }
-
-    conversation.set({
-      avatar: {
-        ...attachment,
-        hash,
-      },
-    });
-    Signal.Data.updateConversation(conversation.attributes);
-
-    message.set({
-      group_update: {
-        ...message.get('group_update'),
-        avatar: null,
-        avatarUpdated: true,
-      },
-    });
-
-    return;
-  }
-
   if (type === 'sticker') {
     const sticker = message.get('sticker');
     if (!sticker) {

ts/textsecure.d.ts

@@ -62,6 +62,11 @@ export type TextSecureType = {
     remove: (key: string | Array<string>) => Promise<void>;
     protocol: StorageProtocolType;
   };
+  messageReceiver: {
+    downloadAttachment: (
+      attachment: AttachmentPointerClass
+    ) => Promise<DownloadAttachmentType>;
+  };
   messaging: {
     sendStickerPackSync: (
       operations: Array<{
@@ -172,6 +177,23 @@ export declare class AttachmentPointerClass {
   cdnNumber?: number;
 }
 
+export type DownloadAttachmentType = {
+  data: ArrayBuffer;
+  cdnId?: ProtoBigNumberType;
+  cdnKey?: string;
+  contentType?: string;
+  size?: number;
+  thumbnail?: ProtoBinaryType;
+  fileName?: string;
+  flags?: number;
+  width?: number;
+  height?: number;
+  caption?: string;
+  blurHash?: string;
+  uploadTimestamp?: ProtoBigNumberType;
+  cdnNumber?: number;
+};
+
 export declare class ContactDetailsClass {
   static decode: (
     data: ArrayBuffer | ByteBufferClass,

ts/textsecure/MessageReceiver.ts

@@ -20,8 +20,8 @@ import { IncomingIdentityKeyError } from './Errors';
 import {
   AttachmentPointerClass,
   DataMessageClass,
+  DownloadAttachmentType,
   EnvelopeClass,
-  ProtoBigNumberType,
   ReceiptMessageClass,
   SyncMessageClass,
   TypingMessageClass,
@@ -62,22 +62,6 @@ declare global {
   }
 }
 
-type AttachmentType = {
-  cdnId?: string;
-  cdnKey?: string;
-  data: ArrayBuffer;
-  contentType?: string;
-  size?: number;
-  fileName?: string;
-  flags?: number;
-  width?: number;
-  height?: number;
-  caption?: string;
-  blurHash?: string;
-  uploadTimestamp?: ProtoBigNumberType;
-  cdnNumber?: number;
-};
-
 type CacheAddItemType = {
   envelope: EnvelopeClass;
   data: UnprocessedType;
@@ -1591,7 +1575,9 @@ class MessageReceiverInner extends EventTarget {
       digest: attachment.digest ? attachment.digest.toString('base64') : null,
     };
   }
-  async downloadAttachment(attachment: AttachmentPointerClass) {
+  async downloadAttachment(
+    attachment: AttachmentPointerClass
+  ): Promise<DownloadAttachmentType> {
     const encrypted = await this.server.getAttachment(
       attachment.cdnId || attachment.cdnKey,
       attachment.cdnNumber || 0
@@ -1623,7 +1609,7 @@ class MessageReceiverInner extends EventTarget {
   }
   async handleAttachment(
     attachment: AttachmentPointerClass
-  ): Promise<AttachmentType> {
+  ): Promise<DownloadAttachmentType> {
     const cleaned = this.cleanAttachment(attachment);
     return this.downloadAttachment(cleaned);
   }
@@ -1866,7 +1852,7 @@ export default class MessageReceiver {
   close: () => Promise<void>;
   downloadAttachment: (
     attachment: AttachmentPointerClass
-  ) => Promise<AttachmentType>;
+  ) => Promise<DownloadAttachmentType>;
   stopProcessing: () => Promise<void>;
   unregisterBatchers: () => void;
 

ts/util/downloadAttachment.ts

@@ -0,0 +1,32 @@
+import {
+  AttachmentPointerClass,
+  DownloadAttachmentType,
+} from '../textsecure.d';
+
+type AttachmentData = AttachmentPointerClass & {
+  id?: string;
+};
+
+export async function downloadAttachment(
+  attachmentData: AttachmentData
+): Promise<DownloadAttachmentType | null> {
+  let downloaded;
+  try {
+    if (attachmentData.id) {
+      // eslint-disable-next-line no-param-reassign
+      attachmentData.cdnId = attachmentData.id;
+    }
+    downloaded = await window.textsecure.messageReceiver.downloadAttachment(
+      attachmentData
+    );
+  } catch (error) {
+    // Attachments on the server expire after 30 days, then start returning 404
+    if (error && error.code === 404) {
+      return null;
+    }
+
+    throw error;
+  }
+
+  return downloaded;
+}

ts/util/index.ts

@@ -4,6 +4,7 @@ import { arrayBufferToObjectURL } from './arrayBufferToObjectURL';
 import { combineNames } from './combineNames';
 import { createBatcher } from './batcher';
 import { createWaitBatcher } from './waitBatcher';
+import { downloadAttachment } from './downloadAttachment';
 import { hasExpired } from './hasExpired';
 import { isFileDangerous } from './isFileDangerous';
 import { makeLookup } from './makeLookup';
@@ -16,6 +17,7 @@ export {
   combineNames,
   createBatcher,
   createWaitBatcher,
+  downloadAttachment,
   GoogleChrome,
   hasExpired,
   isFileDangerous,