diff --git a/services/sync/modules/constants.js b/services/sync/modules/constants.js index f70bbd61c18b..2dac53a844ab 100644 --- a/services/sync/modules/constants.js +++ b/services/sync/modules/constants.js @@ -192,6 +192,8 @@ MIN_PASS_LENGTH: 8, DEVICE_TYPE_DESKTOP: "desktop", DEVICE_TYPE_MOBILE: "mobile", +SQLITE_MAX_VARIABLE_NUMBER: 999, + })) { this[key] = val; this.EXPORTED_SYMBOLS.push(key); diff --git a/services/sync/modules/engines.js b/services/sync/modules/engines.js index 8bc0e7bbfec5..2d88078b75b7 100644 --- a/services/sync/modules/engines.js +++ b/services/sync/modules/engines.js @@ -161,19 +161,25 @@ Tracker.prototype = { return true; }, - removeChangedID: function (id) { - if (!id) { - this._log.warn("Attempted to remove undefined ID to tracker"); + removeChangedID: function (...ids) { + if (!ids.length || this.ignoreAll) { return false; } - if (this.ignoreAll || this._ignored.includes(id)) { - return false; - } - if (this.changedIDs[id] != null) { - this._log.trace("Removing changed ID " + id); - delete this.changedIDs[id]; - this.saveChangedIDs(); + for (let id of ids) { + if (!id) { + this._log.warn("Attempted to remove undefined ID from tracker"); + continue; + } + if (this._ignored.includes(id)) { + this._log.debug(`Not removing ignored ID ${id} from tracker`); + continue; + } + if (this.changedIDs[id] != null) { + this._log.trace("Removing changed ID " + id); + delete this.changedIDs[id]; + } } + this.saveChangedIDs(); return true; }, diff --git a/services/sync/modules/engines/history.js b/services/sync/modules/engines/history.js index 6492a56ec687..0fa479113e90 100644 --- a/services/sync/modules/engines/history.js +++ b/services/sync/modules/engines/history.js @@ -64,6 +64,50 @@ HistoryEngine.prototype = { notifyHistoryObservers("onEndUpdateBatch"); } }, + + pullNewChanges() { + let modifiedGUIDs = Object.keys(this._tracker.changedIDs); + if (!modifiedGUIDs.length) { + return new Changeset(); + } + + let db = PlacesUtils.history.QueryInterface(Ci.nsPIPlacesDatabase) + .DBConnection; + + // Filter out hidden pages and `TRANSITION_FRAMED_LINK` visits. These are + // excluded when rendering the history menu, so we use the same constraints + // for Sync. We also don't want to sync `TRANSITION_EMBED` visits, but those + // aren't stored in the database. + for (let startIndex = 0; + startIndex < modifiedGUIDs.length; + startIndex += SQLITE_MAX_VARIABLE_NUMBER) { + + let chunkLength = Math.min(SQLITE_MAX_VARIABLE_NUMBER, + modifiedGUIDs.length - startIndex); + + let query = ` + SELECT DISTINCT p.guid FROM moz_places p + JOIN moz_historyvisits v ON p.id = v.place_id + WHERE p.guid IN (${new Array(chunkLength).fill("?").join(",")}) AND + (p.hidden = 1 OR v.visit_type IN (0, + ${PlacesUtils.history.TRANSITION_FRAMED_LINK})) + `; + + let statement = db.createAsyncStatement(query); + try { + for (let i = 0; i < chunkLength; i++) { + statement.bindByIndex(i, modifiedGUIDs[startIndex + i]); + } + let results = Async.querySpinningly(statement, ["guid"]); + let guids = results.map(result => result.guid); + this._tracker.removeChangedID(...guids); + } finally { + statement.finalize(); + } + } + + return new Changeset(this._tracker.changedIDs); + }, }; function HistoryStore(name, engine) { @@ -72,7 +116,7 @@ function HistoryStore(name, engine) { // Explicitly nullify our references to our cached services so we don't leak Svc.Obs.add("places-shutdown", function() { for (let query in this._stmts) { - let stmt = this._stmts; + let stmt = this._stmts[query]; stmt.finalize(); } this._stmts = {}; @@ -165,12 +209,18 @@ HistoryStore.prototype = { _urlCols: ["url", "title", "frecency"], get _allUrlStm() { - return this._getStmt( - "SELECT url " + - "FROM moz_places " + - "WHERE last_visit_date > :cutoff_date " + - "ORDER BY frecency DESC " + - "LIMIT :max_results"); + // Filter out hidden pages and framed link visits. See `pullNewChanges` + // for more info. + return this._getStmt(` + SELECT DISTINCT p.url + FROM moz_places p + JOIN moz_historyvisits v ON p.id = v.place_id + WHERE p.last_visit_date > :cutoff_date AND + p.hidden = 0 AND + v.visit_type NOT IN (0, + ${PlacesUtils.history.TRANSITION_FRAMED_LINK}) + ORDER BY frecency DESC + LIMIT :max_results`); }, _allUrlCols: ["url"], diff --git a/services/sync/tests/unit/test_history_tracker.js b/services/sync/tests/unit/test_history_tracker.js index 209a81499129..1f8a48a297d1 100644 --- a/services/sync/tests/unit/test_history_tracker.js +++ b/services/sync/tests/unit/test_history_tracker.js @@ -1,6 +1,7 @@ /* Any copyright is dedicated to the Public Domain. http://creativecommons.org/publicdomain/zero/1.0/ */ +Cu.import("resource://gre/modules/PlacesDBUtils.jsm"); Cu.import("resource://gre/modules/PlacesUtils.jsm"); Cu.import("resource://gre/modules/XPCOMUtils.jsm"); Cu.import("resource://services-sync/engines.js"); @@ -17,7 +18,7 @@ var tracker = engine._tracker; // Don't write out by default. tracker.persistChangedIDs = false; -async function addVisit(suffix) { +async function addVisit(suffix, referrer = null, transition = PlacesUtils.history.TRANSITION_LINK) { let uriString = "http://getfirefox.com/" + suffix; let uri = Utils.makeURI(uriString); _("Adding visit for URI " + uriString); @@ -25,7 +26,8 @@ async function addVisit(suffix) { await PlacesTestUtils.addVisits({ uri, visitDate: Date.now() * 1000, - transition: PlacesUtils.history.TRANSITION_LINK, + transition, + referrer, }); return uri; @@ -216,3 +218,34 @@ add_task(async function test_stop_tracking_twice() { await cleanup(); }); + +add_task(async function test_filter_hidden() { + await startTracking(); + + _("Add visit; should be hidden by the redirect"); + let hiddenURI = await addVisit("hidden"); + let hiddenGUID = engine._store.GUIDForUri(hiddenURI); + _(`Hidden visit GUID: ${hiddenGUID}`); + + _("Add redirect visit; should be tracked"); + let trackedURI = await addVisit("redirect", hiddenURI, + PlacesUtils.history.TRANSITION_REDIRECT_PERMANENT); + let trackedGUID = engine._store.GUIDForUri(trackedURI); + _(`Tracked visit GUID: ${trackedGUID}`); + + _("Add visit for framed link; should be ignored"); + let embedURI = await addVisit("framed_link", null, + PlacesUtils.history.TRANSITION_FRAMED_LINK); + let embedGUID = engine._store.GUIDForUri(embedURI); + _(`Framed link visit GUID: ${embedGUID}`); + + _("Run Places maintenance to mark redirect visit as hidden"); + let maintenanceFinishedPromise = + promiseOneObserver("places-maintenance-finished"); + PlacesDBUtils.maintenanceOnIdle(); + await maintenanceFinishedPromise; + + await verifyTrackedItems([trackedGUID]); + + await cleanup(); +});