Bug 1375223 - Remove Async.querySpinningly. r=kitcambridge

MozReview-Commit-ID: bMo1jyIY5g

--HG--
extra : rebase_source : ac69fde2cb8216300bdb9e7d19528c15cdceb7c8

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 => {

services/common/tests/unit/test_async_querySpinningly.js

@@ -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);
-}

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"

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;

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.");

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() {

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();
 

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

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);
     }
   },