Bug 1249263 - add a `removeByFilter` method to filter by host and time,r=mak
authori.milind.luthra+590334 <i.milind.luthra@gmail.com>
Thu, 11 May 2017 17:22:27 +0200
changeset 357847 8840f2afc5f942d73384ddcc7e4456a53408b619
parent 357846 2d5adb9d53a2bc85eb9da1ff27752bf577e94ccc
child 357848 a9769adade1ce9c93a963e0e5e340487a3d62780
push id31804
push userkwierso@gmail.com
push dateFri, 12 May 2017 00:33:03 +0000
treeherdermozilla-central@2a8e0c4be57d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1249263
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1249263 - add a `removeByFilter` method to filter by host and time,r=mak Added a method in History to filter by host and timeframe, which is designed to act as a replacement for `RemovePagesByTimeFrame` and `RemovePagesFromHost` in the old API. The `filter` object accepts both a host argument, as well as a timeframe, and filters as per one or both of them. This also moves certain code (the method `validatePageInfo` and methods it uses) from History to PlacesUtils such that we can use it for testing as well, and modifies the method to take another parameter which decides whether the visits inside the pageInfo need to be validated as well (since the pageInfo returned from History.jsm::`remove` and History.jsm::`removeByFilter` do not pass a visits array in their callback functions. Shifts `ensureDate` and `isValidTransitionType`(now renamed to `isValidTransition`) inside the history object. MozReview-Commit-ID: EQAHmjf7131
toolkit/components/places/History.jsm
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/history/test_removeByFilter.js
toolkit/components/places/tests/history/xpcshell.ini
--- a/toolkit/components/places/History.jsm
+++ b/toolkit/components/places/History.jsm
@@ -89,19 +89,16 @@ Cu.importGlobalProperties(["URL"]);
  */
 const NOTIFICATION_CHUNK_SIZE = 300;
 const ONRESULT_CHUNK_SIZE = 300;
 
 // This constant determines the maximum number of remove pages before we cycle.
 const REMOVE_PAGES_CHUNKLEN = 300;
 
 
-// Timers resolution is not always good, it can have a 16ms precision on Win.
-const TIMERS_RESOLUTION_SKEW_MS = 16;
-
 /**
  * Sends a bookmarks notification through the given observers.
  *
  * @param observers
  *        array of nsINavBookmarkObserver objects.
  * @param notification
  *        the notification name.
  * @param args
@@ -177,17 +174,17 @@ this.History = Object.freeze({
    * @throws (Error)
    *      If an element of `visits` has an invalid `transition`.
    */
   insert(pageInfo) {
     if (typeof pageInfo != "object" || !pageInfo) {
       throw new TypeError("pageInfo must be an object");
     }
 
-    let info = validatePageInfo(pageInfo);
+    let info = PlacesUtils.validatePageInfo(pageInfo);
 
     return PlacesUtils.withConnectionWrapper("History.jsm: insert",
       db => insert(db, info));
   },
 
   /**
    * Adds a number of visits for a number of pages.
    *
@@ -244,17 +241,17 @@ this.History = Object.freeze({
     if (onResult && typeof onResult != "function") {
       throw new TypeError(`onResult: ${onResult} is not a valid function`);
     }
     if (onError && typeof onError != "function") {
       throw new TypeError(`onError: ${onError} is not a valid function`);
     }
 
     for (let pageInfo of pageInfos) {
-      let info = validatePageInfo(pageInfo);
+      let info = PlacesUtils.validatePageInfo(pageInfo);
       infos.push(info);
     }
 
     return PlacesUtils.withConnectionWrapper("History.jsm: insertMany",
       db => insertMany(db, infos, onResult, onError));
   },
 
   /**
@@ -291,17 +288,17 @@ this.History = Object.freeze({
       pages = [pages];
     }
 
     let guids = [];
     let urls = [];
     for (let page of pages) {
       // Normalize to URL or GUID, or throw if `page` cannot
       // be normalized.
-      let normalized = normalizeToURLOrGUID(page);
+      let normalized = PlacesUtils.normalizeToURLOrGUID(page);
       if (typeof normalized === "string") {
         guids.push(normalized);
       } else {
         urls.push(normalized.href);
       }
     }
 
     // At this stage, we know that either `guids` is not-empty
@@ -376,20 +373,20 @@ this.History = Object.freeze({
       throw new TypeError("Expected a filter");
     }
 
     let hasBeginDate = "beginDate" in filter;
     let hasEndDate = "endDate" in filter;
     let hasURL = "url" in filter;
     let hasLimit = "limit" in filter;
     if (hasBeginDate) {
-      ensureDate(filter.beginDate);
+      this.ensureDate(filter.beginDate);
     }
     if (hasEndDate) {
-      ensureDate(filter.endDate);
+      this.ensureDate(filter.endDate);
     }
     if (hasBeginDate && hasEndDate && filter.beginDate > filter.endDate) {
       throw new TypeError("`beginDate` should be at least as old as `endDate`");
     }
     if (!hasBeginDate && !hasEndDate && !hasURL && !hasLimit) {
       throw new TypeError("Expected a non-empty filter");
     }
 
@@ -410,16 +407,98 @@ this.History = Object.freeze({
     }
 
     return PlacesUtils.withConnectionWrapper("History.jsm: removeVisitsByFilter",
       db => removeVisitsByFilter(db, filter, onResult)
     );
   },
 
   /**
+   * Remove pages from the database based on a filter.
+   *
+   * Any change may be observed through nsINavHistoryObserver
+   *
+   *
+   * @param filter: An object containing a non empty subset of the following
+   * properties:
+   * - host: (string)
+   *     Hostname with subhost wildcard (at most one *), or empty for local files.
+   *     The * can be used only if it is the first character in the url, and not the host.
+   *     For example, *.mozilla.org is allowed, *.org, www.*.org or * is not allowed.
+   * - beginDate: (Date)
+   *     The first time the page was visited (inclusive)
+   * - endDate: (Date)
+   *     The last time the page was visited (inclusive)
+   * @param [optional] onResult: (function(PageInfo))
+   *      A callback invoked for each page found.
+   *
+   * @note This removes pages with at least one visit inside the timeframe.
+   *       Any visits outside the timeframe will also be removed with the page.
+   * @return (Promise)
+   *      A promise resolved once the operation is complete.
+   * @resolve (bool)
+   *      `true` if at least one page was removed, `false` otherwise.
+   * @throws (TypeError)
+   *       if `filter` does not have the expected type, in particular
+   *       if the `object` is empty, or its components do not satisfy the
+   *       criteria given above
+   */
+  removeByFilter(filter, onResult) {
+    if (!filter || typeof filter !== "object") {
+      throw new TypeError("Expected a filter object");
+    }
+
+    let hasHost = "host" in filter;
+    if (hasHost) {
+      if (typeof filter.host !== "string") {
+        throw new TypeError("`host` should be a string");
+      }
+      filter.host = filter.host.toLowerCase();
+    }
+
+    let hasBeginDate = "beginDate" in filter;
+    if (hasBeginDate) {
+      this.ensureDate(filter.beginDate);
+    }
+
+    let hasEndDate = "endDate" in filter;
+    if (hasEndDate) {
+      this.ensureDate(filter.endDate);
+    }
+
+    if (hasBeginDate && hasEndDate && filter.beginDate > filter.endDate) {
+      throw new TypeError("`beginDate` should be at least as old as `endDate`");
+    }
+
+    if (!hasBeginDate && !hasEndDate && !hasHost) {
+      throw new TypeError("Expected a non-empty filter");
+    }
+
+    // Host should follow one of these formats
+    // The first one matches `localhost` or any other custom set in hostsfile
+    // The second one matches *.mozilla.org or mozilla.com etc
+    // The third one is for local files
+    if (hasHost &&
+        !((/^[a-z0-9-]+$/).test(filter.host)) &&
+        !((/^(\*\.)?([a-z0-9-]+)(\.[a-z0-9-]+)+$/).test(filter.host)) &&
+        (filter.host !== "")) {
+      throw new TypeError("Expected well formed hostname string for `host` with atmost 1 wildcard.");
+    }
+
+    if (onResult && typeof onResult != "function") {
+      throw new TypeError("Invalid function: " + onResult);
+    }
+
+    return PlacesUtils.withConnectionWrapper(
+      "History.jsm: removeByFilter",
+      db => removeByFilter(db, filter, onResult)
+    );
+  },
+
+  /**
    * Determine if a page has been visited.
    *
    * @param pages: (URL or nsIURI)
    *      The full URI of the page.
    *            or (string)
    *      The full URI of the page or the GUID of the page.
    *
    * @return (Promise)
@@ -442,16 +521,35 @@ this.History = Object.freeze({
    */
   clear() {
     return PlacesUtils.withConnectionWrapper("History.jsm: clear",
       clear
     );
   },
 
   /**
+   * Is a value a valid transition type?
+   *
+   * @param transitionType: (String)
+   * @return (Boolean)
+   */
+  isValidTransition(transitionType) {
+    return Object.values(History.TRANSITIONS).includes(transitionType);
+  },
+
+  /**
+   * Throw if an object is not a Date object.
+   */
+  ensureDate(arg) {
+    if (!arg || typeof arg != "object" || arg.constructor.name != "Date") {
+      throw new TypeError("Expected a Date, got " + arg);
+    }
+  },
+
+  /**
    * Possible values for the `transition` property of `VisitInfo`
    * objects.
    */
 
   TRANSITIONS: {
     /**
      * The user followed a link and got a new toplevel window.
      */
@@ -501,72 +599,20 @@ this.History = Object.freeze({
     /**
      * The user reloaded a page.
      */
     RELOAD: Ci.nsINavHistoryService.TRANSITION_RELOAD,
   },
 });
 
 /**
- * Validate an input PageInfo object, returning a valid PageInfo object.
- *
- * @param pageInfo: (PageInfo)
- * @return (PageInfo)
- */
-function validatePageInfo(pageInfo) {
-  let info = {
-    visits: [],
-  };
-
-  if (!pageInfo.url) {
-    throw new TypeError("PageInfo object must have a url property");
-  }
-
-  info.url = normalizeToURLOrGUID(pageInfo.url);
-
-  if (typeof pageInfo.title === "string") {
-    info.title = pageInfo.title;
-  } else if (pageInfo.title != null && pageInfo.title != undefined) {
-    throw new TypeError(`title property of PageInfo object: ${pageInfo.title} must be a string if provided`);
-  }
-
-  if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
-    throw new TypeError("PageInfo object must have an array of visits");
-  }
-  for (let inVisit of pageInfo.visits) {
-    let visit = {
-      date: new Date(),
-      transition: inVisit.transition || History.TRANSITIONS.LINK,
-    };
-
-    if (!isValidTransitionType(visit.transition)) {
-      throw new TypeError(`transition: ${visit.transition} is not a valid transition type`);
-    }
-
-    if (inVisit.date) {
-      ensureDate(inVisit.date);
-      if (inVisit.date > (Date.now() + TIMERS_RESOLUTION_SKEW_MS)) {
-        throw new TypeError(`date: ${inVisit.date} cannot be a future date`);
-      }
-      visit.date = inVisit.date;
-    }
-
-    if (inVisit.referrer) {
-      visit.referrer = normalizeToURLOrGUID(inVisit.referrer);
-    }
-    info.visits.push(visit);
-  }
-  return info;
-}
-
-/**
  * Convert a PageInfo object into the format expected by updatePlaces.
  *
  * Note: this assumes that the PageInfo object has already been validated
- * via validatePageInfo.
+ * via PlacesUtils.validatePageInfo.
  *
  * @param pageInfo: (PageInfo)
  * @return (info)
  */
 function convertForUpdatePlaces(pageInfo) {
   let info = {
     uri: PlacesUtils.toURI(pageInfo.url),
     title: pageInfo.title,
@@ -580,60 +626,16 @@ function convertForUpdatePlaces(pageInfo
       referrerURI: (inVisit.referrer) ? PlacesUtils.toURI(inVisit.referrer) : undefined,
     };
     info.visits.push(visit);
   }
   return info;
 }
 
 /**
- * Is a value a valid transition type?
- *
- * @param transitionType: (String)
- * @return (Boolean)
- */
-function isValidTransitionType(transitionType) {
-  return Object.values(History.TRANSITIONS).includes(transitionType);
-}
-
-/**
- * Normalize a key to either a string (if it is a valid GUID) or an
- * instance of `URL` (if it is a `URL`, `nsIURI`, or a string
- * representing a valid url).
- *
- * @throws (TypeError)
- *         If the key is neither a valid guid nor a valid url.
- */
-function normalizeToURLOrGUID(key) {
-  if (typeof key === "string") {
-    // A string may be a URL or a guid
-    if (PlacesUtils.isValidGuid(key)) {
-      return key;
-    }
-    return new URL(key);
-  }
-  if (key instanceof URL) {
-    return key;
-  }
-  if (key instanceof Ci.nsIURI) {
-    return new URL(key.spec);
-  }
-  throw new TypeError("Invalid url or guid: " + key);
-}
-
-/**
- * Throw if an object is not a Date object.
- */
-function ensureDate(arg) {
-  if (!arg || typeof arg != "object" || arg.constructor.name != "Date") {
-    throw new TypeError("Expected a Date, got " + arg);
-  }
-}
-
-/**
  * Convert a list of strings or numbers to its SQL
  * representation as a string.
  */
 function sqlList(list) {
   return list.map(JSON.stringify).join();
 }
 
 /**
@@ -916,16 +918,122 @@ var removeVisitsByFilter = Task.async(fu
   } finally {
     // Ensure we cleanup embed visits, even if we bailed out early.
     PlacesUtils.history.clearEmbedVisits();
   }
 
   return visitsToRemove.length != 0;
 });
 
+// Inner implementation of History.removeByFilter
+var removeByFilter = Task.async(function*(db, filter, onResult = null) {
+  // 1. Create fragment for date filtration
+  let dateFilterSQLFragment = "";
+  let conditions = [];
+  let params = {};
+  if ("beginDate" in filter) {
+    conditions.push("v.visit_date >= :begin");
+    params.begin = PlacesUtils.toPRTime(filter.beginDate);
+  }
+  if ("endDate" in filter) {
+    conditions.push("v.visit_date <= :end");
+    params.end = PlacesUtils.toPRTime(filter.endDate);
+  }
+
+  if (conditions.length !== 0) {
+    dateFilterSQLFragment =
+      `EXISTS
+         (SELECT id FROM moz_historyvisits v WHERE v.place_id = h.id AND
+         ${ conditions.join(" AND ") }
+         LIMIT 1)`;
+  }
+
+  // 2. Create fragment for host and subhost filtering
+  let hostFilterSQLFragment = "";
+  if (filter.host || filter.host === "") {
+    // There are four cases that we need to consider,
+    // mozilla.org, *.mozilla.org, localhost, and local files
+
+    if (filter.host.indexOf("*") === 0) {
+      // Case 1: subhost wildcard is specified (*.mozilla.org)
+      let revHost = filter.host.slice(2).split("").reverse().join("");
+      hostFilterSQLFragment =
+        `h.rev_host between :revHostStart and :revHostEnd`;
+      params.revHostStart = revHost + ".";
+      params.revHostEnd = revHost + "/";
+    } else {
+      // This covers the rest (mozilla.org, localhost and local files)
+      let revHost = filter.host.split("").reverse().join("") + ".";
+      hostFilterSQLFragment =
+        `h.rev_host = :hostName`;
+      params.hostName = revHost;
+    }
+  }
+
+  // 3. Find out what needs to be removed
+  let fragmentArray = [hostFilterSQLFragment, dateFilterSQLFragment];
+  let query =
+      `SELECT h.id, url, rev_host, guid, title, frecency, foreign_count
+       FROM moz_places h WHERE
+       (${ fragmentArray.filter(f => f !== "").join(") AND (") })`;
+  let onResultData = onResult ? [] : null;
+  let pages = [];
+  let hasPagesToRemove = false;
+
+  yield db.executeCached(
+    query,
+    params,
+    row => {
+      let hasForeign = row.getResultByName("foreign_count") != 0;
+      if (!hasForeign) {
+        hasPagesToRemove = true;
+      }
+      let id = row.getResultByName("id");
+      let guid = row.getResultByName("guid");
+      let url = row.getResultByName("url");
+      let page = {
+        id,
+        guid,
+        hasForeign,
+        hasVisits: false,
+        url: new URL(url)
+      };
+      pages.push(page);
+      if (onResult) {
+        onResultData.push({
+          guid,
+          title: row.getResultByName("title"),
+          frecency: row.getResultByName("frecency"),
+          url: new URL(url)
+        });
+      }
+    });
+
+  if (pages.length === 0) {
+    // Nothing to do
+    return false;
+  }
+
+  try {
+    yield db.executeTransaction(Task.async(function*() {
+      // 4. Actually remove visits
+      yield db.execute(`DELETE FROM moz_historyvisits
+                        WHERE place_id IN(${ sqlList(pages.map(p => p.id)) })`);
+      // 5. Clean up and notify
+      yield cleanupPages(db, pages);
+    }));
+
+    notifyCleanup(db, pages);
+    notifyOnResult(onResultData, onResult);
+  } finally {
+    PlacesUtils.history.clearEmbedVisits();
+  }
+
+  return hasPagesToRemove;
+});
 
 // Inner implementation of History.remove.
 var remove = Task.async(function*(db, {guids, urls}, onResult = null) {
   // 1. Find out what needs to be removed
   let query =
     `SELECT id, url, guid, foreign_count, title, frecency
      FROM moz_places
      WHERE guid IN (${ sqlList(guids) })
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -59,16 +59,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 // refresh instead.
 const MIN_TRANSACTIONS_FOR_BATCH = 5;
 
 // On Mac OSX, the transferable system converts "\r\n" to "\n\n", where
 // we really just want "\n". On other platforms, the transferable system
 // converts "\r\n" to "\n".
 const NEWLINE = AppConstants.platform == "macosx" ? "\n" : "\r\n";
 
+// Timers resolution is not always good, it can have a 16ms precision on Win.
+const TIMERS_RESOLUTION_SKEW_MS = 16;
+
 function QI_node(aNode, aIID) {
   var result = null;
   try {
     result = aNode.QueryInterface(aIID);
   } catch (e) {
   }
   return result;
 }
@@ -972,16 +975,98 @@ this.PlacesUtils = {
       }
       default:
         throw Cr.NS_ERROR_INVALID_ARG;
     }
     return nodes;
   },
 
   /**
+   * Validate an input PageInfo object, returning a valid PageInfo object.
+   *
+   * @param pageInfo: (PageInfo)
+   * @return (PageInfo)
+   */
+  validatePageInfo(pageInfo, validateVisits = true) {
+    let info = {
+      visits: [],
+    };
+
+    if (!pageInfo.url) {
+      throw new TypeError("PageInfo object must have a url property");
+    }
+
+    info.url = this.normalizeToURLOrGUID(pageInfo.url);
+
+    if (!validateVisits) {
+      return info;
+    }
+
+    if (typeof pageInfo.title === "string") {
+      info.title = pageInfo.title;
+    } else if (pageInfo.title != null && pageInfo.title != undefined) {
+      throw new TypeError(`title property of PageInfo object: ${pageInfo.title} must be a string if provided`);
+    }
+
+    if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
+      throw new TypeError("PageInfo object must have an array of visits");
+    }
+
+    for (let inVisit of pageInfo.visits) {
+      let visit = {
+        date: new Date(),
+        transition: inVisit.transition || History.TRANSITIONS.LINK,
+      };
+
+      if (!PlacesUtils.history.isValidTransition(visit.transition)) {
+        throw new TypeError(`transition: ${visit.transition} is not a valid transition type`);
+      }
+
+      if (inVisit.date) {
+        PlacesUtils.history.ensureDate(inVisit.date);
+        if (inVisit.date > (Date.now() + TIMERS_RESOLUTION_SKEW_MS)) {
+          throw new TypeError(`date: ${inVisit.date} cannot be a future date`);
+        }
+        visit.date = inVisit.date;
+      }
+
+      if (inVisit.referrer) {
+        visit.referrer = this.normalizeToURLOrGUID(inVisit.referrer);
+      }
+      info.visits.push(visit);
+    }
+    return info;
+  },
+
+  /**
+   * Normalize a key to either a string (if it is a valid GUID) or an
+   * instance of `URL` (if it is a `URL`, `nsIURI`, or a string
+   * representing a valid url).
+   *
+   * @throws (TypeError)
+   *         If the key is neither a valid guid nor a valid url.
+   */
+  normalizeToURLOrGUID(key) {
+    if (typeof key === "string") {
+      // A string may be a URL or a guid
+      if (this.isValidGuid(key)) {
+        return key;
+      }
+      return new URL(key);
+    }
+    if (key instanceof URL) {
+      return key;
+    }
+    if (key instanceof Ci.nsIURI) {
+      return new URL(key.spec);
+    }
+    throw new TypeError("Invalid url or guid: " + key);
+  },
+
+  /**
    * Generates a nsINavHistoryResult for the contents of a folder.
    * @param   folderId
    *          The folder to open
    * @param   [optional] excludeItems
    *          True to hide all items (individual bookmarks). This is used on
    *          the left places pane so you just get a folder hierarchy.
    * @param   [optional] expandQueries
    *          True to make query items expand as new containers. For managing,
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/history/test_removeByFilter.js
@@ -0,0 +1,321 @@
+"use strict";
+
+/* This test will ideally test the following cases
+   (each with and without a callback associated with it)
+   * Case A: Tests which should remove pages (Positives)
+   *   Case A 1: Page has multiple visits both in/out of timeframe, all get deleted
+   *   Case A 2: Page has single uri, removed by host
+   *   Case A 3: Page has random subhost, with same host, removed by wildcard
+   *   Case A 4: Page is localhost and localhost:port, removed by host
+   *   Case A 5: Page is a `file://` type address, removed by empty host
+   * Cases A 1,2,3 will be tried with and without bookmarks added (which prevent page deletion)
+   * Case B: Tests in which no pages are removed (Inverses)
+   *   Case B 1 (inverse): Page has no visits in timeframe, and nothing is deleted
+   *   Case B 2: Page has single uri, not removed since hostname is different
+   *   Case B 3: Page has multiple subhosts, not removed since wildcard doesn't match
+   * Case C: Combinations tests
+   *   Case C 1: Single hostname, multiple visits, at least one in timeframe and hostname
+   *   Case C 2: Random subhosts, multiple visits, at least one in timeframe and hostname-wildcard
+   */
+
+add_task(function* test_removeByFilter() {
+  // Cleanup
+  yield PlacesTestUtils.clearHistory();
+  yield PlacesUtils.bookmarks.eraseEverything();
+
+  // Adding a witness URI
+  let witnessURI = NetUtil.newURI("http://witnessmozilla.org/test_browserhistory/test_removeByFilter" + Math.random());
+  yield PlacesTestUtils.addVisits(witnessURI);
+  Assert.ok((yield PlacesTestUtils.isPageInDB(witnessURI)), "Witness URI is in database");
+
+  let removeByFilterTester = Task.async(function*(visits, filter, checkBeforeRemove, checkAfterRemove, useCallback, bookmarkedUri) {
+    // Add visits for URIs
+    yield PlacesTestUtils.addVisits(visits);
+    if (bookmarkedUri !== null && visits.map(v => v.uri).includes(bookmarkedUri)) {
+      yield PlacesUtils.bookmarks.insert({
+        parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+        url: bookmarkedUri,
+        title: "test bookmark"
+      });
+    }
+    checkBeforeRemove();
+
+    // Take care of any observers (due to bookmarks)
+    let { observer, promiseObserved } = getObserverPromise(bookmarkedUri);
+    if (observer) {
+      PlacesUtils.history.addObserver(observer, false);
+    }
+    // Perfom delete operation on database
+    let removed = false;
+    if (useCallback) {
+      // The amount of callbacks will be the unique URIs to remove from the database
+      let netCallbacksRequired = (new Set(visits.map(v => v.uri))).size;
+      removed = yield PlacesUtils.history.removeByFilter(filter, pageInfo => {
+        Assert.ok(PlacesUtils.validatePageInfo(pageInfo, false), "pageInfo should follow a basic format");
+        Assert.ok(netCallbacksRequired > 0, "Callback called as many times as required");
+        netCallbacksRequired--;
+      });
+    } else {
+      removed = yield PlacesUtils.history.removeByFilter(filter);
+    }
+    checkAfterRemove();
+    yield promiseObserved;
+    if (observer) {
+      PlacesUtils.history.removeObserver(observer);
+      // Remove the added bookmarks as they interfere with following tests
+      PlacesUtils.bookmarks.eraseEverything();
+    }
+      Assert.ok((yield PlacesTestUtils.isPageInDB(witnessURI)), "Witness URI is still in database");
+    return removed;
+  });
+
+  const remoteUriList = [ "http://mozilla.org/test_browserhistory/test_removeByFilter/" + Math.random(),
+                          "http://subdomain1.mozilla.org/test_browserhistory/test_removeByFilter/" + Math.random(),
+                          "http://subdomain2.mozilla.org/test_browserhistory/test_removeByFilter/" + Math.random()
+                        ];
+  const localhostUriList = [ "http://localhost:4500/" + Math.random(), "http://localhost/" + Math.random() ];
+  const fileUriList = [ "file:///home/user/files" + Math.random() ];
+  const title = "Title " + Math.random();
+  let sameHostVisits = [
+    {
+      uri: remoteUriList[0],
+      title,
+      visitDate: new Date(2005, 1, 1) * 1000
+    },
+    {
+      uri: remoteUriList[0],
+      title,
+      visitDate: new Date(2005, 3, 3) * 1000
+    },
+    {
+      uri: remoteUriList[0],
+      title,
+      visitDate: new Date(2007, 1, 1) * 1000
+    }
+  ];
+  let randomHostVisits = [
+    {
+      uri: remoteUriList[0],
+      title,
+      visitDate: new Date(2005, 1, 1) * 1000
+    },
+    {
+      uri: remoteUriList[1],
+      title,
+      visitDate: new Date(2005, 3, 3) * 1000
+    },
+    {
+      uri: remoteUriList[2],
+      title,
+      visitDate: new Date(2007, 1, 1) * 1000
+    }
+  ];
+  let localhostVisits = [
+    {
+      uri: localhostUriList[0],
+      title
+    },
+    {
+      uri: localhostUriList[1],
+      title
+    }
+  ];
+  let fileVisits = [
+    {
+      uri: fileUriList[0],
+      title
+    }
+  ];
+  let assertInDB = function*(aUri) {
+      Assert.ok((yield PlacesTestUtils.isPageInDB(aUri)));
+  };
+  let assertNotInDB = function*(aUri) {
+    Assert.ok(!(yield PlacesTestUtils.isPageInDB(aUri)));
+  };
+  for (let callbackUse of [true, false]) {
+    // Case A Positives
+    for (let bookmarkUse of [true, false]) {
+      let bookmarkedUri = (arr) => undefined;
+      let checkableArray = (arr) => arr;
+      let checkClosure = assertNotInDB;
+      if (bookmarkUse) {
+        bookmarkedUri = (arr) => arr[0];
+        checkableArray = (arr) => arr.slice(1);
+        checkClosure = function(aUri) { };
+      }
+      // Case A 1: Dates
+      yield removeByFilterTester(sameHostVisits,
+                                 { beginDate: new Date(2004, 1, 1), endDate: new Date(2006, 1, 1) },
+                                 () => assertInDB(remoteUriList[0]),
+                                 () => checkClosure(remoteUriList[0]),
+                                 callbackUse, bookmarkedUri(remoteUriList));
+      // Case A 2: Single Sub-host
+      yield removeByFilterTester(sameHostVisits, { host: "mozilla.org" },
+                                 () => assertInDB(remoteUriList[0]),
+                                 () => checkClosure(remoteUriList[0]),
+                                 callbackUse, bookmarkedUri(remoteUriList));
+      // Case A 3: Multiple subhost
+      yield removeByFilterTester(randomHostVisits, { host: "*.mozilla.org" },
+                                 () => remoteUriList.forEach(assertInDB),
+                                 () => checkableArray(remoteUriList).forEach(checkClosure),
+                                 callbackUse, bookmarkedUri(remoteUriList));
+    }
+
+    // Case A 4: Localhost
+    yield removeByFilterTester(localhostVisits, { host: "localhost" },
+                               () => localhostUriList.forEach(assertInDB),
+                               () => localhostUriList.forEach(assertNotInDB),
+                               callbackUse);
+    // Case A 5: Local Files
+    yield removeByFilterTester(fileVisits, { host: "" },
+                               () => fileUriList.forEach(assertInDB),
+                               () => fileUriList.forEach(assertNotInDB),
+                               callbackUse);
+
+    // Case B: Tests which do not remove anything (inverses)
+    // Case B 1: Date
+    yield removeByFilterTester(sameHostVisits,
+                               { beginDate: new Date(2001, 1, 1), endDate: new Date(2002, 1, 1) },
+                               () => assertInDB(remoteUriList[0]),
+                               () => assertInDB(remoteUriList[0]),
+                               callbackUse);
+    // Case B 2 : Single subhost
+    yield removeByFilterTester(sameHostVisits, { host: "notthere.org" },
+                               () => assertInDB(remoteUriList[0]),
+                               () => assertInDB(remoteUriList[0]),
+                               callbackUse);
+    // Case B 3 : Multiple subhosts
+    yield removeByFilterTester(randomHostVisits, { host: "*.notthere.org" },
+                               () => remoteUriList.forEach(assertInDB),
+                               () => remoteUriList.forEach(assertInDB),
+                               callbackUse);
+
+    // Case C: Combination Cases
+    // Case C 1: single subhost
+    yield removeByFilterTester(sameHostVisits,
+                               { host: "mozilla.org",
+                                 beginDate: new Date(2004, 1, 1),
+                                 endDate: new Date(2006, 1, 1) },
+                               () => assertInDB(remoteUriList[0]),
+                               () => assertNotInDB(remoteUriList[0]),
+                               callbackUse);
+    // Case C 2: multiple subhost
+    yield removeByFilterTester(randomHostVisits,
+                               { host: "*.mozilla.org",
+                                 beginDate: new Date(2005, 1, 1),
+                                 endDate: new Date(2017, 1, 1) },
+                               () => remoteUriList.forEach(assertInDB),
+                               () => remoteUriList.forEach(assertNotInDB),
+                               callbackUse);
+  }
+});
+
+
+// Test various error cases
+add_task(function* test_error_cases() {
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter(),
+      /TypeError: Expected a filter/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter("obviously, not a filter"),
+      /TypeError: Expected a filter/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({}),
+      /TypeError: Expected a non-empty filter/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeVisitsByFilter({beginDate: "now"}),
+      /TypeError: Expected a Date/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({beginDate: Date.now()}),
+      /TypeError: Expected a Date/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({beginDate: new Date()}, "obviously, not a callback"),
+      /TypeError: Invalid function/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({beginDate: new Date(1000), endDate: new Date(0)}),
+      /TypeError: `beginDate` should be at least as old/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "#"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "*.org"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "www.*.org"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: {}}),
+      /TypeError: `host` should be a string/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: ".mozilla.org"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "*"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "local.host."}),
+      /TypeError: Expected well formed hostname string for/
+  );
+  Assert.throws(
+    () => PlacesUtils.history.removeByFilter({host: "(local files)"}),
+      /TypeError: Expected well formed hostname string for/
+  );
+});
+
+// Helper functions
+
+function getObserverPromise(bookmarkedUri) {
+  if (!bookmarkedUri) {
+    return { observer: null, promiseObserved: Promise.resolve() };
+  }
+  let observer;
+  let promiseObserved = new Promise((resolve, reject) => {
+    observer = {
+      onBeginUpdateBatch() {},
+      onEndUpdateBatch() {},
+      onVisit(aUri) {
+        reject(new Error("Unexpected call to onVisit"));
+      },
+      onTitleChanged(aUri) {
+        reject(new Error("Unexpected call to onTitleChanged"));
+      },
+      onClearHistory() {
+        reject(new Error("Unexpected call to onClearHistory"));
+      },
+      onPageChanged(aUri) {
+        reject(new Error("Unexpected call to onPageChanged"));
+      },
+      onFrecencyChanged(aURI) {},
+      onManyFrecenciesChanged() {},
+      onDeleteURI(aURI) {
+        try {
+          Assert.notEqual(aURI.spec, bookmarkedUri, "Bookmarked URI should not be deleted");
+        } finally {
+          resolve();
+        }
+      },
+      onDeleteVisits(aURI, aVisitTime) {
+        try {
+          Assert.equal(aVisitTime, PlacesUtils.toPRTime(new Date(0)), "Observing onDeleteVisits deletes all visits");
+          Assert.equal(aURI.spec, bookmarkedUri, "Bookmarked URI should have all visits removed but not the page itself");
+        } finally {
+          resolve();
+        }
+      }
+    };
+  });
+  return { observer, promiseObserved };
+}
--- a/toolkit/components/places/tests/history/xpcshell.ini
+++ b/toolkit/components/places/tests/history/xpcshell.ini
@@ -2,11 +2,12 @@
 head = head_history.js
 
 [test_async_history_api.js]
 [test_insert.js]
 [test_insertMany.js]
 [test_remove.js]
 [test_removeMany.js]
 [test_removeVisits.js]
+[test_removeByFilter.js]
 [test_removeVisitsByFilter.js]
 [test_sameUri_titleChanged.js]
 [test_updatePlaces_embed.js]