Fix passwords not being shown on settings due to deduplication failure

Assertions made by MultiStorePasswordUiEntry/MultiStoreExceptionEntry
when merging two entries [1] are failing in some cases. This causes an
exception to be thrown when the list of saved passwords is being updated,
which in turn results in some passwords not being shown.

In this CL, we treat merge failures more gracefully, namely:
- Instead of assertions, we now have merge() return whether it
succeeded.
- The constructor of the multi-store entries can now only have 1 argument
since we can't "return a failure" when calling the constructor.
- When a merge fails, we just create separate items in the UI for the
entries we were trying to merge.
- We rename merge() to mergeInPlace() to make its side-effect clearer.

[1] https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/resources/settings/autofill_page/multi_store_password_ui_entry.js;l=51

Bug: 1114697
Change-Id: I588af43cba874510b4d685a42434aa988c90b189
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2351911Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/master@{#797704}

chrome/browser/resources/settings/autofill_page/merge_exceptions_store_copies_behavior.js

@@ -67,7 +67,16 @@ export const MergeExceptionsStoreCopiesBehavior = {
     const frontendIdToMergedEntry = new Map();
     for (const entry of exceptionList) {
       if (frontendIdToMergedEntry.has(entry.frontendId)) {
-        frontendIdToMergedEntry.get(entry.frontendId).merge(entry);
+        const mergeSucceded =
+            frontendIdToMergedEntry.get(entry.frontendId).mergeInPlace(entry);
+        if (mergeSucceded) {
+          // The merge is in-place, so nothing to be done.
+        } else {
+          // The merge can fail in weird cases despite |frontendId| matching.
+          // If so, just create another entry in the UI for |entry|. See also
+          // crbug.com/1114697.
+          multiStoreEntries.push(new MultiStoreExceptionEntry(entry));
+        }
       } else {
         const multiStoreEntry = new MultiStoreExceptionEntry(entry);
         frontendIdToMergedEntry.set(entry.frontendId, multiStoreEntry);

chrome/browser/resources/settings/autofill_page/merge_passwords_store_copies_behavior.js

@@ -77,7 +77,16 @@ const MergePasswordsStoreCopiesBehaviorImpl = {
     const frontendIdToMergedEntry = new Map();
     for (const entry of passwordList) {
       if (frontendIdToMergedEntry.has(entry.frontendId)) {
-        frontendIdToMergedEntry.get(entry.frontendId).merge(entry);
+        const mergeSucceded =
+            frontendIdToMergedEntry.get(entry.frontendId).mergeInPlace(entry);
+        if (mergeSucceded) {
+          // The merge is in-place, so nothing to be done.
+        } else {
+          // The merge can fail in weird cases despite |frontendId| matching.
+          // If so, just create another entry in the UI for |entry|. See also
+          // crbug.com/1114697.
+          multiStoreEntries.push(new MultiStorePasswordUiEntry(entry));
+        }
       } else {
         const multiStoreEntry = new MultiStorePasswordUiEntry(entry);
         frontendIdToMergedEntry.set(entry.frontendId, multiStoreEntry);

chrome/browser/resources/settings/autofill_page/multi_store_exception_entry.js

@@ -18,37 +18,37 @@ import {PasswordManagerProxy} from './password_manager_proxy.js';
  */
 export class MultiStoreExceptionEntry extends MultiStoreIdHandler {
   /**
-   * Creates a multi-store entry from duplicates |entry1| and (optional)
-   * |entry2|. If both arguments are passed, they should have the same contents
-   * but should be from different stores.
-   * @param {!PasswordManagerProxy.ExceptionEntry} entry1
-   * @param {PasswordManagerProxy.ExceptionEntry=} entry2
+   * @param {!PasswordManagerProxy.ExceptionEntry} entry
    */
-  constructor(entry1, entry2) {
+  constructor(entry) {
     super();
 
     /** @type {!PasswordManagerProxy.UrlCollection} */
-    this.urls_ = entry1.urls;
+    this.urls_ = entry.urls;
 
-    this.setId(entry1.id, entry1.fromAccountStore);
-
-    if (entry2) {
-      this.merge(entry2);
-    }
+    this.setId(entry.id, entry.fromAccountStore);
   }
 
   /**
    * Incorporates the id of |otherEntry|, as long as |otherEntry| matches
-   * |contents_| and the id corresponding to its store is not set.
+   * |contents_| and the id corresponding to its store is not set. If these
+   * preconditions are not satisfied, results in a no-op.
    * @param {!PasswordManagerProxy.ExceptionEntry} otherEntry
+   * @return {boolean} Returns whether the merge succeeded.
    */
   // TODO(crbug.com/1102294) Consider asserting frontendId as well.
-  merge(otherEntry) {
-    assert(
-        (this.isPresentInAccount() && !otherEntry.fromAccountStore) ||
-        (this.isPresentOnDevice() && otherEntry.fromAccountStore));
-    assert(JSON.stringify(this.urls_) === JSON.stringify(otherEntry.urls));
+  mergeInPlace(otherEntry) {
+    const alreadyHasCopyFromStore =
+        (this.isPresentInAccount() && otherEntry.fromAccountStore) ||
+        (this.isPresentOnDevice() && !otherEntry.fromAccountStore);
+    if (alreadyHasCopyFromStore) {
+      return false;
+    }
+    if (JSON.stringify(this.urls_) !== JSON.stringify(otherEntry.urls)) {
+      return false;
+    }
     this.setId(otherEntry.id, otherEntry.fromAccountStore);
+    return true;
   }
 
   get urls() {

chrome/browser/resources/settings/autofill_page/multi_store_password_ui_entry.js

@@ -18,42 +18,41 @@ import {PasswordManagerProxy} from './password_manager_proxy.js';
  */
 export class MultiStorePasswordUiEntry extends MultiStoreIdHandler {
   /**
-   * Creates a multi-store item from duplicates |entry1| and (optional)
-   * |entry2|. If both arguments are passed, they should have the same contents
-   * but should be from different stores.
-   * @param {!PasswordManagerProxy.PasswordUiEntry} entry1
-   * @param {PasswordManagerProxy.PasswordUiEntry=} entry2
+   * @param {!PasswordManagerProxy.PasswordUiEntry} entry
    */
-  constructor(entry1, entry2) {
+  constructor(entry) {
     super();
 
     /** @type {!MultiStorePasswordUiEntry.Contents} */
-    this.contents_ = MultiStorePasswordUiEntry.getContents_(entry1);
+    this.contents_ = MultiStorePasswordUiEntry.getContents_(entry);
 
     /** @type {string} */
     this.password_ = '';
 
-    this.setId(entry1.id, entry1.fromAccountStore);
-
-    if (entry2) {
-      this.merge(entry2);
-    }
+    this.setId(entry.id, entry.fromAccountStore);
   }
 
   /**
    * Incorporates the id of |otherEntry|, as long as |otherEntry| matches
-   * |contents_| and the id corresponding to its store is not set.
+   * |contents_| and the id corresponding to its store is not set. If these
+   * preconditions are not satisfied, results in a no-op.
    * @param {!PasswordManagerProxy.PasswordUiEntry} otherEntry
+   * @return {boolean} Returns whether the merge succeeded.
    */
   // TODO(crbug.com/1102294) Consider asserting frontendId as well.
-  merge(otherEntry) {
-    assert(
-        (this.isPresentInAccount() && !otherEntry.fromAccountStore) ||
-        (this.isPresentOnDevice() && otherEntry.fromAccountStore));
-    assert(
-        JSON.stringify(this.contents_) ===
-        JSON.stringify(MultiStorePasswordUiEntry.getContents_(otherEntry)));
+  mergeInPlace(otherEntry) {
+    const alreadyHasCopyFromStore =
+        (this.isPresentInAccount() && otherEntry.fromAccountStore) ||
+        (this.isPresentOnDevice() && !otherEntry.fromAccountStore);
+    if (alreadyHasCopyFromStore) {
+      return false;
+    }
+    if (JSON.stringify(this.contents_) !==
+        JSON.stringify(MultiStorePasswordUiEntry.getContents_(otherEntry))) {
+      return false;
+    }
     this.setId(otherEntry.id, otherEntry.fromAccountStore);
+    return true;
   }
 
   /** @return {!PasswordManagerProxy.UrlCollection} */

chrome/test/data/webui/settings/multi_store_exception_entry_test.js

@@ -26,12 +26,30 @@ suite('MultiStoreExceptionEntry', function() {
     expectTrue(multiStoreAccountEntry.isPresentInAccount());
     expectEquals(multiStoreAccountEntry.getAnyId(), 1);
 
-    const multiStoreEntryFromBoth =
-        new MultiStoreExceptionEntry(deviceEntry, accountEntry);
+    const multiStoreEntryFromBoth = new MultiStoreExceptionEntry(deviceEntry);
+    expectTrue(multiStoreEntryFromBoth.mergeInPlace(accountEntry));
     expectTrue(multiStoreEntryFromBoth.isPresentOnDevice());
     expectTrue(multiStoreEntryFromBoth.isPresentInAccount());
     expectTrue(
         multiStoreEntryFromBoth.getAnyId() === 0 ||
         multiStoreEntryFromBoth.getAnyId() === 1);
   });
+
+  test('mergeFailsForRepeatedStore', function() {
+    const deviceEntry1 =
+        createExceptionEntry({url: 'g.com', id: 0, fromAccountStore: false});
+    const deviceEntry2 =
+        createExceptionEntry({url: 'g.com', id: 1, fromAccountStore: false});
+    expectFalse(
+        new MultiStoreExceptionEntry(deviceEntry1).mergeInPlace(deviceEntry2));
+  });
+
+  test('mergeFailsForDifferentContents', function() {
+    const deviceEntry =
+        createExceptionEntry({url: 'a.com', id: 0, fromAccountStore: false});
+    const accountEntry =
+        createExceptionEntry({url: 'b.com', id: 1, fromAccountStore: true});
+    expectFalse(
+        new MultiStoreExceptionEntry(deviceEntry).mergeInPlace(accountEntry));
+  });
 });

chrome/test/data/webui/settings/multi_store_password_ui_entry_test.js

@@ -26,12 +26,30 @@ suite('MultiStorePasswordUiEntry', function() {
     expectTrue(multiStoreAccountEntry.isPresentInAccount());
     expectEquals(multiStoreAccountEntry.getAnyId(), 1);
 
-    const multiStoreEntryFromBoth =
-        new MultiStorePasswordUiEntry(deviceEntry, accountEntry);
+    const multiStoreEntryFromBoth = new MultiStorePasswordUiEntry(deviceEntry);
+    expectTrue(multiStoreEntryFromBoth.mergeInPlace(accountEntry));
     expectTrue(multiStoreEntryFromBoth.isPresentOnDevice());
     expectTrue(multiStoreEntryFromBoth.isPresentInAccount());
     expectTrue(
         multiStoreEntryFromBoth.getAnyId() === 0 ||
         multiStoreEntryFromBoth.getAnyId() === 1);
   });
+
+  test('mergeFailsForRepeatedStore', function() {
+    const deviceEntry1 = createPasswordEntry(
+        {url: 'g.com', username: 'user', id: 0, fromAccountStore: false});
+    const deviceEntry2 = createPasswordEntry(
+        {url: 'g.com', username: 'user', id: 1, fromAccountStore: false});
+    expectFalse(
+        new MultiStorePasswordUiEntry(deviceEntry1).mergeInPlace(deviceEntry2));
+  });
+
+  test('mergeFailsForDifferentContents', function() {
+    const deviceEntry = createPasswordEntry(
+        {url: 'g.com', username: 'user', id: 0, fromAccountStore: false});
+    const accountEntry = createPasswordEntry(
+        {url: 'g.com', username: 'user2', id: 1, fromAccountStore: true});
+    expectFalse(
+        new MultiStorePasswordUiEntry(deviceEntry).mergeInPlace(accountEntry));
+  });
 });

chrome/test/data/webui/settings/passwords_and_autofill_fake_data.js

@@ -87,8 +87,13 @@ export function createMultiStorePasswordEntry(params) {
     });
   }
 
+  if (deviceEntry && accountEntry) {
+    const mergedEntry = new MultiStorePasswordUiEntry(deviceEntry);
+    mergedEntry.mergeInPlace(accountEntry);
+    return mergedEntry;
+  }
   if (deviceEntry) {
-    return new MultiStorePasswordUiEntry(deviceEntry, accountEntry);
+    return new MultiStorePasswordUiEntry(deviceEntry);
   }
   if (accountEntry) {
     return new MultiStorePasswordUiEntry(accountEntry);
@@ -158,8 +163,13 @@ export function createMultiStoreExceptionEntry(params) {
     });
   }
 
+  if (deviceEntry && accountEntry) {
+    const mergedEntry = new MultiStoreExceptionEntry(deviceEntry);
+    mergedEntry.mergeInPlace(accountEntry);
+    return mergedEntry;
+  }
   if (deviceEntry) {
-    return new MultiStoreExceptionEntry(deviceEntry, accountEntry);
+    return new MultiStoreExceptionEntry(deviceEntry);
   }
   if (accountEntry) {
     return new MultiStoreExceptionEntry(accountEntry);