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

@@ -56,35 +56,41 @@ const kSaveDelayMs = 1500;
 /**
  * Handles serialization of the data and persistence into a file.
  *
- * @param config An object containing following members:
+ * @param {Object} config An object containing following members:
- *        - path: String containing the file path where data should be saved.
+ * @param {string} config.path
- *        - sanitizedBasename: Sanitized string identifier used for logging,
+ *   String containing the file path where data should be saved.
- *                             shutdown debugging, and telemetry.  Defaults to
+ * @param {string} [config.sanitizedBasename]
- *                             basename of given `path`, sanitized.
+ *   Sanitized string identifier used for logging,
- *        - dataPostProcessor: Function triggered when data is just loaded. The
+ *   shutdown debugging, and telemetry. Defaults to basename of given `path`,
- *                             data object will be passed as the first argument
+ *   sanitized.
- *                             and should be returned no matter it's modified or
+ * @param {Function} [config.dataPostProcessor]
- *                             not. Its failure leads to the failure of load()
+ *   Function triggered when data is just loaded. The data object will be passed
- *                             and ensureDataReady().
+ *   as the first argument and should be returned no matter if it's modified or
- *        - saveDelayMs: Number indicating the delay (in milliseconds) between a
+ *   not. Its failure leads to the failure of load() and ensureDataReady().
- *                       change to the data and the related save operation. The
+ * @param {number} [config.saveDelayMs]
- *                       default value will be applied if omitted.
+ *   Number indicating the delay (in milliseconds) between a change to the data
- *        - beforeSave: Promise-returning function triggered just before the
+ *   and the related save operation. The default value will be applied if
- *                      data is written to disk. This can be used to create any
+ *   omitted.
- *                      intermediate directories before saving. The file will
+ * @param {Function} [config.beforeSave]
- *                      not be saved if the promise rejects or the function
+ *   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
+ * @param {nsIAsyncShutdownClient} [config.finalizeAt]
- *                      automatically finalize the file when triggered. Defaults
+ *   An `IOUtils` phase or barrier client that should automatically finalize the
- *                      to `profileBeforeChange`; exposed as an option for
+ *   file when triggered. Defaults to `profileBeforeChange`; exposed as an
- *                      testing.
+ *   option for testing.
- *        - compression: A compression algorithm to use when reading and
+ * @param {string} [config.compression]
- *                       writing the data.
+ *   A compression algorithm to use when reading and writing the data.
- *        - backupTo: A string value indicating where writeAtomic should create
+ * @param {string} [config.backupTo]
- *                    a backup before writing to json files. Note that using this
+ *   A string value indicating where writeAtomic should create a backup before
- *                    option currently ensures that we automatically restore backed
+ *   writing to json files. Note that using this option currently ensures that
- *                    up json files in load() and ensureDataReady() when original
+ *   we automatically restore backed up json files in load() and
- *                    files are missing or corrupt.
+ *   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
+          );
+        }
       }
     }
   },

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"
+  );
+});