From 4f9b7d04fa0043cbfe3252209e337522f6050753 Mon Sep 17 00:00:00 2001 From: Edouard Oger Date: Mon, 6 Nov 2017 15:50:28 -0500 Subject: [PATCH] Bug 1404427 - Sync multiple form history deletions. r=kitcambridge MozReview-Commit-ID: H7AmIBtFUOr --HG-- extra : rebase_source : 596e4470b64e9406432c4d54f7ef90138a032e62 --- .../general/browser_sanitize-timespans.js | 2 +- .../components/search/test/browser_426329.js | 13 + toolkit/components/satchel/FormHistory.jsm | 27 +- .../satchel/test/unit/head_satchel.js | 18 ++ .../satchel/test/unit/test_notify.js | 260 ++++++++---------- 5 files changed, 177 insertions(+), 143 deletions(-) diff --git a/browser/base/content/test/general/browser_sanitize-timespans.js b/browser/base/content/test/general/browser_sanitize-timespans.js index 4fda94e9469c..5b0a5066c7f6 100644 --- a/browser/base/content/test/general/browser_sanitize-timespans.js +++ b/browser/base/content/test/general/browser_sanitize-timespans.js @@ -1,7 +1,7 @@ Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://gre/modules/Services.jsm"); -requestLongerTimeout(2); +requestLongerTimeout(5); // Bug 453440 - Test the timespan-based logic of the sanitizer code var now_mSec = Date.now(); diff --git a/browser/components/search/test/browser_426329.js b/browser/components/search/test/browser_426329.js index 4f0120bb5019..66fc09de617e 100644 --- a/browser/components/search/test/browser_426329.js +++ b/browser/components/search/test/browser_426329.js @@ -249,7 +249,10 @@ add_task(async function testClearHistory() { let controller = searchBar.textbox.controllers.getControllerForCommand("cmd_clearhistory"); ok(controller.isCommandEnabled("cmd_clearhistory"), "Clear history command enabled"); + + let historyCleared = promiseObserver("satchel-storage-changed"); controller.doCommand("cmd_clearhistory"); + await historyCleared; let count = await countEntries(); ok(count == 0, "History cleared"); }); @@ -262,3 +265,13 @@ add_task(async function asyncCleanup() { gBrowser.selectedBrowser.loadURI("about:blank"); await promiseRemoveEngine(); }); + +function promiseObserver(topic) { + return new Promise(resolve => { + let obs = (aSubject, aTopic, aData) => { + Services.obs.removeObserver(obs, aTopic); + resolve(aSubject); + }; + Services.obs.addObserver(obs, topic); + }); +} diff --git a/toolkit/components/satchel/FormHistory.jsm b/toolkit/components/satchel/FormHistory.jsm index 6c77abb64e7c..334136759c53 100644 --- a/toolkit/components/satchel/FormHistory.jsm +++ b/toolkit/components/satchel/FormHistory.jsm @@ -619,7 +619,7 @@ function dbClose(aShutdown) { * @param {Array.} 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 @@ -648,7 +648,30 @@ function updateFormHistoryWrite(aChanges, aCallbacks) { 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); diff --git a/toolkit/components/satchel/test/unit/head_satchel.js b/toolkit/components/satchel/test/unit/head_satchel.js index 4add2eccb398..637a90653e17 100644 --- a/toolkit/components/satchel/test/unit/head_satchel.js +++ b/toolkit/components/satchel/test/unit/head_satchel.js @@ -102,6 +102,24 @@ function addEntry(name, value, then) { }, 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, { diff --git a/toolkit/components/satchel/test/unit/test_notify.js b/toolkit/components/satchel/test/unit/test_notify.js index cdcdb14824b8..1755e846dc00 100644 --- a/toolkit/components/satchel/test/unit/test_notify.js +++ b/toolkit/components/satchel/test/unit/test_notify.js @@ -5,196 +5,176 @@ * */ -let expectedNotification; -let expectedData; -let subjectIsGuid = false; -let lastGUID; +XPCOMUtils.defineLazyModuleGetter(this, "setTimeout", "resource://gre/modules/Timer.jsm"); -let TestObserver = { +const TestObserver = { + observed: [], 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(); - }; - - 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); + if (subject instanceof Ci.nsISupportsString) { + subject = subject.toString(); } - - expectedNotification = null; - expectedData = null; + this.observed.push({subject, topic, data}); + }, + reset() { + this.observed = []; }, }; -let testIterator = null; +const entry1 = ["entry1", "value1"]; +const entry2 = ["entry2", "value2"]; +const entry3 = ["entry3", "value3"]; -function run_test() { - do_test_pending(); - testIterator = run_test_steps(); - testIterator.next(); -} +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"); -function next_test() { - testIterator.next(); -} + // Add the observer + Services.obs.addObserver(TestObserver, "satchel-storage-changed"); +}); -function* run_test_steps() { - let testnum = 0; - let testdesc = "Setup of test form history entries"; +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)); - try { - let entry1 = ["entry1", "value1"]; + let count = await promiseCountEntries(entry1[0], entry1[1]); + do_check_eq(count, 1); - /* ========== 1 ========== */ - testnum = 1; - testdesc = "Initial connection to storage module"; + // Update + TestObserver.reset(); - yield updateEntry("remove", null, null, next_test); - yield countEntries(null, null, function(num) { - do_check_false(num, "Checking initial DB is empty"); - 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)); - // Add the observer - Services.obs.addObserver(TestObserver, "satchel-storage-changed"); + count = await promiseCountEntries(entry1[0], entry1[1]); + do_check_eq(count, 1); - /* ========== 2 ========== */ - testnum++; - testdesc = "addEntry"; + // Clean-up + await promiseUpdateEntry("remove", null, null); +}); - expectedNotification = "formhistory-add"; - expectedData = entry1; +add_task(async function removeEntry() { + TestObserver.reset(); + await promiseUpdateEntry("add", entry1[0], entry1[1]); + const guid = TestObserver.observed[0].subject; + TestObserver.reset(); - yield updateEntry("add", entry1[0], entry1[1], next_test); - do_check_eq(expectedNotification, null); // check that observer got a notification - - yield countEntries(entry1[0], entry1[1], function(num) { - do_check_true(num > 0); - next_test(); - }); - - /* ========== 3 ========== */ - testnum++; - testdesc = "modifyEntry"; - - 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(); - }); - - do_check_eq(expectedNotification, null); - - /* ========== 4 ========== */ - testnum++; - testdesc = "removeEntry"; - - 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); + const count = await promiseCountEntries(null, null); + do_check_eq(count, 0); +}); - /* ========== 6 ========== */ - testnum++; - testdesc = "removeAllEntries (again)"; +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(); - expectedNotification = "formhistory-remove"; - expectedData = null; - yield updateEntry("remove", null, null, next_test); + 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)); - do_check_eq(expectedNotification, null); + let count = await promiseCountEntries(entry2[0], entry2[1]); + do_check_eq(count, 0); - /* ========== 7 ========== */ - testnum++; - testdesc = "removeEntriesForName"; + count = await promiseCountEntries(null, null); + do_check_eq(count, 2, "the other entries are still there"); - expectedNotification = "formhistory-remove"; - expectedData = "field2"; - yield updateEntry("remove", null, "field2", next_test); + // Clean-up + await promiseUpdateEntry("remove", null, null); +}); - do_check_eq(expectedNotification, null); +add_task(async function removeEntriesByTimeframe() { + await promiseAddEntry(entry1[0], entry1[1]); + await promiseAddEntry(entry2[0], entry2[1]); - /* ========== 8 ========== */ - testnum++; - testdesc = "removeEntriesByTimeframe"; + const cutoffDate = Date.now(); + // eslint-disable-next-line mozilla/no-arbitrary-setTimeout + await new Promise(res => setTimeout(res, 10)); - expectedNotification = "formhistory-remove"; - expectedData = [10, 99999999999]; + await promiseAddEntry(entry3[0], entry3[1]); + TestObserver.reset(); - yield FormHistory.update({ + 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_test_finished(); - } catch (e) { - throw new Error(`FAILED in test #${testnum} -- ${testdesc}: ${e}`); + }); + 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)); } -} + + 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"); +});