Bug 1404427 - Sync multiple form history deletions. r=kitcambridge
☠☠ backed out by 7e046d162394 ☠ ☠
authorEdouard Oger <eoger@fastmail.com>
Mon, 06 Nov 2017 15:50:28 -0500
changeset 444320 6aa22b862a45b7db9a09175d2b939c4a79a80de7
parent 444244 c2e7d45a29a26698b5fe5976a39a9e001a2d68e5
child 444321 7e505b1a27ad28dcba4b83f7db51b35406be17fc
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskitcambridge
bugs1404427
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1404427 - Sync multiple form history deletions. r=kitcambridge MozReview-Commit-ID: H7AmIBtFUOr
toolkit/components/satchel/FormHistory.jsm
toolkit/components/satchel/test/unit/head_satchel.js
toolkit/components/satchel/test/unit/test_notify.js
--- a/toolkit/components/satchel/FormHistory.jsm
+++ b/toolkit/components/satchel/FormHistory.jsm
@@ -614,17 +614,17 @@ function dbClose(aShutdown) {
 
 /**
  * Constructs and executes database statements from a pre-processed list of
  * inputted changes.
  *
  * @param {Array.<Object>} aChanges changes to form history
  * @param {Object} aCallbacks
  */
-function updateFormHistoryWrite(aChanges, aCallbacks) {
+async function updateFormHistoryWrite(aChanges, aCallbacks) {
   log("updateFormHistoryWrite  " + aChanges.length);
 
   // pass 'now' down so that every entry in the batch has the same timestamp
   let now = Date.now() * 1000;
 
   // for each change, we either create and append a new storage statement to
   // stmts or bind a new set of parameters to an existing storage statement.
   // stmts and bindingArrays are updated when makeXXXStatement eventually
@@ -643,17 +643,40 @@ function updateFormHistoryWrite(aChanges
         let delStmt = makeMoveToDeletedStatement(change.guid, now, change, bindingArrays);
         if (delStmt && !stmts.includes(delStmt)) {
           stmts.push(delStmt);
         }
         if ("timeDeleted" in change) {
           delete change.timeDeleted;
         }
         stmt = makeRemoveStatement(change, bindingArrays);
-        notifications.push(["formhistory-remove", change.guid]);
+
+        // Fetch the GUIDs we are going to delete.
+        try {
+          await new Promise((res, rej) => {
+            let selectStmt = makeSearchStatement(change, ["guid"]);
+            let selectHandlers = {
+              handleCompletion() {
+                res();
+              },
+              handleError() {
+                log("remove select guids failure");
+              },
+              handleResult(aResultSet) {
+                for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
+                  notifications.push(["formhistory-remove", row.getResultByName("guid")]);
+                }
+              },
+            };
+            dbConnection.executeAsync([selectStmt], 1, selectHandlers);
+          });
+        } catch (e) {
+          log("Error in select statement: " + e);
+        }
+
         break;
       case "update":
         log("Update form history " + change);
         let guid = change.guid;
         delete change.guid;
         // a special case for updating the GUID - the new value can be
         // specified in newGuid.
         if (change.newGuid) {
--- a/toolkit/components/satchel/test/unit/head_satchel.js
+++ b/toolkit/components/satchel/test/unit/head_satchel.js
@@ -97,16 +97,34 @@ function addEntry(name, value, then) {
     fieldname: name,
     value,
     timesUsed: 1,
     firstUsed: now,
     lastUsed: now,
   }, then);
 }
 
+function promiseCountEntries(name, value) {
+  return new Promise(res => {
+    countEntries(name, value, res);
+  });
+}
+
+function promiseUpdateEntry(op, name, value) {
+  return new Promise(res => {
+    updateEntry(op, name, value, res);
+  });
+}
+
+function promiseAddEntry(name, value) {
+  return new Promise(res => {
+    addEntry(name, value, res);
+  });
+}
+
 // Wrapper around FormHistory.update which handles errors. Calls then() when done.
 function updateFormHistory(changes, then) {
   FormHistory.update(changes, {
     handleError(error) {
       do_throw("Error occurred updating form history: " + error);
     },
     handleCompletion(reason) {
       if (!reason) {
--- a/toolkit/components/satchel/test/unit/test_notify.js
+++ b/toolkit/components/satchel/test/unit/test_notify.js
@@ -1,200 +1,180 @@
 /*
  * Test suite for satchel notifications
  *
  * Tests notifications dispatched when modifying form history.
  *
  */
 
-let expectedNotification;
-let expectedData;
-let subjectIsGuid = false;
-let lastGUID;
-
-let TestObserver = {
-  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
-
-  observe(subject, topic, data) {
-    do_check_eq(topic, "satchel-storage-changed");
-    do_check_eq(data, expectedNotification);
-
-    let verifySubjectIsGuid = () => {
-      do_check_true(subject instanceof Ci.nsISupportsString);
-      do_check_true(isGUID.test(subject.toString()));
-      lastGUID = subject.toString();
-    };
+XPCOMUtils.defineLazyModuleGetter(this, "setTimeout", "resource://gre/modules/Timer.jsm");
 
-    switch (data) {
-      case "formhistory-add":
-      case "formhistory-update":
-        verifySubjectIsGuid();
-        break;
-      case "formhistory-remove":
-        if (subjectIsGuid) {
-          verifySubjectIsGuid();
-        } else {
-          do_check_eq(null, subject);
-        }
-        break;
-      default:
-        do_throw("Unhandled notification: " + data + " / " + topic);
+const TestObserver = {
+  observed: [],
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
+  observe(subject, topic, data) {
+    if (subject instanceof Ci.nsISupportsString) {
+      subject = subject.toString();
     }
-
-    expectedNotification = null;
-    expectedData = null;
+    this.observed.push({subject, topic, data});
+  },
+  reset() {
+    this.observed = [];
   },
 };
 
-let testIterator = null;
-
-function run_test() {
-  do_test_pending();
-  testIterator = run_test_steps();
-  testIterator.next();
-}
+const entry1 = ["entry1", "value1"];
+const entry2 = ["entry2", "value2"];
+const entry3 = ["entry3", "value3"];
 
-function next_test() {
-  testIterator.next();
-}
-
-function* run_test_steps() {
-  let testnum = 0;
-  let testdesc = "Setup of test form history entries";
+add_task(async function setup() {
+  await promiseUpdateEntry("remove", null, null);
+  const count = await promiseCountEntries(null, null);
+  do_check_false(count, "Checking initial DB is empty");
 
-  try {
-    let entry1 = ["entry1", "value1"];
-
-    /* ========== 1 ========== */
-    testnum = 1;
-    testdesc = "Initial connection to storage module";
+  // Add the observer
+  Services.obs.addObserver(TestObserver, "satchel-storage-changed");
+});
 
-    yield updateEntry("remove", null, null, next_test);
-    yield countEntries(null, null, function(num) {
-      do_check_false(num, "Checking initial DB is empty");
-      next_test();
-    });
+add_task(async function addAndUpdateEntry() {
+  // Add
+  await promiseUpdateEntry("add", entry1[0], entry1[1]);
+  do_check_eq(TestObserver.observed.length, 1);
+  let {subject, data} = TestObserver.observed[0];
+  do_check_eq(data, "formhistory-add");
+  do_check_true(isGUID.test(subject));
 
-    // Add the observer
-    Services.obs.addObserver(TestObserver, "satchel-storage-changed");
-
-    /* ========== 2 ========== */
-    testnum++;
-    testdesc = "addEntry";
+  let count = await promiseCountEntries(entry1[0], entry1[1]);
+  do_check_eq(count, 1);
 
-    expectedNotification = "formhistory-add";
-    expectedData = entry1;
-
-    yield updateEntry("add", entry1[0], entry1[1], next_test);
-    do_check_eq(expectedNotification, null); // check that observer got a notification
+  // Update
+  TestObserver.reset();
 
-    yield countEntries(entry1[0], entry1[1], function(num) {
-      do_check_true(num > 0);
-      next_test();
-    });
+  await promiseUpdateEntry("update", entry1[0], entry1[1]);
+  do_check_eq(TestObserver.observed.length, 1);
+  ({subject, data} = TestObserver.observed[0]);
+  do_check_eq(data, "formhistory-update");
+  do_check_true(isGUID.test(subject));
 
-    /* ========== 3 ========== */
-    testnum++;
-    testdesc = "modifyEntry";
+  count = await promiseCountEntries(entry1[0], entry1[1]);
+  do_check_eq(count, 1);
 
-    expectedNotification = "formhistory-update";
-    expectedData = entry1;
-    // will update previous entry
-    yield updateEntry("update", entry1[0], entry1[1], next_test);
-    yield countEntries(entry1[0], entry1[1], function(num) {
-      do_check_true(num > 0);
-      next_test();
-    });
+  // Clean-up
+  await promiseUpdateEntry("remove", null, null);
+});
 
-    do_check_eq(expectedNotification, null);
-
-    /* ========== 4 ========== */
-    testnum++;
-    testdesc = "removeEntry";
+add_task(async function removeEntry() {
+  TestObserver.reset();
+  await promiseUpdateEntry("add", entry1[0], entry1[1]);
+  const guid = TestObserver.observed[0].subject;
+  TestObserver.reset();
 
-    expectedNotification = "formhistory-remove";
-    expectedData = entry1;
-
-    subjectIsGuid = true;
-    yield FormHistory.update({
+  await new Promise(res => {
+    FormHistory.update({
       op: "remove",
       fieldname: entry1[0],
       value: entry1[1],
-      guid: lastGUID,
+      guid,
     }, {
       handleError(error) {
         do_throw("Error occurred updating form history: " + error);
       },
       handleCompletion(reason) {
         if (!reason) {
-          next_test();
+          res();
         }
       },
     });
-    subjectIsGuid = false;
+  });
+  do_check_eq(TestObserver.observed.length, 1);
+  const {subject, data} = TestObserver.observed[0];
+  do_check_eq(data, "formhistory-remove");
+  do_check_true(isGUID.test(subject));
 
-    do_check_eq(expectedNotification, null);
-    yield countEntries(entry1[0], entry1[1], function(num) {
-      do_check_false(num, "doesn't exist after remove");
-      next_test();
-    });
+  const count = await promiseCountEntries(entry1[0], entry1[1]);
+  do_check_eq(count, 0, "doesn't exist after remove");
+});
 
-    /* ========== 5 ========== */
-    testnum++;
-    testdesc = "removeAllEntries";
+add_task(async function removeAllEntries() {
+  await promiseAddEntry(entry1[0], entry1[1]);
+  await promiseAddEntry(entry2[0], entry2[1]);
+  await promiseAddEntry(entry3[0], entry3[1]);
+  TestObserver.reset();
 
-    expectedNotification = "formhistory-remove";
-    expectedData = null; // no data expected
-    yield updateEntry("remove", null, null, next_test);
+  await promiseUpdateEntry("remove", null, null);
+  do_check_eq(TestObserver.observed.length, 3);
+  for (const notification of TestObserver.observed) {
+    const {subject, data} = notification;
+    do_check_eq(data, "formhistory-remove");
+    do_check_true(isGUID.test(subject));
+  }
 
-    do_check_eq(expectedNotification, null);
-
-    /* ========== 6 ========== */
-    testnum++;
-    testdesc = "removeAllEntries (again)";
+  const count = await promiseCountEntries(null, null);
+  do_check_eq(count, 0);
+});
 
-    expectedNotification = "formhistory-remove";
-    expectedData = null;
-    yield updateEntry("remove", null, null, next_test);
-
-    do_check_eq(expectedNotification, null);
+add_task(async function removeEntriesForName() {
+  await promiseAddEntry(entry1[0], entry1[1]);
+  await promiseAddEntry(entry2[0], entry2[1]);
+  await promiseAddEntry(entry3[0], entry3[1]);
+  TestObserver.reset();
 
-    /* ========== 7 ========== */
-    testnum++;
-    testdesc = "removeEntriesForName";
+  await promiseUpdateEntry("remove", entry2[0], null);
+  do_check_eq(TestObserver.observed.length, 1);
+  const {subject, data} = TestObserver.observed[0];
+  do_check_eq(data, "formhistory-remove");
+  do_check_true(isGUID.test(subject));
 
-    expectedNotification = "formhistory-remove";
-    expectedData = "field2";
-    yield updateEntry("remove", null, "field2", next_test);
+  let count = await promiseCountEntries(entry2[0], entry2[1]);
+  do_check_eq(count, 0);
+
+  count = await promiseCountEntries(null, null);
+  do_check_eq(count, 2, "the other entries are still there");
 
-    do_check_eq(expectedNotification, null);
+  // Clean-up
+  await promiseUpdateEntry("remove", null, null);
+});
 
-    /* ========== 8 ========== */
-    testnum++;
-    testdesc = "removeEntriesByTimeframe";
+add_task(async function removeEntriesByTimeframe() {
+  await promiseAddEntry(entry1[0], entry1[1]);
+  await promiseAddEntry(entry2[0], entry2[1]);
 
-    expectedNotification = "formhistory-remove";
-    expectedData = [10, 99999999999];
+  const cutoffDate = Date.now();
+  // eslint-disable-next-line mozilla/no-arbitrary-setTimeout
+  await new Promise(res => setTimeout(res, 10));
 
-    yield FormHistory.update({
+  await promiseAddEntry(entry3[0], entry3[1]);
+  TestObserver.reset();
+
+  await new Promise(res => {
+    FormHistory.update({
       op: "remove",
-      firstUsedStart: expectedData[0],
-      firstUsedEnd: expectedData[1],
+      firstUsedStart: 10,
+      firstUsedEnd: cutoffDate * 1000,
     }, {
       handleCompletion(reason) {
         if (!reason) {
-          next_test();
+          res();
         }
       },
       handleErrors(error) {
         do_throw("Error occurred updating form history: " + error);
       },
     });
-
-    do_check_eq(expectedNotification, null);
-
-    Services.obs.removeObserver(TestObserver, "satchel-storage-changed");
+  });
+  do_check_eq(TestObserver.observed.length, 2);
+  for (const notification of TestObserver.observed) {
+    const {subject, data} = notification;
+    do_check_eq(data, "formhistory-remove");
+    do_check_true(isGUID.test(subject));
+  }
 
-    do_test_finished();
-  } catch (e) {
-    throw new Error(`FAILED in test #${testnum} -- ${testdesc}: ${e}`);
-  }
-}
+  const count = await promiseCountEntries(null, null);
+  do_check_eq(count, 1, "entry2 should still be there");
+
+  // Clean-up
+  await promiseUpdateEntry("remove", null, null);
+});
+
+add_task(async function teardown() {
+  await promiseUpdateEntry("remove", null, null);
+  Services.obs.removeObserver(TestObserver, "satchel-storage-changed");
+});