From 5a2ad971c32928297e5a8af1dd4d04eb5989a3c7 Mon Sep 17 00:00:00 2001 From: Mike Conley Date: Tue, 13 May 2025 17:50:25 +0000 Subject: [PATCH] Bug 1965034 - Add an optional property to JSONFile to allow consumers to react to save failures. r=rpl,Gijs Differential Revision: https://phabricator.services.mozilla.com/D248283 --- toolkit/modules/JSONFile.sys.mjs | 88 +++++++++++++------ .../modules/tests/xpcshell/test_JSONFile.js | 30 +++++++ 2 files changed, 89 insertions(+), 29 deletions(-) diff --git a/toolkit/modules/JSONFile.sys.mjs b/toolkit/modules/JSONFile.sys.mjs index 6e50384ae8ee..6909c46d5993 100644 --- a/toolkit/modules/JSONFile.sys.mjs +++ b/toolkit/modules/JSONFile.sys.mjs @@ -56,35 +56,41 @@ const kSaveDelayMs = 1500; /** * Handles serialization of the data and persistence into a file. * - * @param config An object containing following members: - * - path: String containing the file path where data should be saved. - * - sanitizedBasename: Sanitized string identifier used for logging, - * shutdown debugging, and telemetry. Defaults to - * basename of given `path`, sanitized. - * - dataPostProcessor: Function triggered when data is just loaded. The - * data object will be passed as the first argument - * and should be returned no matter it's modified or - * not. Its failure leads to the failure of load() - * and ensureDataReady(). - * - saveDelayMs: Number indicating the delay (in milliseconds) between a - * change to the data and the related save operation. The - * default value will be applied if omitted. - * - beforeSave: Promise-returning function triggered just before the - * data is written to disk. This can be used to create any - * intermediate directories before saving. The file will - * not be saved if the promise rejects or the function - * throws an exception. - * - finalizeAt: An `IOUtils` phase or barrier client that should - * automatically finalize the file when triggered. Defaults - * to `profileBeforeChange`; exposed as an option for - * testing. - * - compression: A compression algorithm to use when reading and - * writing the data. - * - backupTo: A string value indicating where writeAtomic should create - * a backup before writing to json files. Note that using this - * option currently ensures that we automatically restore backed - * up json files in load() and ensureDataReady() when original - * files are missing or corrupt. + * @param {Object} config An object containing following members: + * @param {string} config.path + * String containing the file path where data should be saved. + * @param {string} [config.sanitizedBasename] + * Sanitized string identifier used for logging, + * shutdown debugging, and telemetry. Defaults to basename of given `path`, + * sanitized. + * @param {Function} [config.dataPostProcessor] + * Function triggered when data is just loaded. The data object will be passed + * as the first argument and should be returned no matter if it's modified or + * not. Its failure leads to the failure of load() and ensureDataReady(). + * @param {number} [config.saveDelayMs] + * Number indicating the delay (in milliseconds) between a change to the data + * and the related save operation. The default value will be applied if + * omitted. + * @param {Function} [config.beforeSave] + * Promise-returning function triggered just before the data is written to + * disk. This can be used to create any intermediate directories before + * saving. The file will not be saved if the promise rejects or the function + * throws an exception. + * @param {nsIAsyncShutdownClient} [config.finalizeAt] + * An `IOUtils` phase or barrier client that should automatically finalize the + * file when triggered. Defaults to `profileBeforeChange`; exposed as an + * option for testing. + * @param {string} [config.compression] + * A compression algorithm to use when reading and writing the data. + * @param {string} [config.backupTo] + * A string value indicating where writeAtomic should create a backup before + * writing to json files. Note that using this option currently ensures that + * we automatically restore backed up json files in load() and + * ensureDataReady() when original files are missing or corrupt. + * @param {Function} [config.saveFailureHandler] + * A synchronous function that will be called if saving the data object ever + * causes an exception to be thrown (and toJSONSafe is not implemented on + * data). */ export function JSONFile(config) { this.path = config.path; @@ -115,6 +121,10 @@ export function JSONFile(config) { this._options.backupFile = this._options.backupTo = config.backupTo; } + if (config.saveFailureHandler) { + this._saveFailureHandler = config.saveFailureHandler; + } + this._finalizeAt = config.finalizeAt || IOUtils.profileBeforeChange; this._finalizeInternalBound = this._finalizeInternal.bind(this); this._finalizeAt.addBlocker( @@ -145,6 +155,12 @@ JSONFile.prototype = { */ _saver: null, + /** + * A function that will be called if saving the data results in an exception + * being thrown. + */ + _saveFailureHandler: () => {}, + /** * Internal data object. */ @@ -421,6 +437,20 @@ JSONFile.prototype = { this._data.toJSONSafe(), Object.assign({ tmpPath: this.path + ".tmp" }, this._options) ); + } else { + // Something went wrong with saving that we cannot recover from. If + // the consumer of JSONFile has supplied a save failure handler, we'll + // at least let them know. + try { + this._saveFailureHandler(ex); + } catch (saveFailureEx) { + console.error( + "Failed to handle ", + ex, + " in save failure handler due to ", + saveFailureEx + ); + } } } }, diff --git a/toolkit/modules/tests/xpcshell/test_JSONFile.js b/toolkit/modules/tests/xpcshell/test_JSONFile.js index cb1375196349..584ae1dee6a3 100644 --- a/toolkit/modules/tests/xpcshell/test_JSONFile.js +++ b/toolkit/modules/tests/xpcshell/test_JSONFile.js @@ -9,6 +9,7 @@ ChromeUtils.defineESModuleGetters(this, { AsyncShutdown: "resource://gre/modules/AsyncShutdown.sys.mjs", FileTestUtils: "resource://testing-common/FileTestUtils.sys.mjs", JSONFile: "resource://gre/modules/JSONFile.sys.mjs", + sinon: "resource://testing-common/Sinon.sys.mjs", }); /** @@ -324,3 +325,32 @@ add_task(async function test_finalize_on_shutdown() { await storeForLoad.load(); Assert.deepEqual(storeForLoad.data, TEST_DATA); }); + +add_task(async function test_save_failure_handler() { + let path = getTempFile(TEST_STORE_FILE_NAME).path; + let saveFailureHandler = sinon.stub(); + let storeForSave = new JSONFile({ + path, + saveDelayMs: 10, + saveFailureHandler, + }); + await storeForSave.load(); + + // Let's now add some invalid data that we'll fail to save. + let circularThing = {}; + circularThing.circle = circularThing; + + storeForSave.data = { + circularThing, + }; + await storeForSave._save(); + Assert.ok(saveFailureHandler.calledOnce, "Failure handler was called"); + Assert.ok( + saveFailureHandler.calledWith( + sinon.match(error => { + return error.message == "cyclic object value"; + }) + ), + "Handler was passed Error" + ); +});