From e4007fdbe7a111740aaf4b588b48fceddf698feb Mon Sep 17 00:00:00 2001 From: Luciano I Date: Tue, 31 Oct 2017 18:50:31 -0400 Subject: [PATCH] Bug 1375223 - Remove Async.querySpinningly. r=kitcambridge MozReview-Commit-ID: bMo1jyIY5g --HG-- extra : rebase_source : ac69fde2cb8216300bdb9e7d19528c15cdceb7c8 --- services/common/async.js | 55 -------- .../tests/unit/test_async_querySpinningly.js | 121 ------------------ services/common/tests/unit/xpcshell.ini | 1 - services/sync/modules/telemetry.js | 4 - .../sync/tests/unit/test_history_store.js | 33 ++--- .../tests/unit/test_places_guid_downgrade.js | 77 ++++++----- services/sync/tests/unit/test_telemetry.js | 25 ---- .../tps/resource/modules/history.jsm | 53 +------- .../sync/tps/extensions/tps/resource/tps.jsm | 6 +- 9 files changed, 58 insertions(+), 317 deletions(-) delete mode 100644 services/common/tests/unit/test_async_querySpinningly.js diff --git a/services/common/async.js b/services/common/async.js index e9b9475a66ad..5c075090c56b 100644 --- a/services/common/async.js +++ b/services/common/async.js @@ -167,61 +167,6 @@ this.Async = { return callback; }, - // Prototype for mozIStorageCallback, used in querySpinningly. - // This allows us to define the handle* functions just once rather - // than on every querySpinningly invocation. - _storageCallbackPrototype: { - results: null, - - // These are set by queryAsync. - names: null, - syncCb: null, - - handleResult: function handleResult(results) { - if (!this.names) { - return; - } - if (!this.results) { - this.results = []; - } - let row; - while ((row = results.getNextRow()) != null) { - let item = {}; - for (let name of this.names) { - item[name] = row.getResultByName(name); - } - this.results.push(item); - } - }, - handleError: function handleError(error) { - this.syncCb.throw(error); - }, - handleCompletion: function handleCompletion(reason) { - - // If we got an error, handleError will also have been called, so don't - // call the callback! We never cancel statements, so we don't need to - // address that quandary. - if (reason == REASON_ERROR) - return; - - // If we were called with column names but didn't find any results, - // the calling code probably still expects an array as a return value. - if (this.names && !this.results) { - this.results = []; - } - this.syncCb(this.results); - } - }, - - querySpinningly: function querySpinningly(query, names) { - // 'Synchronously' asyncExecute, fetching all results by name. - let storageCallback = Object.create(Async._storageCallbackPrototype); - storageCallback.names = names; - storageCallback.syncCb = Async.makeSyncCallback(); - query.executeAsync(storageCallback); - return Async.waitForSyncCallback(storageCallback.syncCb); - }, - promiseSpinningly(promise) { let cb = Async.makeSpinningCallback(); promise.then(result => { diff --git a/services/common/tests/unit/test_async_querySpinningly.js b/services/common/tests/unit/test_async_querySpinningly.js deleted file mode 100644 index 1ac7204dff08..000000000000 --- a/services/common/tests/unit/test_async_querySpinningly.js +++ /dev/null @@ -1,121 +0,0 @@ -/* Any copyright is dedicated to the Public Domain. - * http://creativecommons.org/publicdomain/zero/1.0/ */ - -Cu.import("resource://gre/modules/XPCOMUtils.jsm"); -Cu.import("resource://gre/modules/Services.jsm"); -Cu.import("resource://gre/modules/FormHistory.jsm"); -Cu.import("resource://services-common/async.js"); -Cu.import("resource://services-common/utils.js"); - -_("Make sure querySpinningly will synchronously fetch rows for a query asyncly"); - -const SQLITE_CONSTRAINT_VIOLATION = 19; // http://www.sqlite.org/c3ref/c_abort.html - -// This test is a bit hacky - it was originally written to use the -// formhistory.sqlite database using the nsIFormHistory2 sync APIs. However, -// that's now been deprecated in favour of the async FormHistory.jsm. -// Rather than re-write the test completely, we cheat - we use FormHistory.jsm -// to initialize the database, then we just re-open it for these tests. - -// Init the forms database. -FormHistory.schemaVersion; -FormHistory.shutdown(); - -// and open the database it just created. -let dbFile = Services.dirsvc.get("ProfD", Ci.nsIFile).clone(); -dbFile.append("formhistory.sqlite"); -let dbConnection = Services.storage.openUnsharedDatabase(dbFile); - -do_register_cleanup(() => { - let cb = Async.makeSpinningCallback(); - dbConnection.asyncClose(cb); - return cb.wait(); -}); - -function querySpinningly(query, names) { - let q = dbConnection.createStatement(query); - let r = Async.querySpinningly(q, names); - q.finalize(); - return r; -} - -function run_test() { - initTestLogging("Trace"); - - _("Make sure the call is async and allows other events to process"); - let isAsync = false; - CommonUtils.nextTick(function() { isAsync = true; }); - do_check_false(isAsync); - - _("Empty out the formhistory table"); - let r0 = querySpinningly("DELETE FROM moz_formhistory"); - do_check_eq(r0, null); - - _("Make sure there's nothing there"); - let r1 = querySpinningly("SELECT 1 FROM moz_formhistory"); - do_check_eq(r1, null); - - _("Insert a row"); - let r2 = querySpinningly("INSERT INTO moz_formhistory (fieldname, value) VALUES ('foo', 'bar')"); - do_check_eq(r2, null); - - _("Request a known value for the one row"); - let r3 = querySpinningly("SELECT 42 num FROM moz_formhistory", ["num"]); - do_check_eq(r3.length, 1); - do_check_eq(r3[0].num, 42); - - _("Get multiple columns"); - let r4 = querySpinningly("SELECT fieldname, value FROM moz_formhistory", ["fieldname", "value"]); - do_check_eq(r4.length, 1); - do_check_eq(r4[0].fieldname, "foo"); - do_check_eq(r4[0].value, "bar"); - - _("Get multiple columns with a different order"); - let r5 = querySpinningly("SELECT fieldname, value FROM moz_formhistory", ["value", "fieldname"]); - do_check_eq(r5.length, 1); - do_check_eq(r5[0].fieldname, "foo"); - do_check_eq(r5[0].value, "bar"); - - _("Add multiple entries (sqlite doesn't support multiple VALUES)"); - let r6 = querySpinningly("INSERT INTO moz_formhistory (fieldname, value) SELECT 'foo', 'baz' UNION SELECT 'more', 'values'"); - do_check_eq(r6, null); - - _("Get multiple rows"); - let r7 = querySpinningly("SELECT fieldname, value FROM moz_formhistory WHERE fieldname = 'foo'", ["fieldname", "value"]); - do_check_eq(r7.length, 2); - do_check_eq(r7[0].fieldname, "foo"); - do_check_eq(r7[1].fieldname, "foo"); - - _("Make sure updates work"); - let r8 = querySpinningly("UPDATE moz_formhistory SET value = 'updated' WHERE fieldname = 'more'"); - do_check_eq(r8, null); - - _("Get the updated"); - let r9 = querySpinningly("SELECT value, fieldname FROM moz_formhistory WHERE fieldname = 'more'", ["fieldname", "value"]); - do_check_eq(r9.length, 1); - do_check_eq(r9[0].fieldname, "more"); - do_check_eq(r9[0].value, "updated"); - - _("Grabbing fewer fields than queried is fine"); - let r10 = querySpinningly("SELECT value, fieldname FROM moz_formhistory", ["fieldname"]); - do_check_eq(r10.length, 3); - - _("Generate an execution error"); - let query = "INSERT INTO moz_formhistory (fieldname, value) VALUES ('one', NULL)"; - let stmt = dbConnection.createStatement(query); - let except; - try { - Async.querySpinningly(stmt); - } catch (e) { - except = e; - } - stmt.finalize(); - do_check_true(!!except); - do_check_eq(except.result, SQLITE_CONSTRAINT_VIOLATION); - - _("Cleaning up"); - querySpinningly("DELETE FROM moz_formhistory"); - - _("Make sure the timeout got to run before this function ends"); - do_check_true(isAsync); -} diff --git a/services/common/tests/unit/xpcshell.ini b/services/common/tests/unit/xpcshell.ini index 8508b861944d..72eded8a78d4 100644 --- a/services/common/tests/unit/xpcshell.ini +++ b/services/common/tests/unit/xpcshell.ini @@ -42,7 +42,6 @@ tags = blocklist [test_utils_uuid.js] [test_async_chain.js] -[test_async_querySpinningly.js] [test_hawkclient.js] skip-if = os == "android" diff --git a/services/sync/modules/telemetry.js b/services/sync/modules/telemetry.js index 22919a63ab51..790cc7e74c45 100644 --- a/services/sync/modules/telemetry.js +++ b/services/sync/modules/telemetry.js @@ -699,10 +699,6 @@ class SyncTelemetryImpl { return { name: "autherror", from: error.source }; } - if (error instanceof Ci.mozIStorageError) { - return { name: "sqlerror", code: error.result }; - } - let httpCode = error.status || (error.response && error.response.status) || error.code; diff --git a/services/sync/tests/unit/test_history_store.js b/services/sync/tests/unit/test_history_store.js index 0ed71be2923e..1ecf557eb24f 100644 --- a/services/sync/tests/unit/test_history_store.js +++ b/services/sync/tests/unit/test_history_store.js @@ -176,25 +176,20 @@ add_task(async function test_null_title() { add_task(async function test_invalid_records() { _("Make sure we handle invalid URLs in places databases gracefully."); - let connection = PlacesUtils.history - .QueryInterface(Ci.nsPIPlacesDatabase) - .DBConnection; - let stmt = connection.createAsyncStatement( - "INSERT INTO moz_places " - + "(url, url_hash, title, rev_host, visit_count, last_visit_date) " - + "VALUES ('invalid-uri', hash('invalid-uri'), 'Invalid URI', '.', 1, " + TIMESTAMP3 + ")" - ); - Async.querySpinningly(stmt); - stmt.finalize(); - // Add the corresponding visit to retain database coherence. - stmt = connection.createAsyncStatement( - "INSERT INTO moz_historyvisits " - + "(place_id, visit_date, visit_type, session) " - + "VALUES ((SELECT id FROM moz_places WHERE url_hash = hash('invalid-uri') AND url = 'invalid-uri'), " - + TIMESTAMP3 + ", " + Ci.nsINavHistoryService.TRANSITION_TYPED + ", 1)" - ); - Async.querySpinningly(stmt); - stmt.finalize(); + await PlacesUtils.withConnectionWrapper("test_invalid_record", async function(db) { + await db.execute( + "INSERT INTO moz_places " + + "(url, url_hash, title, rev_host, visit_count, last_visit_date) " + + "VALUES ('invalid-uri', hash('invalid-uri'), 'Invalid URI', '.', 1, " + TIMESTAMP3 + ")" + ); + // Add the corresponding visit to retain database coherence. + await db.execute( + "INSERT INTO moz_historyvisits " + + "(place_id, visit_date, visit_type, session) " + + "VALUES ((SELECT id FROM moz_places WHERE url_hash = hash('invalid-uri') AND url = 'invalid-uri'), " + + TIMESTAMP3 + ", " + Ci.nsINavHistoryService.TRANSITION_TYPED + ", 1)" + ); + }); do_check_attribute_count((await store.getAllIDs()), 4); _("Make sure we report records with invalid URIs."); diff --git a/services/sync/tests/unit/test_places_guid_downgrade.js b/services/sync/tests/unit/test_places_guid_downgrade.js index effb30154c5f..14f2db6bce55 100644 --- a/services/sync/tests/unit/test_places_guid_downgrade.js +++ b/services/sync/tests/unit/test_places_guid_downgrade.js @@ -2,7 +2,6 @@ http://creativecommons.org/publicdomain/zero/1.0/ */ Cu.import("resource://services-common/utils.js"); -Cu.import("resource://services-common/async.js"); Cu.import("resource://services-sync/util.js"); Cu.import("resource://services-sync/engines.js"); Cu.import("resource://services-sync/engines/history.js"); @@ -118,33 +117,31 @@ add_task(async function test_history_guids() { dump("tbguid: " + tbguid + "\n"); _("History: Verify GUIDs are added to the guid column."); - let connection = PlacesUtils.history - .QueryInterface(Ci.nsPIPlacesDatabase) - .DBConnection; - let stmt = connection.createAsyncStatement( - "SELECT id FROM moz_places WHERE guid = :guid"); - - stmt.params.guid = fxguid; - let result = Async.querySpinningly(stmt, ["id"]); + let db = await PlacesUtils.promiseDBConnection(); + let result = await db.execute( + "SELECT id FROM moz_places WHERE guid = :guid", + {guid: fxguid} + ); do_check_eq(result.length, 1); - stmt.params.guid = tbguid; - result = Async.querySpinningly(stmt, ["id"]); + result = await db.execute( + "SELECT id FROM moz_places WHERE guid = :guid", + {guid: tbguid} + ); do_check_eq(result.length, 1); - stmt.finalize(); _("History: Verify GUIDs weren't added to annotations."); - stmt = connection.createAsyncStatement( - "SELECT a.content AS guid FROM moz_annos a WHERE guid = :guid"); - - stmt.params.guid = fxguid; - result = Async.querySpinningly(stmt, ["guid"]); + result = await db.execute( + "SELECT a.content AS guid FROM moz_annos a WHERE guid = :guid", + {guid: fxguid} + ); do_check_eq(result.length, 0); - stmt.params.guid = tbguid; - result = Async.querySpinningly(stmt, ["guid"]); + result = await db.execute( + "SELECT a.content AS guid FROM moz_annos a WHERE guid = :guid", + {guid: tbguid} + ); do_check_eq(result.length, 0); - stmt.finalize(); } await new Promise((resolve, reject) => { @@ -177,35 +174,33 @@ add_task(async function test_bookmark_guids() { let tbguid = await store.GUIDForId(tbid); _("Bookmarks: Verify GUIDs are added to the guid column."); - let connection = PlacesUtils.history - .QueryInterface(Ci.nsPIPlacesDatabase) - .DBConnection; - let stmt = connection.createAsyncStatement( - "SELECT id FROM moz_bookmarks WHERE guid = :guid"); - - stmt.params.guid = fxguid; - let result = Async.querySpinningly(stmt, ["id"]); + let db = await PlacesUtils.promiseDBConnection(); + let result = await db.execute( + "SELECT id FROM moz_bookmarks WHERE guid = :guid", + {guid: fxguid} + ); do_check_eq(result.length, 1); - do_check_eq(result[0].id, fxid); + do_check_eq(result[0].getResultByName("id"), fxid); - stmt.params.guid = tbguid; - result = Async.querySpinningly(stmt, ["id"]); + result = await db.execute( + "SELECT id FROM moz_bookmarks WHERE guid = :guid", + {guid: tbguid} + ); do_check_eq(result.length, 1); - do_check_eq(result[0].id, tbid); - stmt.finalize(); + do_check_eq(result[0].getResultByName("id"), tbid); _("Bookmarks: Verify GUIDs weren't added to annotations."); - stmt = connection.createAsyncStatement( - "SELECT a.content AS guid FROM moz_items_annos a WHERE guid = :guid"); - - stmt.params.guid = fxguid; - result = Async.querySpinningly(stmt, ["guid"]); + result = await db.execute( + "SELECT a.content AS guid FROM moz_items_annos a WHERE guid = :guid", + {guid: fxguid} + ); do_check_eq(result.length, 0); - stmt.params.guid = tbguid; - result = Async.querySpinningly(stmt, ["guid"]); + result = await db.execute( + "SELECT a.content AS guid FROM moz_items_annos a WHERE guid = :guid", + {guid: tbguid} + ); do_check_eq(result.length, 0); - stmt.finalize(); }); function run_test() { diff --git a/services/sync/tests/unit/test_telemetry.js b/services/sync/tests/unit/test_telemetry.js index 595d465bff8e..ab9b18b00ba2 100644 --- a/services/sync/tests/unit/test_telemetry.js +++ b/services/sync/tests/unit/test_telemetry.js @@ -588,31 +588,6 @@ add_task(async function test_no_foreign_engines_in_error_ping() { } }); -add_task(async function test_sql_error() { - enableValidationPrefs(); - - await Service.engineManager.register(SteamEngine); - let engine = Service.engineManager.get("steam"); - engine.enabled = true; - let server = await serverForFoo(engine); - await SyncTestingInfrastructure(server); - engine._sync = function() { - // Just grab a DB connection and issue a bogus SQL statement synchronously. - let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection; - Async.querySpinningly(db.createAsyncStatement("select bar from foo")); - }; - try { - _(`test_sql_error: Steam tracker contents: ${ - JSON.stringify(engine._tracker.changedIDs)}`); - let ping = await sync_and_validate_telem(true); - let enginePing = ping.engines.find(e => e.name === "steam"); - deepEqual(enginePing.failureReason, { name: "sqlerror", code: 1 }); - } finally { - await cleanAndGo(engine, server); - Service.engineManager.unregister(engine); - } -}); - add_task(async function test_no_foreign_engines_in_success_ping() { enableValidationPrefs(); diff --git a/services/sync/tps/extensions/tps/resource/modules/history.jsm b/services/sync/tps/extensions/tps/resource/modules/history.jsm index 157d87c4ab8d..1789cf617326 100644 --- a/services/sync/tps/extensions/tps/resource/modules/history.jsm +++ b/services/sync/tps/extensions/tps/resource/modules/history.jsm @@ -13,10 +13,11 @@ const {classes: Cc, interfaces: Ci, utils: Cu} = Components; Cu.import("resource://gre/modules/Services.jsm"); Cu.import("resource://gre/modules/PlacesUtils.jsm"); +Cu.import("resource://gre/modules/PlacesSyncUtils.jsm"); Cu.import("resource://tps/logger.jsm"); Cu.import("resource://services-common/async.js"); -var DumpHistory = function TPS_History__DumpHistory() { +var DumpHistory = async function TPS_History__DumpHistory() { let query = PlacesUtils.history.getNewQuery(); let options = PlacesUtils.history.getNewQueryOptions(); let root = PlacesUtils.history.executeQuery(query, options).root; @@ -25,7 +26,7 @@ var DumpHistory = function TPS_History__DumpHistory() { for (var i = 0; i < root.childCount; i++) { let node = root.getChild(i); let uri = node.uri; - let curvisits = HistoryEntry._getVisits(uri); + let curvisits = await PlacesSyncUtils.history.fetchVisitsForURL(uri); for (var visit of curvisits) { Logger.logInfo("URI: " + uri + ", type=" + visit.type + ", date=" + visit.date, true); } @@ -40,50 +41,6 @@ var DumpHistory = function TPS_History__DumpHistory() { * Contains methods for manipulating browser history entries. */ var HistoryEntry = { - /** - * _db - * - * Returns the DBConnection object for the history service. - */ - get _db() { - return PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase).DBConnection; - }, - - /** - * _visitStm - * - * Return the SQL statement for getting history visit information - * from the moz_historyvisits table. Borrowed from Weave's - * history.js. - */ - get _visitStm() { - let stm = this._db.createStatement( - "SELECT visit_type type, visit_date date " + - "FROM moz_historyvisits " + - "WHERE place_id = (" + - "SELECT id " + - "FROM moz_places " + - "WHERE url_hash = hash(:url) AND url = :url) " + - "ORDER BY date DESC LIMIT 20"); - this.__defineGetter__("_visitStm", () => stm); - return stm; - }, - - /** - * _getVisits - * - * Gets history information about visits to a given uri. - * - * @param uri The uri to get visits for - * @return an array of objects with 'date' and 'type' properties, - * corresponding to the visits in the history database for the - * given uri - */ - _getVisits: function HistStore__getVisits(uri) { - this._visitStm.params.url = uri; - return Async.querySpinningly(this._visitStm, ["date", "type"]); - }, - /** * Add * @@ -138,11 +95,11 @@ var HistoryEntry = { * the time the current Crossweave run was started * @return true if all the visits for the uri are found, otherwise false */ - Find(item, usSinceEpoch) { + async Find(item, usSinceEpoch) { Logger.AssertTrue("visits" in item && "uri" in item, "History entry in test file must have both 'visits' " + "and 'uri' properties"); - let curvisits = this._getVisits(item.uri); + let curvisits = await PlacesSyncUtils.history.fetchVisitsForURL(item.uri); for (let visit of curvisits) { for (let itemvisit of item.visits) { let expectedDate = itemvisit.date * 60 * 60 * 1000 * 1000 diff --git a/services/sync/tps/extensions/tps/resource/tps.jsm b/services/sync/tps/extensions/tps/resource/tps.jsm index 36af8242da40..4ba842341e15 100644 --- a/services/sync/tps/extensions/tps/resource/tps.jsm +++ b/services/sync/tps/extensions/tps/resource/tps.jsm @@ -414,11 +414,11 @@ var TPS = { HistoryEntry.Delete(entry, this._usSinceEpoch); break; case ACTION_VERIFY: - Logger.AssertTrue(HistoryEntry.Find(entry, this._usSinceEpoch), + Logger.AssertTrue((await HistoryEntry.Find(entry, this._usSinceEpoch)), "Uri visits not found in history database"); break; case ACTION_VERIFY_NOT: - Logger.AssertTrue(!HistoryEntry.Find(entry, this._usSinceEpoch), + Logger.AssertTrue(!(await HistoryEntry.Find(entry, this._usSinceEpoch)), "Uri visits found in history database, but they shouldn't be"); break; default: @@ -428,7 +428,7 @@ var TPS = { Logger.logPass("executing action " + action.toUpperCase() + " on history"); } catch (e) { - DumpHistory(); + await DumpHistory(); throw (e); } },