Bug 1909424 - Fix the creation of tombstone records when accounts are removed. r=aleca

We need to clear the data from the cache, or an ordinary record will be created. This should
happen in a synchronous way to avoid possible race conditions.

Differential Revision: https://phabricator.services.mozilla.com/D217391

--HG--
extra : moz-landing-system : lando

mail/services/sync/modules/CachedStore.sys.mjs

@@ -45,38 +45,92 @@ async function setData(engine, key, value) {
   jsonFile.saveSoon();
 }
 
+/**
+ * An engine store that automatically caches data sent to and from the server.
+ *
+ * @extends {Store}
+ * @see {engines.sys.mjs}
+ */
 export function CachedStore(name, engine) {
   Store.call(this, name, engine);
 }
 CachedStore.prototype = {
   __proto__: Store.prototype,
+  _deleted: new Set(),
 
+  /**
+   * Apply a single record against the store.
+   *
+   * @param {CryptoWrapper} record
+   */
   async create(record) {
     await setData(this.name, record.id, record.cleartext);
   },
 
+  /**
+   * Remove an item in the store from a record.
+   *
+   * @param {CryptoWrapper} record
+   */
   async remove(record) {
     await setData(this.name, record.id, null);
   },
 
+  /**
+   * Remove record data from the cache, but first record the record's ID as
+   * deleted, so we can return immediately.
+   *
+   * @param {string} id
+   */
+  markDeleted(id) {
+    this._deleted.add(id);
+    setData(this.name, id, null).then(() => this._deleted.delete(id));
+  },
+
+  /**
+   * Update an item from a record.
+   *
+   * @param {CryptoWrapper} record
+   */
   async update(record) {
     await setData(this.name, record.id, record.cleartext);
   },
 
+  /**
+   * Determine whether a record with the specified ID exists.
+   *
+   * @param {string} id
+   * @return {boolean}
+   */
   async itemExists(id) {
     return id in (await this.getAllIDs());
   },
 
+  /**
+   * Obtain the set of all known record IDs.
+   *
+   * @return {object}
+   */
   async getAllIDs() {
     const ids = {};
     const keys = await getDataKeys(this.name);
     for (const k of keys) {
-      ids[k] = true;
+      if (!this._deleted.has(ids[k])) {
+        ids[k] = true;
+      }
     }
     return ids;
   },
 
+  /**
+   * Get record data from the cache, but not if the ID is marked as deleted.
+   *
+   * @return {object}
+   */
   async getCreateRecordData(id) {
+    if (this._deleted.has(id)) {
+      return null;
+    }
     return getData(this.name, id);
   },
 };

mail/services/sync/modules/engines/addressBooks.sys.mjs

@@ -338,8 +338,11 @@ AddressBookTracker.prototype = {
         break;
       }
       case "addrbook-directory-created":
+        book = subject;
+        break;
       case "addrbook-directory-deleted":
         book = subject;
+        this.engine._store.markDeleted(book.UID);
         break;
     }
 

mail/services/sync/modules/engines/calendars.sys.mjs

@@ -308,6 +308,7 @@ CalendarTracker.prototype = {
     }
 
     if (shouldSyncCalendar(calendar)) {
+      this.engine._store.markDeleted(calendar.id);
       this._changedIDs.add(calendar.id);
       this.score += SCORE_INCREMENT_XLARGE;
     }

mail/services/sync/modules/engines/identities.sys.mjs

@@ -302,12 +302,16 @@ IdentityTracker.prototype = {
       }
     };
 
-    if (
-      ["account-identity-added", "account-identity-removed"].includes(topic)
-    ) {
+    if (topic == "account-identity-added") {
       markAsChanged(subject.QueryInterface(Ci.nsIMsgIdentity));
       return;
     }
+    if (topic == "account-identity-removed") {
+      subject.QueryInterface(Ci.nsIMsgIdentity);
+      this.engine._store.markDeleted(subject.UID);
+      markAsChanged(subject);
+      return;
+    }
 
     const idKey = data.split(".")[2];
     const prefName = data.substring(idKey.length + 15);

mail/services/sync/modules/engines/servers.sys.mjs

@@ -376,8 +376,10 @@ ServerTracker.prototype = {
     let server;
     if (topic == "message-server-removed") {
       server = subject.QueryInterface(Ci.nsIMsgIncomingServer);
+      this.engine._store.markDeleted(server.UID);
     } else if (topic == "message-smtpserver-removed") {
       server = subject.QueryInterface(Ci.nsISmtpServer);
+      this.engine._store.markDeleted(server.UID);
     } else {
       const [, group, serverKey] = data.split(".", 3);
       const prefName = data.substring(group.length + serverKey.length + 7);

mail/services/sync/test/unit/head.js

@@ -10,6 +10,12 @@ const { BulkKeyBundle } = ChromeUtils.importESModule(
 const { MailServices } = ChromeUtils.importESModule(
   "resource:///modules/MailServices.sys.mjs"
 );
+const { setTimeout } = ChromeUtils.importESModule(
+  "resource://gre/modules/Timer.sys.mjs"
+);
+const { TestUtils } = ChromeUtils.importESModule(
+  "resource://testing-common/TestUtils.sys.mjs"
+);
 
 add_setup(async function () {
   try {
@@ -118,8 +124,14 @@ async function roundTripRecord(record, constructor) {
  *
  * @param {Tracker} tracker
  * @param {string} expectedUID - The UID of an object that has changed.
+ * @param {CryptoWrapper} record - The result of `createRecord` called before
+ *   clearing the tracker.
  */
 async function assertChangeTracked(tracker, expectedUID) {
+  await TestUtils.waitForCondition(
+    () => tracker.engine.score > 0,
+    "waiting for tracker score to change"
+  );
   Assert.equal(
     tracker.engine.score,
     301,
@@ -131,8 +143,12 @@ async function assertChangeTracked(tracker, expectedUID) {
     `${expectedUID} is marked as changed`
   );
 
+  const record = await tracker.engine._store.createRecord(expectedUID);
+
   tracker.clearChangedIDs();
   tracker.resetScore();
+
+  return record;
 }
 
 /**
@@ -141,6 +157,8 @@ async function assertChangeTracked(tracker, expectedUID) {
  * @param {Tracker} tracker
  */
 async function assertNoChangeTracked(tracker) {
+  // Wait a bit, to prove nothing happened.
+  await new Promise(resolve => setTimeout(resolve, 250));
   Assert.equal(
     tracker.engine.score,
     0,

mail/services/sync/test/unit/test_addressBook_tracker.js

@@ -8,9 +8,6 @@ const { AddressBooksEngine, AddressBookRecord } = ChromeUtils.importESModule(
 const { Service } = ChromeUtils.importESModule(
   "resource://services-sync/service.sys.mjs"
 );
-const { TestUtils } = ChromeUtils.importESModule(
-  "resource://testing-common/TestUtils.sys.mjs"
-);
 
 let engine, store, tracker;
 
@@ -58,7 +55,9 @@ add_task(async function testCardDAVAddressBook() {
   const deletedPromise = TestUtils.topicObserved("addrbook-directory-deleted");
   MailServices.ab.deleteAddressBook(book.URI);
   await deletedPromise;
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, AddressBookRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });
 
@@ -94,7 +93,9 @@ add_task(async function testLDAPAddressBook() {
   const deletedPromise = TestUtils.topicObserved("addrbook-directory-deleted");
   MailServices.ab.deleteAddressBook(book.URI);
   await deletedPromise;
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, AddressBookRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });
 

mail/services/sync/test/unit/test_calendar_tracker.js

@@ -63,7 +63,9 @@ add_task(async function testNetworkCalendar() {
 
   cal.manager.unregisterCalendar(calendar);
   cal.manager.removeCalendar(calendar);
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, CalendarRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });
 

mail/services/sync/test/unit/test_identity_tracker.js

@@ -81,7 +81,9 @@ add_task(async function testIdentity() {
   ]);
 
   accountA.removeIdentity(identity);
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, IdentityRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });
 

mail/services/sync/test/unit/test_server_tracker.js

@@ -58,7 +58,9 @@ add_task(async function testTrackingIncomingServers() {
   ]);
 
   MailServices.accounts.removeIncomingServer(incomingServer, false);
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, ServerRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });
 
@@ -84,7 +86,9 @@ add_task(async function testTrackingOutgoingServers() {
   ]);
 
   MailServices.outgoingServer.deleteServer(outgoingServer);
-  await assertChangeTracked(tracker, id);
+  let record = await assertChangeTracked(tracker, id);
+  record = await roundTripRecord(record, ServerRecord);
+  Assert.ok(record.deleted, "record should be a tombstone record");
   await assertNoChangeTracked(tracker);
 });