Bug 1375223 - Remove Async.querySpinningly. r?kitcambridge draft
authorLuciano I <lyret@protonmail.com>
Sat, 19 Aug 2017 01:54:33 -0300
changeset 649346 9411bb8bcab88cda8b693435a22b81a5cdbd6795
parent 649312 4f4487cc2d30d988742109868dcf21c4113f12f5
child 727068 1ffd0c85026b18735dc24ae110143ca31555079d
push id75020
push userbmo:lyret@protonmail.com
push dateSat, 19 Aug 2017 04:56:02 +0000
reviewerskitcambridge
bugs1375223
milestone57.0a1
Bug 1375223 - Remove Async.querySpinningly. r?kitcambridge MozReview-Commit-ID: GaWy0Tnuer1
services/common/tests/unit/test_async_querySpinningly.js
services/common/tests/unit/xpcshell.ini
services/sync/tests/unit/test_history_store.js
services/sync/tests/unit/test_places_guid_downgrade.js
services/sync/tests/unit/test_telemetry.js
services/sync/tps/extensions/tps/resource/modules/history.jsm
deleted file mode 100644
--- 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);
-}
--- a/services/common/tests/unit/xpcshell.ini
+++ b/services/common/tests/unit/xpcshell.ini
@@ -38,17 +38,16 @@ tags = blocklist
 [test_utils_json.js]
 [test_utils_makeURI.js]
 [test_utils_namedTimer.js]
 [test_utils_sets.js]
 [test_utils_utf8.js]
 [test_utils_uuid.js]
 
 [test_async_chain.js]
-[test_async_querySpinningly.js]
 
 [test_hawkclient.js]
 skip-if = os == "android"
 [test_hawkrequest.js]
 skip-if = os == "android"
 
 [test_logmanager.js]
 [test_observers.js]
--- a/services/sync/tests/unit/test_history_store.js
+++ b/services/sync/tests/unit/test_history_store.js
@@ -163,35 +163,32 @@ add_task(async function test_null_title(
   do_check_attribute_count((await store.getAllIDs()), 3);
   let queryres = queryHistoryVisits(resuri);
   do_check_eq(queryres.length, 1);
   do_check_eq(queryres[0].time, TIMESTAMP3);
 });
 
 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 + ")"
+  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 + ")"
+      );
+      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)"
+      );
+    }
   );
-  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();
+
   do_check_attribute_count((await store.getAllIDs()), 4);
 
   _("Make sure we report records with invalid URIs.");
   let invalid_uri_guid = Utils.makeGUID();
   let failed = await store.applyIncomingBatch([{
     id: invalid_uri_guid,
     histUri: ":::::::::::::::",
     title: "Doesn't have a valid URI",
--- a/services/sync/tests/unit/test_places_guid_downgrade.js
+++ b/services/sync/tests/unit/test_places_guid_downgrade.js
@@ -112,43 +112,41 @@ add_task(async function test_history_gui
 
   async function onVisitAdded() {
     let fxguid = await store.GUIDForUri(fxuri, true);
     let tbguid = await store.GUIDForUri(tburi, true);
     dump("fxguid: " + fxguid + "\n");
     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");
+    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 = fxguid;
-    let 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.params.guid = tbguid;
-    result = Async.querySpinningly(stmt, ["id"]);
-    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) => {
     PlacesUtils.asyncHistory.updatePlaces(places, {
       handleError: function handleError() {
         do_throw("Unexpected error in adding visit.");
       },
       handleResult: function handleResult() {},
@@ -171,44 +169,42 @@ add_task(async function test_bookmark_gu
     tburi,
     PlacesUtils.bookmarks.DEFAULT_INDEX,
     "Get Thunderbird!");
 
   let fxguid = await store.GUIDForId(fxid);
   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");
+  let db = await PlacesUtils.promiseDBConnection();
+  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].getResultByName("id"), fxid);
 
-  stmt.params.guid = fxguid;
-  let 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, fxid);
-
-  stmt.params.guid = tbguid;
-  result = Async.querySpinningly(stmt, ["id"]);
-  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() {
   setPlacesDatabase("places_v10_from_v11.sqlite");
 
   run_next_test();
 }
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -553,41 +553,16 @@ add_task(async function test_no_foreign_
     equal(ping.status.service, SYNC_FAILED_PARTIAL);
     ok(ping.engines.every(e => e.name !== "bogus"));
   } finally {
     await cleanAndGo(engine, server);
     Service.engineManager.unregister(engine);
   }
 });
 
-add_task(async function test_sql_error() {
-  enableValidationPrefs();
-
-  await Service.engineManager.register(SteamEngine);
-  let engine = Service.engineManager.get("steam");
-  engine.enabled = true;
-  let server = 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();
 
   await Service.engineManager.register(BogusEngine);
   let engine = Service.engineManager.get("bogus");
   engine.enabled = true;
   let server = serverForFoo(engine);
 
--- a/services/sync/tps/extensions/tps/resource/modules/history.jsm
+++ b/services/sync/tps/extensions/tps/resource/modules/history.jsm
@@ -8,88 +8,45 @@
   */
 
 var EXPORTED_SYMBOLS = ["HistoryEntry", "DumpHistory"];
 
 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() {
   let query = PlacesUtils.history.getNewQuery();
   let options = PlacesUtils.history.getNewQueryOptions();
   let root = PlacesUtils.history.executeQuery(query, options).root;
   root.containerOpen = true;
   Logger.logInfo("\n\ndumping history\n", true);
   for (var i = 0; i < root.childCount; i++) {
     let node = root.getChild(i);
     let uri = node.uri;
-    let curvisits = HistoryEntry._getVisits(uri);
+    let curvisits = PlacesSyncUtils.history.fetchVisitsForURL(uri);
     for (var visit of curvisits) {
       Logger.logInfo("URI: " + uri + ", type=" + visit.type + ", date=" + visit.date, true);
     }
   }
   root.containerOpen = false;
   Logger.logInfo("\nend history dump\n", true);
 };
 
 /**
  * HistoryEntry object
  *
  * 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
    *
    * Adds visits for a uri to the history database.  Throws on error.
    *
    * @param item An object representing one or more visits to a specific uri
    * @param usSinceEpoch The number of microseconds from Epoch to
    *        the time the current Crossweave run was started
    * @return nothing
@@ -137,17 +94,17 @@ var HistoryEntry = {
    * @param usSinceEpoch The number of microseconds from Epoch to
    *        the time the current Crossweave run was started
    * @return true if all the visits for the uri are found, otherwise false
    */
   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 = PlacesSyncUtils.history.fetchVisitsForURL(uri);
     for (let visit of curvisits) {
       for (let itemvisit of item.visits) {
         let expectedDate = itemvisit.date * 60 * 60 * 1000 * 1000
             + usSinceEpoch;
         if (visit.type == itemvisit.type && visit.date == expectedDate) {
           itemvisit.found = true;
         }
       }