From 14591358f19209ff3d7178d3a1de5b9284a70583 Mon Sep 17 00:00:00 2001 From: Scott Nonnenberg Date: Mon, 11 Jul 2022 17:32:26 -0700 Subject: [PATCH] Simplify expireTimer change handling, queue for contact sync --- ts/background.ts | 114 +++++++++++++++++-------------- ts/models/messages.ts | 75 ++++++++++---------- ts/textsecure/MessageReceiver.ts | 3 +- 3 files changed, 98 insertions(+), 94 deletions(-) diff --git a/ts/background.ts b/ts/background.ts index 7e33266e4..ce781611a 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -317,7 +317,7 @@ export async function startApp(): Promise { ); messageReceiver.addEventListener( 'contact', - queuedEventListener(onContactReceived) + queuedEventListener(onContactReceived, false) ); messageReceiver.addEventListener( 'contactSync', @@ -2718,7 +2718,9 @@ export async function startApp(): Promise { window.Whisper.events.trigger('contactSync:complete'); } - async function onContactReceived(ev: ContactEvent) { + // Note: Like the handling for incoming/outgoing messages, this method is synchronous, + // deferring its async logic to the function passed to conversation.queueJob(). + function onContactReceived(ev: ContactEvent) { const details = ev.contactDetails; const c = new window.Whisper.Conversation({ @@ -2735,60 +2737,66 @@ export async function startApp(): Promise { return; } - try { - const detailsId = window.ConversationController.ensureContactIds({ - e164: details.number, - uuid: details.uuid, - highTrust: true, - reason: 'onContactReceived', - }); - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const conversation = window.ConversationController.get(detailsId)!; + const detailsId = window.ConversationController.ensureContactIds({ + e164: details.number, + uuid: details.uuid, + highTrust: true, + reason: 'onContactReceived', + }); + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + const conversation = window.ConversationController.get(detailsId)!; - conversation.set({ - name: details.name, - inbox_position: details.inboxPosition, - }); - - // Update the conversation avatar only if new avatar exists and hash differs - const { avatar } = details; - if (avatar && avatar.data) { - const newAttributes = await Conversation.maybeUpdateAvatar( - conversation.attributes, - avatar.data, - { - writeNewAttachmentData, - deleteAttachmentData, - doesAttachmentExist, - } - ); - conversation.set(newAttributes); - } else { - const { attributes } = conversation; - if (attributes.avatar && attributes.avatar.path) { - await deleteAttachmentData(attributes.avatar.path); - } - conversation.set({ avatar: null }); - } - - window.Signal.Data.updateConversation(conversation.attributes); - - // expireTimer isn't stored in Storage Service so we have to rely on the - // contact sync. - const { expireTimer } = details; - const isValidExpireTimer = typeof expireTimer === 'number'; - if (isValidExpireTimer) { - await conversation.updateExpirationTimer(expireTimer, { - source: window.ConversationController.getOurConversationId(), - receivedAt: ev.receivedAtCounter, - fromSync: true, - isInitialSync, - reason: 'contact sync', + // It's important to use queueJob here because we might update the expiration timer + // and we don't want conflicts with incoming message processing happening on the + // conversation queue. + conversation.queueJob('onContactReceived', async () => { + try { + conversation.set({ + name: details.name, + inbox_position: details.inboxPosition, }); + + // Update the conversation avatar only if new avatar exists and hash differs + const { avatar } = details; + if (avatar && avatar.data) { + const newAttributes = await Conversation.maybeUpdateAvatar( + conversation.attributes, + avatar.data, + { + writeNewAttachmentData, + deleteAttachmentData, + doesAttachmentExist, + } + ); + conversation.set(newAttributes); + } else { + const { attributes } = conversation; + if (attributes.avatar && attributes.avatar.path) { + await deleteAttachmentData(attributes.avatar.path); + } + conversation.set({ avatar: null }); + } + + window.Signal.Data.updateConversation(conversation.attributes); + + // expireTimer isn't in Storage Service so we have to rely on contact sync. + const { expireTimer } = details; + const isValidExpireTimer = typeof expireTimer === 'number'; + if (isValidExpireTimer) { + await conversation.updateExpirationTimer(expireTimer, { + source: window.ConversationController.getOurConversationId(), + receivedAt: ev.receivedAtCounter, + fromSync: true, + isInitialSync, + reason: 'contact sync', + }); + } + + window.Whisper.events.trigger('incrementProgress'); + } catch (error) { + log.error('onContactReceived error:', Errors.toLogFormat(error)); } - } catch (error) { - log.error('onContactReceived error:', Errors.toLogFormat(error)); - } + }); } async function onGroupSyncComplete() { diff --git a/ts/models/messages.ts b/ts/models/messages.ts index 6939b9b94..d4e570223 100644 --- a/ts/models/messages.ts +++ b/ts/models/messages.ts @@ -2045,7 +2045,7 @@ export class MessageModel extends window.Backbone.Model { const idLog = conversation.idForLogging(); await conversation.queueJob('handleDataMessage', async () => { log.info( - `handleDataMessage/${idLog}: processsing message ${message.idForLogging()}` + `handleDataMessage/${idLog}: processing message ${message.idForLogging()}` ); if ( @@ -2603,49 +2603,44 @@ export class MessageModel extends window.Backbone.Model { expireTimer: initialMessage.expireTimer, }, }); - conversation.set({ expireTimer: dataMessage.expireTimer }); - } - // NOTE: Remove once the above calls this.model.updateExpirationTimer() - const { expireTimer } = dataMessage; - const shouldLogExpireTimerChange = - isExpirationTimerUpdate(message.attributes) || expireTimer; - if (shouldLogExpireTimerChange) { - log.info("Update conversation 'expireTimer'", { - id: conversation.idForLogging(), - expireTimer, - source: 'handleDataMessage', - }); - } - - if (!isEndSession(message.attributes)) { - if (dataMessage.expireTimer) { - if ( - dataMessage.expireTimer !== conversation.get('expireTimer') - ) { - conversation.updateExpirationTimer(dataMessage.expireTimer, { - source, - receivedAt: message.get('received_at'), - receivedAtMS: message.get('received_at_ms'), - sentAt: message.get('sent_at'), - fromGroupUpdate: isGroupUpdate(message.attributes), - reason: `handleDataMessage(${this.idForLogging()})`, - }); - } - } else if ( - conversation.get('expireTimer') && - // We only turn off timers if it's not a group update - !isGroupUpdate(message.attributes) - ) { - conversation.updateExpirationTimer(undefined, { - source, - receivedAt: message.get('received_at'), - receivedAtMS: message.get('received_at_ms'), - sentAt: message.get('sent_at'), - reason: `handleDataMessage(${this.idForLogging()})`, + if (conversation.get('expireTimer') !== dataMessage.expireTimer) { + log.info('Incoming expirationTimerUpdate changed timer', { + id: conversation.idForLogging(), + expireTimer: dataMessage.expireTimer || 'disabled', + source: 'handleDataMessage/expirationTimerUpdate', + }); + conversation.set({ + expireTimer: dataMessage.expireTimer, }); } } + + // Note: For incoming expire timer updates (not normal messages that come + // along with an expireTimer), the conversation will be updated by this + // point and these calls will return early. + if (dataMessage.expireTimer) { + conversation.updateExpirationTimer(dataMessage.expireTimer, { + source, + receivedAt: message.get('received_at'), + receivedAtMS: message.get('received_at_ms'), + sentAt: message.get('sent_at'), + fromGroupUpdate: isGroupUpdate(message.attributes), + reason: `handleDataMessage(${this.idForLogging()})`, + }); + } else if ( + // We won't turn off timers for these kinds of messages: + !isGroupUpdate(message.attributes) && + !isEndSession(message.attributes) + ) { + conversation.updateExpirationTimer(undefined, { + source, + receivedAt: message.get('received_at'), + receivedAtMS: message.get('received_at_ms'), + sentAt: message.get('sent_at'), + reason: `handleDataMessage(${this.idForLogging()})`, + }); + } } if (initialMessage.profileKey) { diff --git a/ts/textsecure/MessageReceiver.ts b/ts/textsecure/MessageReceiver.ts index dfc1654d8..d54cb42b6 100644 --- a/ts/textsecure/MessageReceiver.ts +++ b/ts/textsecure/MessageReceiver.ts @@ -2885,7 +2885,7 @@ export default class MessageReceiver envelope: ProcessedEnvelope, contacts: Proto.SyncMessage.IContacts ): Promise { - log.info('MessageReceiver: handleContacts'); + log.info(`MessageReceiver: handleContacts ${getEnvelopeId(envelope)}`); const { blob } = contacts; if (!blob) { throw new Error('MessageReceiver.handleContacts: blob field was missing'); @@ -2924,6 +2924,7 @@ export default class MessageReceiver groups: Proto.SyncMessage.IGroups ): Promise { log.info('group sync'); + log.info(`MessageReceiver: handleGroups ${getEnvelopeId(envelope)}`); const { blob } = groups; this.removeFromCache(envelope);