--- a/services/sync/modules/engines.js
+++ b/services/sync/modules/engines.js
@@ -9,17 +9,19 @@ this.EXPORTED_SYMBOLS = [
"Tracker",
"Store",
"Changeset"
];
var {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
Cu.import("resource://services-common/async.js");
+Cu.import("resource://gre/modules/JSONFile.jsm");
Cu.import("resource://gre/modules/Log.jsm");
+Cu.import("resource://gre/modules/osfile.jsm");
Cu.import("resource://services-common/observers.js");
Cu.import("resource://services-sync/constants.js");
Cu.import("resource://services-sync/identity.js");
Cu.import("resource://services-sync/record.js");
Cu.import("resource://services-sync/resource.js");
Cu.import("resource://services-sync/util.js");
XPCOMUtils.defineLazyModuleGetter(this, "fxAccounts",
@@ -46,19 +48,24 @@ this.Tracker = function Tracker(name, en
this.engine = engine;
this._log = Log.repository.getLogger("Sync.Tracker." + name);
let level = Svc.Prefs.get("log.logger.engine." + this.name, "Debug");
this._log.level = Log.Level[level];
this._score = 0;
this._ignored = [];
+ this._storage = new JSONFile({
+ path: Utils.jsonFilePath("changes/" + this.file),
+ // We use arrow functions instead of `.bind(this)` so that tests can
+ // easily override these hooks.
+ dataPostProcessor: json => this._dataPostProcessor(json),
+ beforeSave: () => this._beforeSave(),
+ });
this.ignoreAll = false;
- this.changedIDs = {};
- this.loadChangedIDs();
Svc.Obs.add("weave:engine:start-tracking", this);
Svc.Obs.add("weave:engine:stop-tracking", this);
Svc.Prefs.observe("engine." + this.engine.prefName, this);
};
Tracker.prototype = {
@@ -71,55 +78,50 @@ Tracker.prototype = {
* 100: Please sync me ASAP!
*
* Setting it to other values should (but doesn't currently) throw an exception
*/
get score() {
return this._score;
},
+ // Default to an empty object if the file doesn't exist.
+ _dataPostProcessor(json) {
+ return typeof json == "object" && json || {};
+ },
+
+ // Ensure the Weave storage directory exists before writing the file.
+ _beforeSave() {
+ let basename = OS.Path.dirname(this._storage.path);
+ return OS.File.makeDir(basename, { from: OS.Constants.Path.profileDir });
+ },
+
+ get changedIDs() {
+ Async.promiseSpinningly(this._storage.load());
+ return this._storage.data;
+ },
+
set score(value) {
this._score = value;
Observers.notify("weave:engine:score:updated", this.name);
},
// Should be called by service everytime a sync has been done for an engine
resetScore: function () {
this._score = 0;
},
persistChangedIDs: true,
- /**
- * Persist changedIDs to disk at a later date.
- * Optionally pass a callback to be invoked when the write has occurred.
- */
- saveChangedIDs: function (cb) {
+ _saveChangedIDs() {
if (!this.persistChangedIDs) {
this._log.debug("Not saving changedIDs.");
return;
}
- Utils.namedTimer(function () {
- this._log.debug("Saving changed IDs to " + this.file);
- Utils.jsonSave("changes/" + this.file, this, this.changedIDs, cb);
- }, 1000, this, "_lazySave");
- },
-
- loadChangedIDs: function (cb) {
- Utils.jsonLoad("changes/" + this.file, this, function(json) {
- if (json && (typeof(json) == "object")) {
- this.changedIDs = json;
- } else if (json !== null) {
- this._log.warn("Changed IDs file " + this.file + " contains non-object value.");
- json = null;
- }
- if (cb) {
- cb.call(this, json);
- }
- });
+ this._storage.saveSoon();
},
// ignore/unignore specific IDs. Useful for ignoring items that are
// being processed, or that shouldn't be synced.
// But note: not persisted to disk
ignoreID: function (id) {
this.unignoreID(id);
@@ -130,17 +132,17 @@ Tracker.prototype = {
let index = this._ignored.indexOf(id);
if (index != -1)
this._ignored.splice(index, 1);
},
_saveChangedID(id, when) {
this._log.trace(`Adding changed ID: ${id}, ${JSON.stringify(when)}`);
this.changedIDs[id] = when;
- this.saveChangedIDs(this.onSavedChangedIDs);
+ this._saveChangedIDs();
},
addChangedID: function (id, when) {
if (!id) {
this._log.warn("Attempted to add undefined ID to tracker");
return false;
}
@@ -174,24 +176,24 @@ Tracker.prototype = {
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();
+ this._saveChangedIDs();
return true;
},
clearChangedIDs: function () {
this._log.trace("Clearing changed ID list");
- this.changedIDs = {};
- this.saveChangedIDs();
+ this._storage.data = {};
+ this._saveChangedIDs();
},
_now() {
return Date.now() / 1000;
},
_isTracking: false,
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -856,18 +856,16 @@ BookmarksStore.prototype = {
// all concepts of "add a changed ID." However, it still registers an observer
// to bump the score, so that changed bookmarks are synced immediately.
function BookmarksTracker(name, engine) {
this._batchDepth = 0;
this._batchSawScoreIncrement = false;
this._migratedOldEntries = false;
Tracker.call(this, name, engine);
- delete this.changedIDs; // so our getter/setter takes effect.
-
Svc.Obs.add("places-shutdown", this);
}
BookmarksTracker.prototype = {
__proto__: Tracker.prototype,
//`_ignore` checks the change source for each observer notification, so we
// don't want to let the engine ignore all changes during a sync.
get ignoreAll() {
@@ -904,41 +902,26 @@ BookmarksTracker.prototype = {
removeChangedID(id) {
throw new Error("Don't remove IDs from the bookmarks tracker");
},
// This method is called at various times, so we override with a no-op
// instead of throwing.
clearChangedIDs() {},
- saveChangedIDs(cb) {
- if (cb) {
- cb();
- }
- },
-
- loadChangedIDs(cb) {
- if (cb) {
- cb();
- }
- },
-
promiseChangedIDs() {
return PlacesSyncUtils.bookmarks.pullChanges();
},
get changedIDs() {
throw new Error("Use promiseChangedIDs");
},
set changedIDs(obj) {
- // let engine init set it to nothing.
- if (Object.keys(obj).length != 0) {
- throw new Error("Don't set initial changed bookmark IDs");
- }
+ throw new Error("Don't set initial changed bookmark IDs");
},
// Migrates tracker entries from the old JSON-based tracker to Places. This
// is called the first time we start tracking changes.
_migrateOldEntries: Task.async(function* () {
let existingIDs = yield Utils.jsonLoad("changes/" + this.file, this);
if (existingIDs === null) {
// If the tracker file doesn't exist, we don't need to migrate, even if
--- a/services/sync/modules/engines/extension-storage.js
+++ b/services/sync/modules/engines/extension-storage.js
@@ -89,20 +89,16 @@ ExtensionStorageTracker.prototype = {
// Single adds, removes and changes are not so important on their
// own, so let's just increment score a bit.
this.score += SCORE_INCREMENT_MEDIUM;
},
// Override a bunch of methods which don't do anything for us.
// This is a performance hack.
- saveChangedIDs: function() {
- },
- loadChangedIDs: function() {
- },
ignoreID: function() {
},
unignoreID: function() {
},
addChangedID: function() {
},
removeChangedID: function() {
},
--- a/services/sync/modules/engines/prefs.js
+++ b/services/sync/modules/engines/prefs.js
@@ -221,20 +221,16 @@ PrefTracker.prototype = {
get modified() {
return Svc.Prefs.get("engine.prefs.modified", false);
},
set modified(value) {
Svc.Prefs.set("engine.prefs.modified", value);
},
- loadChangedIDs: function loadChangedIDs() {
- // Don't read changed IDs from disk at start up.
- },
-
clearChangedIDs: function clearChangedIDs() {
this.modified = false;
},
__prefs: null,
get _prefs() {
if (!this.__prefs) {
this.__prefs = new Preferences();
--- a/services/sync/modules/engines/tabs.js
+++ b/services/sync/modules/engines/tabs.js
@@ -285,20 +285,16 @@ function TabTracker(name, engine) {
this.onTab = Utils.bind2(this, this.onTab);
this._unregisterListeners = Utils.bind2(this, this._unregisterListeners);
}
TabTracker.prototype = {
__proto__: Tracker.prototype,
QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
- loadChangedIDs: function () {
- // Don't read changed IDs from disk at start up.
- },
-
clearChangedIDs: function () {
this.modified = false;
},
_topics: ["pageshow", "TabOpen", "TabClose", "TabSelect"],
_registerListenersForWindow: function (window) {
this._log.trace("Registering tab listeners in window");
--- a/services/sync/modules/util.js
+++ b/services/sync/modules/util.js
@@ -327,31 +327,35 @@ this.Utils = {
* Take a base64-encoded 128-bit AES key, returning it as five groups of five
* uppercase alphanumeric characters, separated by hyphens.
* A.K.A. base64-to-base32 encoding.
*/
presentEncodedKeyAsSyncKey : function presentEncodedKeyAsSyncKey(encodedKey) {
return Utils.encodeKeyBase32(atob(encodedKey));
},
+ jsonFilePath(filePath) {
+ return OS.Path.normalize(OS.Path.join(OS.Constants.Path.profileDir, "weave", filePath + ".json"));
+ },
+
/**
* Load a JSON file from disk in the profile directory.
*
* @param filePath
* JSON file path load from profile. Loaded file will be
* <profile>/<filePath>.json. i.e. Do not specify the ".json"
* extension.
* @param that
* Object to use for logging and "this" for callback.
* @param callback
* Function to process json object as its first argument. If the file
* could not be loaded, the first argument will be undefined.
*/
jsonLoad: Task.async(function*(filePath, that, callback) {
- let path = OS.Path.join(OS.Constants.Path.profileDir, "weave", filePath + ".json");
+ let path = Utils.jsonFilePath(filePath);
if (that._log) {
that._log.trace("Loading json from disk: " + filePath);
}
let json;
try {
--- a/services/sync/tests/unit/test_engine.js
+++ b/services/sync/tests/unit/test_engine.js
@@ -1,11 +1,12 @@
/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */
+Cu.import("resource://gre/modules/osfile.jsm");
Cu.import("resource://services-common/observers.js");
Cu.import("resource://services-sync/engines.js");
Cu.import("resource://services-sync/service.js");
Cu.import("resource://services-sync/util.js");
function SteamStore(engine) {
Store.call(this, "Steam", engine);
this.wasWiped = false;
@@ -58,79 +59,75 @@ var engineObserver = {
};
Observers.add("weave:engine:reset-client:start", engineObserver);
Observers.add("weave:engine:reset-client:finish", engineObserver);
Observers.add("weave:engine:wipe-client:start", engineObserver);
Observers.add("weave:engine:wipe-client:finish", engineObserver);
Observers.add("weave:engine:sync:start", engineObserver);
Observers.add("weave:engine:sync:finish", engineObserver);
-function run_test() {
- run_next_test();
-}
-
-add_test(function test_members() {
+add_task(async function test_members() {
_("Engine object members");
let engine = new SteamEngine("Steam", Service);
do_check_eq(engine.Name, "Steam");
do_check_eq(engine.prefName, "steam");
do_check_true(engine._store instanceof SteamStore);
do_check_true(engine._tracker instanceof SteamTracker);
- run_next_test();
});
-add_test(function test_score() {
+add_task(async function test_score() {
_("Engine.score corresponds to tracker.score and is readonly");
let engine = new SteamEngine("Steam", Service);
do_check_eq(engine.score, 0);
engine._tracker.score += 5;
do_check_eq(engine.score, 5);
try {
engine.score = 10;
} catch(ex) {
// Setting an attribute that has a getter produces an error in
// Firefox <= 3.6 and is ignored in later versions. Either way,
// the attribute's value won't change.
}
do_check_eq(engine.score, 5);
- run_next_test();
});
-add_test(function test_resetClient() {
+add_task(async function test_resetClient() {
_("Engine.resetClient calls _resetClient");
let engine = new SteamEngine("Steam", Service);
do_check_false(engine.wasReset);
engine.resetClient();
do_check_true(engine.wasReset);
do_check_eq(engineObserver.topics[0], "weave:engine:reset-client:start");
do_check_eq(engineObserver.topics[1], "weave:engine:reset-client:finish");
engine.wasReset = false;
engineObserver.reset();
- run_next_test();
});
-add_test(function test_invalidChangedIDs() {
+add_task(async function test_invalidChangedIDs() {
_("Test that invalid changed IDs on disk don't end up live.");
let engine = new SteamEngine("Steam", Service);
let tracker = engine._tracker;
- tracker.changedIDs = 5;
- tracker.saveChangedIDs(function onSaved() {
- tracker.changedIDs = {placeholder: true};
- tracker.loadChangedIDs(function onLoaded(json) {
- do_check_null(json);
- do_check_true(tracker.changedIDs.placeholder);
- run_next_test();
- });
- });
+
+ await tracker._beforeSave();
+ await OS.File.writeAtomic(tracker._storage.path, new TextEncoder().encode("5"),
+ { tmpPath: tracker._storage.path + ".tmp" });
+
+ ok(!tracker._storage.dataReady);
+ tracker.changedIDs.placeholder = true;
+ deepEqual(tracker.changedIDs, { placeholder: true },
+ "Accessing changed IDs should load changes from disk as a side effect");
+ ok(tracker._storage.dataReady);
+
+ do_check_true(tracker.changedIDs.placeholder);
});
-add_test(function test_wipeClient() {
+add_task(async function test_wipeClient() {
_("Engine.wipeClient calls resetClient, wipes store, clears changed IDs");
let engine = new SteamEngine("Steam", Service);
do_check_false(engine.wasReset);
do_check_false(engine._store.wasWiped);
do_check_true(engine._tracker.addChangedID("a-changed-id"));
do_check_true("a-changed-id" in engine._tracker.changedIDs);
engine.wipeClient();
@@ -140,61 +137,58 @@ add_test(function test_wipeClient() {
do_check_eq(engineObserver.topics[0], "weave:engine:wipe-client:start");
do_check_eq(engineObserver.topics[1], "weave:engine:reset-client:start");
do_check_eq(engineObserver.topics[2], "weave:engine:reset-client:finish");
do_check_eq(engineObserver.topics[3], "weave:engine:wipe-client:finish");
engine.wasReset = false;
engine._store.wasWiped = false;
engineObserver.reset();
- run_next_test();
});
-add_test(function test_enabled() {
+add_task(async function test_enabled() {
_("Engine.enabled corresponds to preference");
let engine = new SteamEngine("Steam", Service);
try {
do_check_false(engine.enabled);
Svc.Prefs.set("engine.steam", true);
do_check_true(engine.enabled);
engine.enabled = false;
do_check_false(Svc.Prefs.get("engine.steam"));
- run_next_test();
} finally {
Svc.Prefs.resetBranch("");
}
});
-add_test(function test_sync() {
+add_task(async function test_sync() {
let engine = new SteamEngine("Steam", Service);
try {
_("Engine.sync doesn't call _sync if it's not enabled");
do_check_false(engine.enabled);
do_check_false(engine.wasSynced);
engine.sync();
do_check_false(engine.wasSynced);
_("Engine.sync calls _sync if it's enabled");
engine.enabled = true;
engine.sync();
do_check_true(engine.wasSynced);
do_check_eq(engineObserver.topics[0], "weave:engine:sync:start");
do_check_eq(engineObserver.topics[1], "weave:engine:sync:finish");
- run_next_test();
} finally {
Svc.Prefs.resetBranch("");
engine.wasSynced = false;
engineObserver.reset();
}
});
-add_test(function test_disabled_no_track() {
+add_task(async function test_disabled_no_track() {
_("When an engine is disabled, its tracker is not tracking.");
let engine = new SteamEngine("Steam", Service);
let tracker = engine._tracker;
do_check_eq(engine, tracker.engine);
do_check_false(engine.enabled);
do_check_false(tracker._isTracking);
do_check_empty(tracker.changedIDs);
@@ -209,11 +203,9 @@ add_test(function test_disabled_no_track
do_check_true(tracker._isTracking);
do_check_empty(tracker.changedIDs);
tracker.addChangedID("abcdefghijkl");
do_check_true(0 < tracker.changedIDs["abcdefghijkl"]);
Svc.Prefs.set("engine." + engine.prefName, false);
do_check_false(tracker._isTracking);
do_check_empty(tracker.changedIDs);
-
- run_next_test();
});
--- a/services/sync/tests/unit/test_history_tracker.js
+++ b/services/sync/tests/unit/test_history_tracker.js
@@ -13,27 +13,60 @@ Cu.import("resource://services-sync/util
Service.engineManager.clear();
Service.engineManager.register(HistoryEngine);
var engine = Service.engineManager.get("history");
var tracker = engine._tracker;
// Don't write out by default.
tracker.persistChangedIDs = false;
+// Places notifies history observers asynchronously, so `addVisits` might return
+// before the tracker receives the notification. This helper registers an
+// observer that resolves once the expected notification fires.
+async function promiseVisit(expectedType, expectedURI) {
+ return new Promise(resolve => {
+ function done(type, uri) {
+ if (uri.equals(expectedURI) && type == expectedType) {
+ PlacesUtils.history.removeObserver(observer);
+ resolve();
+ }
+ }
+ let observer = {
+ onVisit(uri) {
+ done("added", uri);
+ },
+ onBeginUpdateBatch() {},
+ onEndUpdateBatch() {},
+ onTitleChanged() {},
+ onFrecencyChanged() {},
+ onManyFrecenciesChanged() {},
+ onDeleteURI(uri) {
+ done("removed", uri);
+ },
+ onClearHistory() {},
+ onPageChanged() {},
+ onDeleteVisits() {},
+ };
+ PlacesUtils.history.addObserver(observer, false);
+ });
+}
+
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);
+ let visitAddedPromise = promiseVisit("added", uri);
await PlacesTestUtils.addVisits({
uri,
visitDate: Date.now() * 1000,
transition,
referrer,
});
+ await visitAddedPromise;
return uri;
}
function run_test() {
initTestLogging("Trace");
Log.repository.getLogger("Sync.Tracker.History").level = Log.Level.Trace;
run_next_test();
@@ -95,23 +128,30 @@ add_task(async function test_not_trackin
await addVisit("not_tracking");
await verifyTrackerEmpty();
await cleanup();
});
add_task(async function test_start_tracking() {
_("Add hook for save completion.");
- let savePromise = new Promise(resolve => {
+ let savePromise = new Promise((resolve, reject) => {
tracker.persistChangedIDs = true;
- tracker.onSavedChangedIDs = function () {
- // Turn this back off.
- tracker.persistChangedIDs = false;
- delete tracker.onSavedChangedIDs;
- resolve();
+ let save = tracker._storage._save;
+ tracker._storage._save = async function() {
+ try {
+ await save.call(this);
+ resolve();
+ } catch (ex) {
+ reject(ex);
+ } finally {
+ // Turn this back off.
+ tracker.persistChangedIDs = false;
+ tracker._storage._save = save;
+ }
};
});
_("Tell the tracker to start tracking changes.");
await startTracking();
let scorePromise = promiseOneObserver("weave:engine:score:updated");
await addVisit("start_tracking");
await scorePromise;
@@ -151,19 +191,20 @@ add_task(async function test_track_delet
// This isn't present because we weren't tracking when it was visited.
await addVisit("track_delete");
let uri = Utils.makeURI("http://getfirefox.com/track_delete");
let guid = engine._store.GUIDForUri(uri);
await verifyTrackerEmpty();
await startTracking();
+ let visitRemovedPromise = promiseVisit("removed", uri);
let scorePromise = promiseOneObserver("weave:engine:score:updated");
PlacesUtils.history.removePage(uri);
- await scorePromise;
+ await Promise.all([scorePromise, visitRemovedPromise]);
await verifyTrackedItems([guid]);
do_check_eq(tracker.score, SCORE_INCREMENT_XLARGE);
await cleanup();
});
add_task(async function test_dont_track_expiration() {
@@ -172,32 +213,33 @@ add_task(async function test_dont_track_
let guidToExpire = engine._store.GUIDForUri(uriToExpire);
let uriToRemove = await addVisit("to_remove");
let guidToRemove = engine._store.GUIDForUri(uriToRemove);
await resetTracker();
await verifyTrackerEmpty();
await startTracking();
+ let visitRemovedPromise = promiseVisit("removed", uriToRemove);
let scorePromise = promiseOneObserver("weave:engine:score:updated");
// Observe expiration.
Services.obs.addObserver(function onExpiration(aSubject, aTopic, aData) {
Services.obs.removeObserver(onExpiration, aTopic);
// Remove the remaining page to update its score.
PlacesUtils.history.removePage(uriToRemove);
}, PlacesUtils.TOPIC_EXPIRATION_FINISHED, false);
// Force expiration of 1 entry.
Services.prefs.setIntPref("places.history.expiration.max_pages", 0);
Cc["@mozilla.org/places/expiration;1"]
.getService(Ci.nsIObserver)
.observe(null, "places-debug-start-expiration", 1);
- await scorePromise;
+ await Promise.all([scorePromise, visitRemovedPromise]);
await verifyTrackedItems([guidToRemove]);
await cleanup();
});
add_task(async function test_stop_tracking() {
_("Let's stop tracking again.");
await stopTracking();
--- a/services/sync/tests/unit/test_tracker_addChanged.js
+++ b/services/sync/tests/unit/test_tracker_addChanged.js
@@ -1,20 +1,16 @@
/* Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/ */
Cu.import("resource://services-sync/engines.js");
Cu.import("resource://services-sync/service.js");
Cu.import("resource://services-sync/util.js");
-function run_test() {
- run_next_test();
-}
-
-add_test(function test_tracker_basics() {
+add_task(async function test_tracker_basics() {
let tracker = new Tracker("Tracker", Service);
tracker.persistChangedIDs = false;
let id = "the_id!";
_("Make sure nothing exists yet..");
do_check_eq(tracker.changedIDs[id], null);
@@ -28,32 +24,34 @@ add_test(function test_tracker_basics()
_("An older time will not replace the newer 10");
tracker.addChangedID(id, 5);
do_check_eq(tracker.changedIDs[id], 10);
_("Adding without time defaults to current time");
tracker.addChangedID(id);
do_check_true(tracker.changedIDs[id] > 10);
-
- run_next_test();
});
-add_test(function test_tracker_persistence() {
+add_task(async function test_tracker_persistence() {
let tracker = new Tracker("Tracker", Service);
let id = "abcdef";
tracker.persistChangedIDs = true;
- tracker.onSavedChangedIDs = function () {
- _("IDs saved.");
- do_check_eq(5, tracker.changedIDs[id]);
- // Verify the write by reading the file back.
- Utils.jsonLoad("changes/tracker", this, function (json) {
- do_check_eq(5, json[id]);
- tracker.persistChangedIDs = false;
- delete tracker.onSavedChangedIDs;
- run_next_test();
- });
- };
+ let promiseSave = new Promise((resolve, reject) => {
+ let save = tracker._storage._save;
+ tracker._storage._save = function() {
+ save.call(tracker._storage).then(resolve, reject);
+ };
+ });
tracker.addChangedID(id, 5);
+
+ await promiseSave;
+
+ _("IDs saved.");
+ do_check_eq(5, tracker.changedIDs[id]);
+
+ let json = await Utils.jsonLoad("changes/tracker", tracker);
+ do_check_eq(5, json[id]);
+ tracker.persistChangedIDs = false;
});