This is because we wait 1s before writing the file, which by that time will be too late. The easiest solution for this is probably to use the new JsonFile module here.

Comment on attachment 8819131 [details] Bug 1319175 - Add a `data` setter and `beforeSave` hook to `JSONFile`. https://reviewboard.mozilla.org/r/98968/#review99258 ::: toolkit/modules/JSONFile.jsm:84 (Diff revision 1) > + * - beforeSave: Promise-returning function triggered just before the > + * data is written to disk. This can be used to create any > + * intermediate directories before saving. Perhaps we should define in the comment what happens if beforeSave throws or returns a rejected promise? Do we want it to prevent a write?

Comment on attachment 8819132 [details] Bug 1319175 - Switch to `JSONFile` for tracker persistence. https://reviewboard.mozilla.org/r/98970/#review99266 This looks good with the suggested changes, thanks! ::: services/sync/modules/engines.js:55 (Diff revision 1) > let level = Svc.Prefs.get("log.logger.engine." + this.name, "Debug"); > this._log.level = Log.Level[level]; > > this._score = 0; > this._ignored = []; > + this._store = new JSONFile({ I think `this._store` might end up causing confusion with our "store" object and `this._store` on engines. Maybe `this._storage`? ::: services/sync/modules/engines.js:93 (Diff revision 1) > + let basename = OS.Path.dirname(this._store.path); > + return OS.File.makeDir(basename, { from: OS.Constants.Path.profileDir }); > + }, > + > + get changedIDs() { > + this._store.ensureDataReady(); Isn't ensureDataReady going to be janky for large change sets? It might be better to use its "load" method and promiseSpinningly? ::: services/sync/tests/unit/test_engine.js (Diff revision 1) > engine.wasReset = false; > engineObserver.reset(); > run_next_test(); > }); > > -add_test(function test_invalidChangedIDs() { ISTM this test might still have value, although changing how the "invalid" data starts on disk would be necessary.

Comment on attachment 8819132 [details] Bug 1319175 - Switch to `JSONFile` for tracker persistence. https://reviewboard.mozilla.org/r/98970/#review99266 > Isn't ensureDataReady going to be janky for large change sets? It might be better to use its "load" method and promiseSpinningly? Using `promiseSpinningly` here has an interesting side effect: scheduled async functions (and tasks) will still run in the meantime. In http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/services/sync/tests/unit/test_history_tracker.js#132-133, for example, the `changedIDs` getter will run on the tick *after* `verifyTrackedCount`, meaning the tracker won't see the new value yet. It's easy to work around; I can avoid calling `Async.promiseSpinningly(this._storage.load())` if the data is already loaded. But I suspect this kind of "synchronously asynchronous" getter might still cause a problem where we bump the score, but `changedIDs` won't have the new change yet. What do you think is the right approach here? AFAICT, the old persistence didn't really handle this (http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/services/sync/modules/engines.js#55-56,109). Any changes that were added before `loadChangedIDs` would get clobbered when `loadChangedIDs` finished, and any persisted changes would get clobbered if `saveChangedIDs` finished before `loadChangedIDs`. > ISTM this test might still have value, although changing how the "invalid" data starts on disk would be necessary. Sure thing. Rewrote the test so that it starts out with invalid data, since I removed the `changedIDs` setter.

Comment on attachment 8819131 [details] Bug 1319175 - Add a `data` setter and `beforeSave` hook to `JSONFile`. https://reviewboard.mozilla.org/r/98968/#review99258 > Perhaps we should define in the comment what happens if beforeSave throws or returns a rejected promise? Do we want it to prevent a write? I think we do. Sync uses this to create directories; if that fails, the write will fail, anyway. Clarified in the comment and added a test. Thanks!

Mark, please take another look. This should fix the intermittent failure we were seeing yesterday.

Comment on attachment 8819132 [details] Bug 1319175 - Switch to `JSONFile` for tracker persistence. https://reviewboard.mozilla.org/r/98970/#review100368 Looks great, thanks!

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1febc9e8d6d5 Add a `data` setter and `beforeSave` hook to `JSONFile`. r=MattN https://hg.mozilla.org/integration/autoland/rev/ced746a2532e Switch to `JSONFile` for tracker persistence. r=markh

https://hg.mozilla.org/mozilla-central/rev/1febc9e8d6d5 https://hg.mozilla.org/mozilla-central/rev/ced746a2532e