Bug 1243778 - PushRecord::getLastVisit cannot rely on the Places url index anymore. r=kitcambridge

dom/push/PushRecord.jsm

@@ -19,7 +19,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "Messaging",
 
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
                                   "resource://gre/modules/PlacesUtils.jsm");
-
+XPCOMUtils.defineLazyModuleGetter(this, "Task",
+                                  "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
                                   "resource://gre/modules/PrivateBrowsingUtils.jsm");
 
@@ -109,23 +110,19 @@ PushRecord.prototype = {
    *  visited the site, or `-Infinity` if the site is not in the user's history.
    *  The time is expressed in milliseconds since Epoch.
    */
-  getLastVisit() {
+  getLastVisit: Task.async(function* () {
     if (!this.quotaApplies() || this.isTabOpen()) {
       // If the registration isn't subject to quota, or the user already
       // has the site open, skip expensive database queries.
-      return Promise.resolve(Date.now());
+      return Date.now();
     }
 
     if (AppConstants.MOZ_ANDROID_HISTORY) {
-      return Messaging.sendRequestForResult({
+      let result = yield Messaging.sendRequestForResult({
         type: "History:GetPrePathLastVisitedTimeMilliseconds",
         prePath: this.uri.prePath,
-      }).then(result => {
-        if (result == 0) {
-          return -Infinity;
-        }
-        return result;
       });
+      return result == 0 ? -Infinity : result;
     }
 
     // Places History transition types that can fire a
@@ -140,36 +137,35 @@ PushRecord.prototype = {
       Ci.nsINavHistoryService.TRANSITION_REDIRECT_TEMPORARY
     ].join(",");
 
-    return PlacesUtils.withConnectionWrapper("PushRecord.getLastVisit", db => {
-      // We're using a custom query instead of `nsINavHistoryQueryOptions`
-      // because the latter doesn't expose a way to filter by transition type:
-      // `setTransitions` performs a logical "and," but we want an "or." We
-      // also avoid an unneeded left join on `moz_favicons`, and an `ORDER BY`
-      // clause that emits a suboptimal index warning.
-      return db.executeCached(
-        `SELECT MAX(p.last_visit_date)
-         FROM moz_places p
-         INNER JOIN moz_historyvisits h ON p.id = h.place_id
-         WHERE (
-           p.url >= :urlLowerBound AND p.url <= :urlUpperBound AND
-           h.visit_type IN (${QUOTA_REFRESH_TRANSITIONS_SQL})
-         )
-        `,
-        {
-          // Restrict the query to all pages for this origin.
-          urlLowerBound: this.uri.prePath,
-          urlUpperBound: this.uri.prePath + "\x7f",
-        }
-      );
-    }).then(rows => {
-      if (!rows.length) {
-        return -Infinity;
+    let db =  yield PlacesUtils.promiseDBConnection();
+    // We're using a custom query instead of `nsINavHistoryQueryOptions`
+    // because the latter doesn't expose a way to filter by transition type:
+    // `setTransitions` performs a logical "and," but we want an "or." We
+    // also avoid an unneeded left join on `moz_favicons`, and an `ORDER BY`
+    // clause that emits a suboptimal index warning.
+    let rows = yield db.executeCached(
+      `SELECT MAX(visit_date) AS lastVisit
+       FROM moz_places p
+       JOIN moz_historyvisits ON p.id = place_id
+       WHERE rev_host = get_unreversed_host(:host || '.') || '.'
+         AND url BETWEEN :prePath AND :prePath || X'FFFF'
+         AND visit_type IN (${QUOTA_REFRESH_TRANSITIONS_SQL})
+      `,
+      {
+        // Restrict the query to all pages for this origin.
+        host: this.uri.host,
+        prePath: this.uri.prePath,
       }
-      // Places records times in microseconds.
-      let lastVisit = rows[0].getResultByIndex(0);
-      return lastVisit / 1000;
-    });
-  },
+    );
+
+    if (!rows.length) {
+      return -Infinity;
+    }
+    // Places records times in microseconds.
+    let lastVisit = rows[0].getResultByName("lastVisit");
+
+    return lastVisit / 1000;
+  }),
 
   isTabOpen() {
     let windows = Services.wm.getEnumerator("navigator:browser");

dom/push/test/xpcshell/head.js

@@ -13,6 +13,8 @@ Cu.import('resource://gre/modules/Promise.jsm');
 Cu.import('resource://gre/modules/Preferences.jsm');
 Cu.import('resource://gre/modules/PlacesUtils.jsm');
 Cu.import('resource://gre/modules/ObjectUtils.jsm');
+XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
+                                  "resource://testing-common/PlacesTestUtils.jsm");
 
 const serviceExports = Cu.import('resource://gre/modules/PushService.jsm', {});
 const servicePrefs = new Preferences('dom.push.');
@@ -61,25 +63,6 @@ function after(times, func) {
   };
 }
 
-/**
- * Updates the places database.
- *
- * @param {mozIPlaceInfo} place A place record to insert.
- * @returns {Promise} A promise that fulfills when the database is updated.
- */
-function addVisit(place) {
-  return new Promise((resolve, reject) => {
-    if (typeof place.uri == 'string') {
-      place.uri = Services.io.newURI(place.uri, null, null);
-    }
-    PlacesUtils.asyncHistory.updatePlaces(place, {
-      handleCompletion: resolve,
-      handleError: reject,
-      handleResult() {},
-    });
-  });
-}
-
 /**
  * Defers one or more callbacks until the next turn of the event loop. Multiple
  * callbacks are executed in order.

dom/push/test/xpcshell/test_drop_expired.js

@@ -12,13 +12,11 @@ var quotaURI;
 var permURI;
 
 function visitURI(uri, timestamp) {
-  return addVisit({
+  return PlacesTestUtils.addVisits({
     uri: uri,
     title: uri.spec,
-    visits: [{
-      visitDate: timestamp * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
+    visitDate: timestamp * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
 }
 

dom/push/test/xpcshell/test_quota_exceeded.js

@@ -45,37 +45,34 @@ add_task(function* test_expiration_origin_threshold() {
 
   // The notification threshold is per-origin, even with multiple service
   // workers for different scopes.
-  yield addVisit({
-    uri: 'https://example.com/login',
-    title: 'Sign in to see your auctions',
-    visits: [{
+  yield PlacesTestUtils.addVisits([
+    {
+      uri: 'https://example.com/login',
+      title: 'Sign in to see your auctions',
       visitDate: (Date.now() - 7 * 24 * 60 * 60 * 1000) * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
-  });
-
-  // We'll always use your most recent visit to an origin.
-  yield addVisit({
-    uri: 'https://example.com/auctions',
-    title: 'Your auctions',
-    visits: [{
+      transition: Ci.nsINavHistoryService.TRANSITION_LINK
+    },
+    // We'll always use your most recent visit to an origin.
+    {
+      uri: 'https://example.com/auctions',
+      title: 'Your auctions',
       visitDate: (Date.now() - 2 * 24 * 60 * 60 * 1000) * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
-  });
-
-  // ...But we won't count downloads or embeds.
-  yield addVisit({
-    uri: 'https://example.com/invoices/invoice.pdf',
-    title: 'Invoice #123',
-    visits: [{
+      transition: Ci.nsINavHistoryService.TRANSITION_LINK
+    },
+    // ...But we won't count downloads or embeds.
+    {
+      uri: 'https://example.com/invoices/invoice.pdf',
+      title: 'Invoice #123',
       visitDate: (Date.now() - 1 * 24 * 60 * 60 * 1000) * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_EMBED,
-    }, {
+      transition: Ci.nsINavHistoryService.TRANSITION_EMBED
+    },
+    {
+      uri: 'https://example.com/invoices/invoice.pdf',
+      title: 'Invoice #123',
       visitDate: Date.now() * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
-    }],
-  });
+      transition: Ci.nsINavHistoryService.TRANSITION_DOWNLOAD
+    }
+  ]);
 
   // We expect to receive 6 notifications: 5 on the `auctions` channel,
   // and 1 on the `deals` channel. They're from the same origin, but

dom/push/test/xpcshell/test_quota_observer.js

@@ -57,13 +57,11 @@ add_task(function* test_expiration_history_observer() {
     quota: 0,
   });
 
-  yield addVisit({
+  yield PlacesTestUtils.addVisits({
     uri: 'https://example.com/infrequent',
     title: 'Infrequently-visited page',
-    visits: [{
-      visitDate: (Date.now() - 14 * 24 * 60 * 60 * 1000) * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
+    visitDate: (Date.now() - 14 * 24 * 60 * 60 * 1000) * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
 
   let unregisterDone;
@@ -127,13 +125,11 @@ add_task(function* test_expiration_history_observer() {
   });
 
   // Now visit the site...
-  yield addVisit({
+  yield PlacesTestUtils.addVisits({
     uri: 'https://example.com/another-page',
     title: 'Infrequently-visited page',
-    visits: [{
-      visitDate: Date.now() * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
+    visitDate: Date.now() * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
   Services.obs.notifyObservers(null, 'idle-daily', '');
 

dom/push/test/xpcshell/test_quota_with_notification.js

@@ -43,13 +43,11 @@ add_task(function* test_expiration_origin_threshold() {
   });
 
   // A visit one day ago should provide a quota of 8 messages.
-  yield addVisit({
+  yield PlacesTestUtils.addVisits({
     uri: 'https://example.com/login',
     title: 'Sign in to see your auctions',
-    visits: [{
-      visitDate: (Date.now() - 1 * 24 * 60 * 60 * 1000) * 1000,
-      transitionType: Ci.nsINavHistoryService.TRANSITION_LINK,
-    }],
+    visitDate: (Date.now() - 1 * 24 * 60 * 60 * 1000) * 1000,
+    transition: Ci.nsINavHistoryService.TRANSITION_LINK
   });
 
   let numMessages = 10;

toolkit/components/places/PlacesUtils.jsm

@@ -1385,19 +1385,28 @@ this.PlacesUtils = {
   },
 
   /**
-   * Gets the shared Sqlite.jsm readonly connection to the Places database.
-   * This is intended to be used mostly internally, and by other Places modules.
-   * Outside the Places component, it should be used only as a last resort.
+   * Gets a shared Sqlite.jsm readonly connection to the Places database,
+   * usable only for SELECT queries.
+   *
+   * This is intended to be used mostly internally, components outside of
+   * Places should, when possible, use API calls and file bugs to get proper
+   * APIs, where they are missing.
    * Keep in mind the Places DB schema is by no means frozen or even stable.
    * Your custom queries can - and will - break overtime.
+   *
+   * Example:
+   * let db = yield PlacesUtils.promiseDBConnection();
+   * let rows = yield db.executeCached(sql, params);
    */
   promiseDBConnection: () => gAsyncDBConnPromised,
 
   /**
-   * Perform a read/write operation on the Places database.
+   * Performs a read/write operation on the Places database through a Sqlite.jsm
+   * wrapped connection to the Places database.
    *
-   * Gets a Sqlite.jsm wrapped connection to the Places database.
-   * This is intended to be used mostly internally, and by other Places modules.
+   * This is intended to be used only by Places itself, always use APIs if you
+   * need to modify the Places database. Use promiseDBConnection if you need to
+   * SELECT from the database and there's no covering API.
    * Keep in mind the Places DB schema is by no means frozen or even stable.
    * Your custom queries can - and will - break overtime.
    *

toolkit/components/places/tests/PlacesTestUtils.jsm

@@ -6,13 +6,16 @@ this.EXPORTED_SYMBOLS = [
 
 const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
+Cu.importGlobalProperties(["URL"]);
+
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
-Cu.import("resource://gre/modules/Task.jsm");
-
+XPCOMUtils.defineLazyModuleGetter(this, "Task",
+                                  "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "PlacesUtils",
-  "resource://gre/modules/PlacesUtils.jsm");
-
+                                  "resource://gre/modules/PlacesUtils.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
+                                  "resource://gre/modules/NetUtil.jsm");
 
 this.PlacesTestUtils = Object.freeze({
   /**
@@ -33,48 +36,56 @@ this.PlacesTestUtils = Object.freeze({
    * @resolves When all visits have been added successfully.
    * @rejects JavaScript exception.
    */
-  addVisits: Task.async(function*(placeInfo) {
-    let promise = new Promise((resolve, reject) => {
-      let places = [];
-      if (placeInfo instanceof Ci.nsIURI) {
-        places.push({ uri: placeInfo });
-      }
-      else if (Array.isArray(placeInfo)) {
-        places = places.concat(placeInfo);
-      } else {
-        places.push(placeInfo)
-      }
+  addVisits: Task.async(function* (placeInfo) {
+    let places = [];
+    if (placeInfo instanceof Ci.nsIURI ||
+        placeInfo instanceof URL ||
+        typeof placeInfo == "string") {
+      places.push({ uri: placeInfo });
+    }
+    else if (Array.isArray(placeInfo)) {
+      places = places.concat(placeInfo);
+    } else if (typeof placeInfo == "object" && placeInfo.uri) {
+      places.push(placeInfo)
+    } else {
+      throw new Error("Unsupported type passed to addVisits");
+    }
 
-      // Create mozIVisitInfo for each entry.
-      let now = Date.now();
-      for (let place of places) {
-        if (typeof place.title != "string") {
-          place.title = "test visit for " + place.uri.spec;
-        }
-        place.visits = [{
-          transitionType: place.transition === undefined ? Ci.nsINavHistoryService.TRANSITION_LINK
-                                                             : place.transition,
-          visitDate: place.visitDate || (now++) * 1000,
-          referrerURI: place.referrer
-        }];
+    // Create mozIVisitInfo for each entry.
+    let now = Date.now();
+    for (let place of places) {
+      if (typeof place.title != "string") {
+        place.title = "test visit for " + place.uri.spec;
       }
+      if (typeof place.uri == "string") {
+        place.uri = NetUtil.newURI(place.uri);
+      } else if (place.uri instanceof URL) {
+        place.uri = NetUtil.newURI(place.href);
+      }
+      place.visits = [{
+        transitionType: place.transition === undefined ? Ci.nsINavHistoryService.TRANSITION_LINK
+                                                       : place.transition,
+        visitDate: place.visitDate || (now++) * 1000,
+        referrerURI: place.referrer
+      }];
+    }
 
+    yield new Promise((resolve, reject) => {
       PlacesUtils.asyncHistory.updatePlaces(
         places,
         {
-          handleError: function AAV_handleError(resultCode, placeInfo) {
+          handleError(resultCode, placeInfo) {
             let ex = new Components.Exception("Unexpected error in adding visits.",
                                               resultCode);
             reject(ex);
           },
           handleResult: function () {},
-          handleCompletion: function UP_handleCompletion() {
+          handleCompletion() {
             resolve();
           }
         }
       );
     });
-    return (yield promise);
   }),
 
   /**