Retain protections on gv1 records that match gv2 ids

ts/models/conversations.ts

@@ -3725,7 +3725,9 @@ export class ConversationModel extends window.Backbone.Model<
 
     if (Boolean(before) !== Boolean(after)) {
       if (after) {
-        this.unpin();
+        // we're capturing a storage sync below so
+        // we don't need to capture it twice
+        this.unpin({ stopStorageSync: true });
       }
       this.captureChange('isArchived');
     }
@@ -5105,6 +5107,10 @@ export class ConversationModel extends window.Backbone.Model<
   }
 
   pin(): void {
+    if (this.get('isPinned')) {
+      return;
+    }
+
     window.log.info('pinning', this.idForLogging());
     const pinnedConversationIds = new Set(
       window.storage.get<Array<string>>('pinnedConversationIds', [])
@@ -5115,14 +5121,18 @@ export class ConversationModel extends window.Backbone.Model<
     this.writePinnedConversations([...pinnedConversationIds]);
 
     this.set('isPinned', true);
-    window.Signal.Data.updateConversation(this.attributes);
 
     if (this.get('isArchived')) {
-      this.setArchived(false);
+      this.set({ isArchived: false });
     }
+    window.Signal.Data.updateConversation(this.attributes);
   }
 
-  unpin(): void {
+  unpin({ stopStorageSync = false } = {}): void {
+    if (!this.get('isPinned')) {
+      return;
+    }
+
     window.log.info('un-pinning', this.idForLogging());
 
     const pinnedConversationIds = new Set(
@@ -5131,7 +5141,9 @@ export class ConversationModel extends window.Backbone.Model<
 
     pinnedConversationIds.delete(this.id);
 
-    this.writePinnedConversations([...pinnedConversationIds]);
+    if (!stopStorageSync) {
+      this.writePinnedConversations([...pinnedConversationIds]);
+    }
 
     this.set('isPinned', false);
     window.Signal.Data.updateConversation(this.attributes);

ts/services/storage.ts

@@ -131,8 +131,6 @@ async function generateManifest(
 
   await window.ConversationController.checkForConflicts();
 
-  await repairUnknownAndErroredRecords();
-
   const ITEM_TYPE = window.textsecure.protobuf.ManifestRecord.Identifier.Type;
 
   const conversationsToUpdate = [];
@@ -351,54 +349,6 @@ async function generateManifest(
   };
 }
 
-async function repairUnknownAndErroredRecords() {
-  const unknownRecordsArray: ReadonlyArray<UnknownRecord> =
-    window.storage.get('storage-service-unknown-records') || [];
-
-  const recordsWithErrors: ReadonlyArray<UnknownRecord> =
-    window.storage.get('storage-service-error-records') || [];
-
-  const remoteRecords = unknownRecordsArray.concat(recordsWithErrors);
-
-  // No repair necessary
-  if (remoteRecords.length === 0) {
-    return;
-  }
-
-  // Process unknown and records with records from the past sync to see
-  // if they can be merged
-  const remoteRecordsMap: Map<string, RemoteRecord> = new Map();
-  remoteRecords.forEach(record => {
-    remoteRecordsMap.set(record.storageID, record);
-  });
-
-  window.log.info(
-    'storageService.repairUnknownAndErroredRecords: found ' +
-      `${unknownRecordsArray.length} unknown records and ` +
-      `${recordsWithErrors.length} errored records, attempting repair`
-  );
-  const conflictCount = await processRemoteRecords(remoteRecordsMap);
-  if (conflictCount !== 0) {
-    window.log.info(
-      'storageService.repairUnknownAndErroredRecords: fixed ' +
-        `${conflictCount} conflicts`
-    );
-  }
-
-  const newUnknownCount = (
-    window.storage.get('storage-service-unknown-records') || []
-  ).length;
-  const newErroredCount = (
-    window.storage.get('storage-service-error-records') || []
-  ).length;
-
-  window.log.info(
-    'storageService.repairUnknownAndErroredRecords: ' +
-      `${newUnknownCount} unknown records and ` +
-      `${newErroredCount} errored records after repair`
-  );
-}
-
 async function uploadManifest(
   version: number,
   {
@@ -647,8 +597,9 @@ async function mergeRecord(
   } catch (err) {
     hasError = true;
     window.log.error(
-      'storageService.mergeRecord: merging record failed',
-      err && err.stack ? err.stack : String(err)
+      'storageService.mergeRecord: Error with',
+      storageID,
+      itemType
     );
   }
 
@@ -691,14 +642,6 @@ async function processManifest(
     }
   });
 
-  const recordsWithErrors: ReadonlyArray<UnknownRecord> =
-    window.storage.get('storage-service-error-records') || [];
-
-  // Do not fetch any records that we failed to merge in the previous fetch
-  recordsWithErrors.forEach((record: UnknownRecord) => {
-    localKeys.push(record.storageID);
-  });
-
   window.log.info(
     'storageService.processManifest: local keys:',
     localKeys.length
@@ -708,10 +651,6 @@ async function processManifest(
     'storageService.processManifest: incl. unknown records:',
     unknownRecordsArray.length
   );
-  window.log.info(
-    'storageService.processManifest: incl. records with errors:',
-    recordsWithErrors.length
-  );
 
   const remoteKeys = Array.from(remoteKeysTypeMap.keys());
 
@@ -1062,6 +1001,9 @@ async function upload(fromSync = false): Promise<void> {
   }
 
   if (!fromSync) {
+    // Syncing before we upload so that we repair any unknown records and
+    // records with errors as well as ensure that we have the latest up to date
+    // manifest.
     await sync();
   }
 

ts/services/storageRecordOps.ts

@@ -388,7 +388,20 @@ export async function mergeGroupV1Record(
   // Attempt to fetch an existing group pertaining to the `groupId` or create
   // a new group and populate it with the attributes from the record.
   let conversation = window.ConversationController.get(groupId);
+
+  // Because ConversationController.get retrieves all types of records we
+  // may sometimes have a situation where we get a record of groupv1 type
+  // where the binary representation of its ID matches a v2 record in memory.
+  // Here we ensure that the record we're about to process is GV1 otherwise
+  // we drop the update.
+  if (conversation && !conversation.isGroupV1()) {
+    throw new Error(`Record has group type mismatch ${conversation.debugID()}`);
+  }
+
   if (!conversation) {
+    // It's possible this group was migrated to a GV2 if so we attempt to
+    // retrieve the master key and find the conversation locally. If we
+    // are successful then we continue setting and applying state.
     const masterKeyBuffer = await deriveMasterKeyFromGroupV1(groupId);
     const fields = deriveGroupFields(masterKeyBuffer);
     const derivedGroupV2Id = arrayBufferToBase64(fields.id);
@@ -415,8 +428,6 @@ export async function mergeGroupV1Record(
     );
   }
 
-  // If we receive a group V1 record, remote data should take precendence
-  // even if the group is actually V2 on our end.
   conversation.set({
     isArchived: Boolean(groupV1Record.archived),
     markedUnread: Boolean(groupV1Record.markedUnread),