Bug 1243778 - PushRecord::getLastVisit cannot rely on the Places url index anymore. r=kitcambridge draft
authorMarco Bonardo <mbonardo@mozilla.com>
Mon, 08 Feb 2016 14:42:07 +0100
changeset 329784 ba09532f414d4fb2d6e0a14ff150974f4116b6c0
parent 329582 5728bfd4c648ce684b2ebfdcfd9e399a98d202fa
child 514037 8621ffb473a0b2cdce5ae32dc213bf4b5be0023e
push id10610
push usermak77@bonardo.net
push dateTue, 09 Feb 2016 14:17:44 +0000
reviewerskitcambridge
bugs1243778
milestone47.0a1
Bug 1243778 - PushRecord::getLastVisit cannot rely on the Places url index anymore. r=kitcambridge
dom/push/PushRecord.jsm
dom/push/test/xpcshell/head.js
dom/push/test/xpcshell/test_drop_expired.js
dom/push/test/xpcshell/test_quota_exceeded.js
dom/push/test/xpcshell/test_quota_observer.js
dom/push/test/xpcshell/test_quota_with_notification.js
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/PlacesTestUtils.jsm
--- a/dom/push/PushRecord.jsm
+++ b/dom/push/PushRecord.jsm
@@ -14,17 +14,18 @@ Cu.import("resource://gre/modules/Prefer
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Messaging",
                                   "resource://gre/modules/Messaging.jsm");
 
 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");
 
 
 this.EXPORTED_SYMBOLS = ["PushRecord"];
 
 const prefs = new Preferences("dom.push.");
 
@@ -104,77 +105,72 @@ PushRecord.prototype = {
   /**
    * Queries the Places database for the last time a user visited the site
    * associated with a push registration.
    *
    * @returns {Promise} A promise resolved with either the last time the user
    *  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
     // `pushsubscriptionchange` event when the user visits a site with expired push
     // registrations. Visits only count if the user sees the origin in the address
     // bar. This excludes embedded resources, downloads, and framed links.
     const QUOTA_REFRESH_TRANSITIONS_SQL = [
       Ci.nsINavHistoryService.TRANSITION_LINK,
       Ci.nsINavHistoryService.TRANSITION_TYPED,
       Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
       Ci.nsINavHistoryService.TRANSITION_REDIRECT_PERMANENT,
       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");
     while (windows.hasMoreElements()) {
       let window = windows.getNext();
       if (window.closed || PrivateBrowsingUtils.isWindowPrivate(window)) {
         continue;
       }
--- a/dom/push/test/xpcshell/head.js
+++ b/dom/push/test/xpcshell/head.js
@@ -8,16 +8,18 @@ var {classes: Cc, interfaces: Ci, utils:
 Cu.import('resource://gre/modules/XPCOMUtils.jsm');
 Cu.import('resource://gre/modules/Services.jsm');
 Cu.import('resource://gre/modules/Task.jsm');
 Cu.import('resource://gre/modules/Timer.jsm');
 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.');
 
 const DEFAULT_TIMEOUT = 5000;
 
 const WEBSOCKET_CLOSE_GOING_AWAY = 1001;
 
@@ -57,35 +59,16 @@ function after(times, func) {
   return function afterFunc() {
     if (--times <= 0) {
       return func.apply(this, arguments);
     }
   };
 }
 
 /**
- * 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.
  *
  * @param {Function[]} callbacks The callbacks to execute. One callback will be
  *  executed per tick.
  */
 function waterfall(...callbacks) {
   callbacks.reduce((promise, callback) => promise.then(() => {
--- a/dom/push/test/xpcshell/test_drop_expired.js
+++ b/dom/push/test/xpcshell/test_drop_expired.js
@@ -7,23 +7,21 @@ const {PushDB, PushService, PushServiceW
 
 const userAgentID = '2c43af06-ab6e-476a-adc4-16cbda54fb89';
 
 var db;
 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
   });
 }
 
 var putRecord = Task.async(function* ({scope, perm, quota, lastPush, lastVisit}) {
   let uri = Services.io.newURI(scope, null, null);
 
   Services.perms.add(uri, 'desktop-notification',
     Ci.nsIPermissionManager[perm]);
--- a/dom/push/test/xpcshell/test_quota_exceeded.js
+++ b/dom/push/test/xpcshell/test_quota_exceeded.js
@@ -40,47 +40,44 @@ add_task(function* test_expiration_origi
     lastPush: 0,
     version: null,
     originAttributes: '',
     quota: 16,
   });
 
   // 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
   // different scopes, so each can send 5 notifications before we remove
   // their subscription.
   let updates = 0;
   let notifyPromise = promiseObserverNotification('push-message', (subject, data) => {
     updates++;
--- a/dom/push/test/xpcshell/test_quota_observer.js
+++ b/dom/push/test/xpcshell/test_quota_observer.js
@@ -52,23 +52,21 @@ add_task(function* test_expiration_histo
     scope: 'https://example.com/stuff',
     pushCount: 0,
     lastPush: 0,
     version: null,
     originAttributes: '',
     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;
   let unregisterPromise = new Promise(resolve => unregisterDone = resolve);
   let subChangePromise = promiseObserverNotification('push-subscription-change', (subject, data) =>
     data == 'https://example.com/stuff');
 
   PushService.init({
@@ -122,23 +120,21 @@ add_task(function* test_expiration_histo
     pushCount: 0,
     lastPush: 0,
     version: null,
     originAttributes: '',
     quota: 0,
   });
 
   // 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', '');
 
   // And we should receive notifications for both scopes.
   yield waitForPromise(subChangePromise, DEFAULT_TIMEOUT,
     'Timed out waiting for subscription change events');
   deepEqual(notifiedScopes.sort(), [
     'https://example.com/auctions',
--- a/dom/push/test/xpcshell/test_quota_with_notification.js
+++ b/dom/push/test/xpcshell/test_quota_with_notification.js
@@ -38,23 +38,21 @@ add_task(function* test_expiration_origi
     pushCount: 0,
     lastPush: 0,
     version: null,
     originAttributes: '',
     quota: 16,
   });
 
   // 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;
 
   let updates = 0;
   let notifyPromise = promiseObserverNotification('push-message', (subject, data) => {
     updates++;
     return updates == numMessages;
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -1380,29 +1380,38 @@ this.PlacesUtils = {
       aStream.write(json, json.length);
     }
     else {
       throw Cr.NS_ERROR_UNEXPECTED;
     }
   },
 
   /**
-   * 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.
    *
    * As all operations on the Places database are asynchronous, if shutdown
    * is initiated while an operation is pending, this could cause dataloss.
    * Using `withConnectionWrapper` ensures that shutdown waits until all
    * operations are complete before proceeding.
    *
--- a/toolkit/components/places/tests/PlacesTestUtils.jsm
+++ b/toolkit/components/places/tests/PlacesTestUtils.jsm
@@ -1,23 +1,26 @@
 "use strict";
 
 this.EXPORTED_SYMBOLS = [
   "PlacesTestUtils",
 ];
 
 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({
   /**
    * Asynchronously adds visits to a page.
    *
    * @param aPlaceInfo
    *        Can be an nsIURI, in such a case a single LINK visit will be added.
    *        Otherwise can be an object describing the visit to add, or an array
@@ -28,58 +31,66 @@ this.PlacesTestUtils = Object.freeze({
    *            [optional] visitDate: visit date in microseconds from the epoch
    *            [optional] referrer: nsIURI of the referrer for this visit
    *          }
    *
    * @return {Promise}
    * @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);
   }),
 
   /**
    * Clear all history.
    *
    * @return {Promise}
    * @resolves When history was cleared successfully.
    * @rejects JavaScript exception.