Fix ConversationController load race condition

ts/ConversationController.ts

@@ -114,19 +114,11 @@ export function start(): void {
 }
 
 export class ConversationController {
-  private _initialFetchComplete: boolean | undefined;
+  private _initialFetchComplete = false;
 
-  private _initialPromise: Promise<void> = Promise.resolve();
+  private _initialPromise: undefined | Promise<void>;
 
-  private _conversations: ConversationModelCollectionType;
+  constructor(private _conversations: ConversationModelCollectionType) {}
-
-  constructor(conversations?: ConversationModelCollectionType) {
-    if (!conversations) {
-      throw new Error('ConversationController: need conversation collection!');
-    }
-
-    this._conversations = conversations;
-  }
 
   get(id?: string | null): ConversationModel | undefined {
     if (!this._initialFetchComplete) {
@@ -255,7 +247,7 @@ export class ConversationController {
     type: ConversationAttributesTypeType,
     additionalInitialProps = {}
   ): Promise<ConversationModel> {
-    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<void> {
-    return this._initialPromise;
-  }
-
   reset(): void {
-    this._initialPromise = Promise.resolve();
+    delete this._initialPromise;
     this._initialFetchComplete = false;
     this._conversations.reset([]);
   }
 
-  isFetchComplete(): boolean | undefined {
+  load(): Promise<void> {
-    return this._initialFetchComplete;
+    this._initialPromise ||= this.doLoad();
+    return this._initialPromise;
   }
 
-  async load(): Promise<void> {
+  private async doLoad(): Promise<void> {
     log.info('ConversationController: starting initial fetch');
 
     if (this._conversations.length) {
       throw new Error('ConversationController: Already loaded!');
     }
 
-    const load = async () => {
+    try {
-      try {
+      const collection = await getAllConversations({
-        const collection = await getAllConversations({
+        ConversationCollection: window.Whisper.ConversationCollection,
-          ConversationCollection: window.Whisper.ConversationCollection,
+      });
-        });
 
-        // Get rid of temporary conversations
+      // Get rid of temporary conversations
-        const temporaryConversations = collection.filter(conversation =>
+      const temporaryConversations = collection.filter(conversation =>
-          Boolean(conversation.get('isTemporary'))
+        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;
+    }
   }
 }

ts/background.ts

@@ -812,9 +812,6 @@ export async function startApp(): Promise<void> {
       }
     });
 
-    // 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;

ts/jobs/normalMessageSendJobQueue.ts

@@ -91,7 +91,7 @@ export class NormalMessageSendJobQueue extends JobQueue<NormalMessageSendJobData
       timeRemaining,
     });
 
-    await window.ConversationController.loadPromise();
+    await window.ConversationController.load();
 
     const message = await getMessageById(messageId);
     if (!message) {

ts/jobs/reactionJobQueue.ts

@@ -70,7 +70,7 @@ export class ReactionJobQueue extends JobQueue<ReactionJobData> {
       timeRemaining,
     });
 
-    await window.ConversationController.loadPromise();
+    await window.ConversationController.load();
 
     const ourConversationId = window.ConversationController.getOurConversationIdOrThrow();
 

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,

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

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,

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",