From 825617006677dc2db0a1e2b1cb7d273bd5c6ac92 Mon Sep 17 00:00:00 2001 From: Evan Hahn <69474926+EvanHahn-Signal@users.noreply.github.com> Date: Thu, 4 Nov 2021 16:11:47 -0500 Subject: [PATCH] Fix `ConversationController` load race condition --- ts/ConversationController.ts | 179 ++++++++---------- ts/background.ts | 3 - ts/jobs/normalMessageSendJobQueue.ts | 2 +- ts/jobs/reactionJobQueue.ts | 2 +- ts/state/ducks/app.ts | 2 +- ts/test-electron/models/conversations_test.ts | 2 +- .../textsecure/KeyChangeListener_test.ts | 1 - ts/util/lint/exceptions.json | 42 ++++ 8 files changed, 127 insertions(+), 106 deletions(-) diff --git a/ts/ConversationController.ts b/ts/ConversationController.ts index 8d85538e3958..7db94a23e5df 100644 --- a/ts/ConversationController.ts +++ b/ts/ConversationController.ts @@ -114,19 +114,11 @@ export function start(): void { } export class ConversationController { - private _initialFetchComplete: boolean | undefined; + private _initialFetchComplete = false; - private _initialPromise: Promise = Promise.resolve(); + private _initialPromise: undefined | Promise; - private _conversations: ConversationModelCollectionType; - - constructor(conversations?: ConversationModelCollectionType) { - if (!conversations) { - throw new Error('ConversationController: need conversation collection!'); - } - - this._conversations = conversations; - } + constructor(private _conversations: ConversationModelCollectionType) {} get(id?: string | null): ConversationModel | undefined { if (!this._initialFetchComplete) { @@ -255,7 +247,7 @@ export class ConversationController { type: ConversationAttributesTypeType, additionalInitialProps = {} ): Promise { - await this._initialPromise; + await this.load(); const conversation = this.getOrCreate(id, type, additionalInitialProps); if (conversation) { @@ -756,109 +748,100 @@ export class ConversationController { ); } - async loadPromise(): Promise { - return this._initialPromise; - } - reset(): void { - this._initialPromise = Promise.resolve(); + delete this._initialPromise; this._initialFetchComplete = false; this._conversations.reset([]); } - isFetchComplete(): boolean | undefined { - return this._initialFetchComplete; + load(): Promise { + this._initialPromise ||= this.doLoad(); + return this._initialPromise; } - async load(): Promise { + private async doLoad(): Promise { log.info('ConversationController: starting initial fetch'); if (this._conversations.length) { throw new Error('ConversationController: Already loaded!'); } - const load = async () => { - try { - const collection = await getAllConversations({ - ConversationCollection: window.Whisper.ConversationCollection, - }); + try { + const collection = await getAllConversations({ + ConversationCollection: window.Whisper.ConversationCollection, + }); - // Get rid of temporary conversations - const temporaryConversations = collection.filter(conversation => - Boolean(conversation.get('isTemporary')) + // Get rid of temporary conversations + const temporaryConversations = collection.filter(conversation => + Boolean(conversation.get('isTemporary')) + ); + + if (temporaryConversations.length) { + log.warn( + `ConversationController: Removing ${temporaryConversations.length} temporary conversations` ); - - if (temporaryConversations.length) { - log.warn( - `ConversationController: Removing ${temporaryConversations.length} temporary conversations` - ); - } - const queue = new PQueue({ concurrency: 3, timeout: 1000 * 60 * 2 }); - queue.addAll( - temporaryConversations.map(item => async () => { - await removeConversation(item.id, { - Conversation: window.Whisper.Conversation, - }); - }) - ); - await queue.onIdle(); - - // Hydrate the final set of conversations - this._conversations.add( - collection.filter(conversation => !conversation.get('isTemporary')) - ); - - this._initialFetchComplete = true; - - await Promise.all( - this._conversations.map(async conversation => { - try { - // Hydrate contactCollection, now that initial fetch is complete - conversation.fetchContacts(); - - const isChanged = maybeDeriveGroupV2Id(conversation); - if (isChanged) { - updateConversation(conversation.attributes); - } - - // In case a too-large draft was saved to the database - const draft = conversation.get('draft'); - if (draft && draft.length > MAX_MESSAGE_BODY_LENGTH) { - conversation.set({ - draft: draft.slice(0, MAX_MESSAGE_BODY_LENGTH), - }); - updateConversation(conversation.attributes); - } - - // Clean up the conversations that have UUID as their e164. - const e164 = conversation.get('e164'); - const uuid = conversation.get('uuid'); - if (isValidUuid(e164) && uuid) { - conversation.set({ e164: undefined }); - updateConversation(conversation.attributes); - - log.info(`Cleaning up conversation(${uuid}) with invalid e164`); - } - } catch (error) { - log.error( - 'ConversationController.load/map: Failed to prepare a conversation', - error && error.stack ? error.stack : error - ); - } - }) - ); - log.info('ConversationController: done with initial fetch'); - } catch (error) { - log.error( - 'ConversationController: initial fetch failed', - error && error.stack ? error.stack : error - ); - throw error; } - }; + const queue = new PQueue({ concurrency: 3, timeout: 1000 * 60 * 2 }); + queue.addAll( + temporaryConversations.map(item => async () => { + await removeConversation(item.id, { + Conversation: window.Whisper.Conversation, + }); + }) + ); + await queue.onIdle(); - this._initialPromise = load(); + // Hydrate the final set of conversations + this._conversations.add( + collection.filter(conversation => !conversation.get('isTemporary')) + ); - return this._initialPromise; + this._initialFetchComplete = true; + + await Promise.all( + this._conversations.map(async conversation => { + try { + // Hydrate contactCollection, now that initial fetch is complete + conversation.fetchContacts(); + + const isChanged = maybeDeriveGroupV2Id(conversation); + if (isChanged) { + updateConversation(conversation.attributes); + } + + // In case a too-large draft was saved to the database + const draft = conversation.get('draft'); + if (draft && draft.length > MAX_MESSAGE_BODY_LENGTH) { + conversation.set({ + draft: draft.slice(0, MAX_MESSAGE_BODY_LENGTH), + }); + updateConversation(conversation.attributes); + } + + // Clean up the conversations that have UUID as their e164. + const e164 = conversation.get('e164'); + const uuid = conversation.get('uuid'); + if (isValidUuid(e164) && uuid) { + conversation.set({ e164: undefined }); + updateConversation(conversation.attributes); + + log.info(`Cleaning up conversation(${uuid}) with invalid e164`); + } + } catch (error) { + log.error( + 'ConversationController.load/map: Failed to prepare a conversation', + error && error.stack ? error.stack : error + ); + } + }) + ); + log.info('ConversationController: done with initial fetch'); + } catch (error) { + log.error( + 'ConversationController: initial fetch failed', + error && error.stack ? error.stack : error + ); + throw error; + } } } diff --git a/ts/background.ts b/ts/background.ts index 8cd3ed57f271..11d2f7b5b149 100644 --- a/ts/background.ts +++ b/ts/background.ts @@ -812,9 +812,6 @@ export async function startApp(): Promise { } }); - // We start this up before window.ConversationController.load() to - // ensure that our feature flags are represented in the cached props - // we generate on load of each convo. window.Signal.RemoteConfig.initRemoteConfig(server); let retryReceiptLifespan: number | undefined; diff --git a/ts/jobs/normalMessageSendJobQueue.ts b/ts/jobs/normalMessageSendJobQueue.ts index c72b89bb592a..be4549d67159 100644 --- a/ts/jobs/normalMessageSendJobQueue.ts +++ b/ts/jobs/normalMessageSendJobQueue.ts @@ -91,7 +91,7 @@ export class NormalMessageSendJobQueue extends JobQueue { timeRemaining, }); - await window.ConversationController.loadPromise(); + await window.ConversationController.load(); const ourConversationId = window.ConversationController.getOurConversationIdOrThrow(); diff --git a/ts/state/ducks/app.ts b/ts/state/ducks/app.ts index 20ea0349692d..83828f2c3ef6 100644 --- a/ts/state/ducks/app.ts +++ b/ts/state/ducks/app.ts @@ -70,7 +70,7 @@ function openInbox(): ThunkAction< return async dispatch => { log.info('open inbox'); - await window.ConversationController.loadPromise(); + await window.ConversationController.load(); dispatch({ type: OPEN_INBOX, diff --git a/ts/test-electron/models/conversations_test.ts b/ts/test-electron/models/conversations_test.ts index 05d3ee956b33..58b9f5d6e06a 100644 --- a/ts/test-electron/models/conversations_test.ts +++ b/ts/test-electron/models/conversations_test.ts @@ -43,7 +43,7 @@ describe('Conversations', () => { deviceName: 'my device', password: 'password', }); - await window.ConversationController.loadPromise(); + await window.ConversationController.load(); await window.Signal.Data.saveConversation(conversation.attributes); diff --git a/ts/test-electron/textsecure/KeyChangeListener_test.ts b/ts/test-electron/textsecure/KeyChangeListener_test.ts index 638e14e52058..f043f79018e7 100644 --- a/ts/test-electron/textsecure/KeyChangeListener_test.ts +++ b/ts/test-electron/textsecure/KeyChangeListener_test.ts @@ -55,7 +55,6 @@ describe('KeyChangeListener', () => { beforeEach(async () => { window.ConversationController.reset(); await window.ConversationController.load(); - await window.ConversationController.loadPromise(); convo = window.ConversationController.dangerouslyCreateAndAdd({ id: uuidWithKeyChange, diff --git a/ts/util/lint/exceptions.json b/ts/util/lint/exceptions.json index e60c43356ec8..de398124300c 100644 --- a/ts/util/lint/exceptions.json +++ b/ts/util/lint/exceptions.json @@ -13072,6 +13072,34 @@ "reasonCategory": "usageTrusted", "updated": "2021-10-22T00:52:39.251Z" }, + { + "rule": "jQuery-load(", + "path": "ts/jobs/normalMessageSendJobQueue.js", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, + { + "rule": "jQuery-load(", + "path": "ts/jobs/normalMessageSendJobQueue.ts", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, + { + "rule": "jQuery-load(", + "path": "ts/jobs/reactionJobQueue.js", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, + { + "rule": "jQuery-load(", + "path": "ts/jobs/reactionJobQueue.ts", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, { "rule": "jQuery-append(", "path": "ts/logging/debuglogs.js", @@ -13215,6 +13243,20 @@ "updated": "2021-01-21T23:06:13.270Z", "reasonDetail": "Doesn't manipulate the DOM." }, + { + "rule": "jQuery-load(", + "path": "ts/state/ducks/app.js", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, + { + "rule": "jQuery-load(", + "path": "ts/state/ducks/app.ts", + "line": " await window.ConversationController.load();", + "reasonCategory": "falseMatch", + "updated": "2021-11-04T16:14:03.477Z" + }, { "rule": "jQuery-load(", "path": "ts/types/Stickers.js",