Reliability fixes for conversation formatting and message send

test/models/messages_test.js

@@ -41,7 +41,6 @@ describe('Message', () => {
       const fakeDataMessage = new ArrayBuffer(0);
       const result = {
         dataMessage: fakeDataMessage,
-        discoveredIdentifierPairs: [],
       };
       const promise = Promise.resolve(result);
       await message.send(promise);
@@ -52,11 +51,7 @@ describe('Message', () => {
     it('updates the `sent` attribute', async () => {
       const message = createMessage({ type: 'outgoing', source, sent: false });
 
-      await message.send(
+      await message.send(Promise.resolve({}));
-        Promise.resolve({
-          discoveredIdentifierPairs: [],
-        })
-      );
 
       assert.isTrue(message.get('sent'));
     });
@@ -69,11 +64,7 @@ describe('Message', () => {
         callCount += 1;
       });
 
-      await message.send(
+      await message.send(Promise.resolve({}));
-        Promise.resolve({
-          discoveredIdentifierPairs: [],
-        })
-      );
 
       assert.strictEqual(callCount, 1);
     });
@@ -86,11 +77,7 @@ describe('Message', () => {
         calls.push(args);
       });
 
-      await message.send(
+      await message.send(Promise.resolve({}));
-        Promise.resolve({
-          discoveredIdentifierPairs: [],
-        })
-      );
 
       assert.lengthOf(calls, 1);
       assert.strictEqual(calls[0][0], message);
@@ -125,7 +112,6 @@ describe('Message', () => {
 
       const result = {
         errors: [new Error('baz qux')],
-        discoveredIdentifierPairs: [],
       };
       const promise = Promise.reject(result);
       await message.send(promise);

ts/ConversationController.ts

@@ -655,7 +655,14 @@ export class ConversationController {
     const groups = await getAllGroupsInvolvingId(conversationId, {
       ConversationCollection: window.Whisper.ConversationCollection,
     });
-    return groups.map(group => this._conversations.add(group));
+    return groups.map(group => {
+      const existing = this.get(group.id);
+      if (existing) {
+        return existing;
+      }
+
+      return this._conversations.add(group);
+    });
   }
 
   async loadPromise(): Promise<void> {

ts/components/ConversationListItem.stories.tsx

@@ -24,9 +24,11 @@ story.addDecorator(storyFn => (
 const createProps = (overrideProps: Partial<Props> = {}): Props => ({
   ...overrideProps,
   i18n,
-  isAccepted: boolean(
+  acceptedMessageRequest: boolean(
-    'isAccepted',
+    'acceptedMessageRequest',
-    overrideProps.isAccepted !== undefined ? overrideProps.isAccepted : true
+    overrideProps.acceptedMessageRequest !== undefined
+      ? overrideProps.acceptedMessageRequest
+      : true
   ),
   isMe: boolean('isMe', overrideProps.isMe || false),
   avatarPath: text('avatarPath', overrideProps.avatarPath || ''),
@@ -103,7 +105,7 @@ story.add('Typing Status', () => {
 
 story.add('Message Request', () => {
   const props = createProps({
-    isAccepted: false,
+    acceptedMessageRequest: false,
     lastMessage: {
       text: 'A Message',
       status: 'delivered',

ts/components/ConversationListItem.tsx

@@ -39,7 +39,7 @@ export type PropsData = {
   unreadCount?: number;
   isSelected: boolean;
 
-  isAccepted?: boolean;
+  acceptedMessageRequest?: boolean;
   draftPreview?: string;
   shouldShowDraft?: boolean;
 
@@ -167,7 +167,7 @@ export class ConversationListItem extends React.PureComponent<Props> {
     const {
       draftPreview,
       i18n,
-      isAccepted,
+      acceptedMessageRequest,
       lastMessage,
       muteExpiresAt,
       shouldShowDraft,
@@ -209,7 +209,7 @@ export class ConversationListItem extends React.PureComponent<Props> {
           {muteExpiresAt && Date.now() < muteExpiresAt && (
             <span className="module-conversation-list-item__muted" />
           )}
-          {!isAccepted ? (
+          {!acceptedMessageRequest ? (
             <span className="module-conversation-list-item__message-request">
               {i18n('ConversationListItem--message-request')}
             </span>

ts/components/conversation/ConversationHeader.stories.tsx

@@ -70,7 +70,7 @@ const stories: Array<ConversationHeaderStory> = [
           type: 'direct',
           id: '1',
           profileName: '🔥Flames🔥',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -85,7 +85,7 @@ const stories: Array<ConversationHeaderStory> = [
           phoneNumber: '(202) 555-0002',
           type: 'direct',
           id: '2',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -100,7 +100,7 @@ const stories: Array<ConversationHeaderStory> = [
           phoneNumber: '(202) 555-0002',
           type: 'direct',
           id: '2',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -115,7 +115,7 @@ const stories: Array<ConversationHeaderStory> = [
           id: '3',
           title: '🔥Flames🔥',
           profileName: '🔥Flames🔥',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -127,7 +127,7 @@ const stories: Array<ConversationHeaderStory> = [
           phoneNumber: '(202) 555-0011',
           type: 'direct',
           id: '11',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -141,7 +141,7 @@ const stories: Array<ConversationHeaderStory> = [
           title: '(202) 555-0004',
           type: 'direct',
           id: '4',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -165,7 +165,7 @@ const stories: Array<ConversationHeaderStory> = [
               value: 10,
             },
           ],
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -179,7 +179,7 @@ const stories: Array<ConversationHeaderStory> = [
           type: 'direct',
           id: '6',
           muteExpirationLabel: '10/18/3000, 11:11 AM',
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -211,7 +211,7 @@ const stories: Array<ConversationHeaderStory> = [
               value: 10,
             },
           ],
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -237,7 +237,7 @@ const stories: Array<ConversationHeaderStory> = [
               value: 10,
             },
           ],
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -257,7 +257,7 @@ const stories: Array<ConversationHeaderStory> = [
           id: '7',
           type: 'direct',
           isMe: true,
-          isAccepted: true,
+          acceptedMessageRequest: true,
           ...actionProps,
           ...housekeepingProps,
         },
@@ -277,7 +277,7 @@ const stories: Array<ConversationHeaderStory> = [
           id: '7',
           type: 'direct',
           isMe: false,
-          isAccepted: false,
+          acceptedMessageRequest: false,
           ...actionProps,
           ...housekeepingProps,
         },

ts/components/conversation/ConversationHeader.tsx

@@ -31,7 +31,7 @@ export interface PropsDataType {
   type: 'direct' | 'group';
   title: string;
 
-  isAccepted?: boolean;
+  acceptedMessageRequest?: boolean;
   isVerified?: boolean;
   isMe?: boolean;
   isArchived?: boolean;
@@ -305,7 +305,7 @@ export class ConversationHeader extends React.Component<PropsType> {
     const {
       disableTimerChanges,
       i18n,
-      isAccepted,
+      acceptedMessageRequest,
       isMe,
       isPinned,
       type,
@@ -388,7 +388,7 @@ export class ConversationHeader extends React.Component<PropsType> {
             {i18n('showSafetyNumber')}
           </MenuItem>
         ) : null}
-        {!isGroup && isAccepted ? (
+        {!isGroup && acceptedMessageRequest ? (
           <MenuItem onClick={onResetSession}>{i18n('resetSession')}</MenuItem>
         ) : null}
         {isArchived ? (

ts/model-types.d.ts

@@ -147,7 +147,7 @@ export type ConversationAttributesType = {
   draftTimestamp: number | null;
   inbox_position: number;
   isPinned: boolean;
-  lastMessageDeletedForEveryone: unknown;
+  lastMessageDeletedForEveryone: boolean;
   lastMessageStatus: LastMessageStatus | null;
   messageCount: number;
   messageCountBeforeMessageRequests: number;

ts/models/conversations.ts

@@ -80,6 +80,8 @@ export class ConversationModel extends window.Backbone.Model<
 
   cachedProps?: ConversationType | null;
 
+  oldCachedProps?: ConversationType | null;
+
   contactTypingTimers?: Record<
     string,
     { senderId: string; timer: NodeJS.Timer }
@@ -235,6 +237,9 @@ export class ConversationModel extends window.Backbone.Model<
     // We clear our cached props whenever we change so that the next call to format() will
     //   result in refresh via a getProps() call. See format() below.
     this.on('change', () => {
+      if (this.cachedProps) {
+        this.oldCachedProps = this.cachedProps;
+      }
       this.cachedProps = null;
     });
   }
@@ -1020,12 +1025,38 @@ export class ConversationModel extends window.Backbone.Model<
   }
 
   format(): ConversationType {
-    this.cachedProps = this.cachedProps || this.getProps();
+    if (this.cachedProps) {
+      return this.cachedProps;
+    }
+
+    const oldFormat = this.format;
+    // We don't want to crash or have an infinite loop if we loop back into this function
+    //   again. We'll log a warning and returned old cached props or throw an error.
+    this.format = () => {
+      const { stack } = new Error('for stack');
+      window.log.warn(
+        `Conversation.format()/${this.idForLogging()} reentrant call! ${stack}`
+      );
+      if (!this.oldCachedProps) {
+        throw new Error(
+          `Conversation.format()/${this.idForLogging()} reentrant call, no old cached props!`
+        );
+      }
+      return this.oldCachedProps;
+    };
+
+    this.cachedProps = this.getProps();
+
+    this.format = oldFormat;
 
     return this.cachedProps;
   }
 
-  getProps(): ConversationType {
+  // Note: this should never be called directly. Use conversation.format() instead, which
+  //   maintains a cache, and protects against reentrant calls.
+  // Note: When writing code inside this function, do not call .format() on a conversation
+  //   unless you are sure that it's not this very same conversation.
+  private getProps(): ConversationType {
     // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
     const color = this.getColor()!;
 
@@ -1060,7 +1091,7 @@ export class ConversationModel extends window.Backbone.Model<
 
     // TODO: DESKTOP-720
     /* eslint-disable @typescript-eslint/no-non-null-assertion */
-    const result = {
+    const result: ConversationType = {
       id: this.id,
       uuid: this.get('uuid'),
       e164: this.get('e164'),
@@ -1077,7 +1108,6 @@ export class ConversationModel extends window.Backbone.Model<
       firstName: this.get('profileName')!,
       groupVersion,
       inboxPosition,
-      isAccepted: this.getAccepted(),
       isArchived: this.get('isArchived')!,
       isBlocked: this.isBlocked(),
       isMe: this.isMe(),
@@ -1104,9 +1134,17 @@ export class ConversationModel extends window.Backbone.Model<
       timestamp,
       title: this.getTitle()!,
       type: (this.isPrivate() ? 'direct' : 'group') as ConversationTypeType,
-      typingContact: typingContact ? typingContact.format() : null,
       unreadCount: this.get('unreadCount')! || 0,
     };
+
+    if (typingContact) {
+      // We don't want to call .format() on our own conversation
+      if (typingContact.id === this.id) {
+        result.typingContact = result;
+      } else {
+        result.typingContact = typingContact.format();
+      }
+    }
     /* eslint-enable @typescript-eslint/no-non-null-assertion */
 
     return result;
@@ -2710,8 +2748,7 @@ export class ConversationModel extends window.Backbone.Model<
         if (result) {
           await this.handleMessageSendResult(
             result.failoverIdentifiers,
-            result.unidentifiedDeliveries,
+            result.unidentifiedDeliveries
-            result.discoveredIdentifierPairs
           );
         }
         return result;
@@ -2721,8 +2758,7 @@ export class ConversationModel extends window.Backbone.Model<
         if (result) {
           await this.handleMessageSendResult(
             result.failoverIdentifiers,
-            result.unidentifiedDeliveries,
+            result.unidentifiedDeliveries
-            result.discoveredIdentifierPairs
           );
         }
         throw result;
@@ -2732,23 +2768,8 @@ export class ConversationModel extends window.Backbone.Model<
 
   async handleMessageSendResult(
     failoverIdentifiers: Array<string> | undefined,
-    unidentifiedDeliveries: Array<string> | undefined,
+    unidentifiedDeliveries: Array<string> | undefined
-    discoveredIdentifierPairs:
-      | Array<{
-          uuid: string | null;
-          e164: string | null;
-        }>
-      | undefined
   ): Promise<void> {
-    (discoveredIdentifierPairs || []).forEach(item => {
-      const { uuid, e164 } = item;
-      window.ConversationController.ensureContactIds({
-        uuid,
-        e164,
-        highTrust: true,
-      });
-    });
-
     await Promise.all(
       (failoverIdentifiers || []).map(async identifier => {
         const conversation = window.ConversationController.get(identifier);
@@ -4246,15 +4267,37 @@ window.Whisper.ConversationCollection = window.Backbone.Collection.extend({
     this._byGroupId = Object.create(null);
   },
 
-  add(...models: Array<WhatIsThis>) {
+  add(data: WhatIsThis | Array<WhatIsThis>) {
-    const result = window.Backbone.Collection.prototype.add.apply(
+    let hydratedData;
-      this,
+
-      models as WhatIsThis
+    // First, we need to ensure that the data we're working with is Conversation models
+    if (Array.isArray(data)) {
+      hydratedData = [];
+      for (let i = 0, max = data.length; i < max; i += 1) {
+        const item = data[i];
+
+        // We create a new model if it's not already a model
+        if (!item.get) {
+          hydratedData.push(new Whisper.Conversation(item));
+        } else {
+          hydratedData.push(item);
+        }
+      }
+    } else if (!data.get) {
+      hydratedData = new Whisper.Conversation(data);
+    } else {
+      hydratedData = data;
+    }
+
+    // Next, we update our lookups first to prevent infinite loops on the 'add' event
+    this.generateLookups(
+      Array.isArray(hydratedData) ? hydratedData : [hydratedData]
     );
 
-    this.generateLookups(Array.isArray(result) ? result.slice(0) : [result]);
+    // Lastly, we fire off the add events related to this change
+    window.Backbone.Collection.prototype.add.call(this, hydratedData);
 
-    return result;
+    return hydratedData;
   },
 
   /**

ts/state/ducks/conversations.ts

@@ -54,6 +54,7 @@ export type ConversationType = {
   lastMessage?: {
     status: LastMessageStatus;
     text: string;
+    deletedForEveryone?: boolean;
   };
   phoneNumber?: string;
   membersCount?: number;
@@ -76,6 +77,7 @@ export type ConversationType = {
   draftText?: string | null;
   draftPreview?: string;
 
+  sharedGroupNames?: Array<string>;
   groupVersion?: 1 | 2;
   isMissingMandatoryProfileSharing?: boolean;
   messageRequestsEnabled?: boolean;

ts/textsecure/OutgoingMessage.ts

@@ -53,11 +53,6 @@ export default class OutgoingMessage {
 
   unidentifiedDeliveries: Array<unknown>;
 
-  discoveredIdentifierPairs: Array<{
-    e164: string;
-    uuid: string;
-  }>;
-
   sendMetadata?: SendMetadataType;
 
   senderCertificate?: ArrayBuffer;
@@ -93,7 +88,6 @@ export default class OutgoingMessage {
     this.successfulIdentifiers = [];
     this.failoverIdentifiers = [];
     this.unidentifiedDeliveries = [];
-    this.discoveredIdentifierPairs = [];
 
     const { sendMetadata, senderCertificate, online } = options;
     this.sendMetadata = sendMetadata;
@@ -109,7 +103,6 @@ export default class OutgoingMessage {
         failoverIdentifiers: this.failoverIdentifiers,
         errors: this.errors,
         unidentifiedDeliveries: this.unidentifiedDeliveries,
-        discoveredIdentifierPairs: this.discoveredIdentifierPairs,
       });
     }
   }
@@ -621,9 +614,10 @@ export default class OutgoingMessage {
             ]);
             const uuid = lookup[identifier];
             if (uuid) {
-              this.discoveredIdentifierPairs.push({
+              window.ConversationController.ensureContactIds({
                 uuid,
                 e164: identifier,
+                highTrust: true,
               });
               identifier = uuid;
             } else {

ts/textsecure/SendMessage.ts

@@ -78,10 +78,6 @@ export type CallbackResultType = {
   errors?: Array<CustomError>;
   unidentifiedDeliveries?: Array<any>;
   dataMessage?: ArrayBuffer;
-  discoveredIdentifierPairs: Array<{
-    e164: string;
-    uuid: string | null;
-  }>;
 };
 
 type PreviewType = {
@@ -1383,7 +1379,6 @@ export default class MessageSender {
     if (identifiers.length === 0) {
       return Promise.resolve({
         dataMessage: proto.toArrayBuffer(),
-        discoveredIdentifierPairs: [],
         errors: [],
         failoverIdentifiers: [],
         successfulIdentifiers: [],
@@ -1657,7 +1652,6 @@ export default class MessageSender {
         errors: [],
         unidentifiedDeliveries: [],
         dataMessage: await this.getMessageProtoObj(attrs),
-        discoveredIdentifierPairs: [],
       });
     }
 
@@ -1731,7 +1725,6 @@ export default class MessageSender {
         errors: [],
         unidentifiedDeliveries: [],
         dataMessage: await this.getMessageProtoObj(attrs),
-        discoveredIdentifierPairs: [],
       });
     }