Bug 1634127 - Download attachments in IndexedDB by default r=necko-reviewers,robwu,dragana

This patch changes the default behaviour of `download()`.

- Previous file-based behaviour was moved to `downloadToDisk()` and `deleteFromDisk()`. Existing consumers were migrated to avoid behaviour change.
- `download()` has now `{useCache: true}` by default, option was dropped, and `deleteCached()` is now `deleteDownloaded()`

Differential Revision: https://phabricator.services.mozilla.com/D141980

browser/components/newtab/lib/ASRouter.jsm

@@ -303,7 +303,7 @@ const MessageLoaderUtils = {
               "newtab"
             );
             // Await here in order to capture the exceptions for reporting.
-            await downloader.download(record.data, {
+            await downloader.downloadToDisk(record.data, {
               retries: RS_DOWNLOAD_MAX_RETRIES,
             });
             RemoteL10n.reloadL10n();

browser/components/urlbar/UrlbarQuickSuggest.jsm

@@ -410,7 +410,12 @@ class QuickSuggest extends EventEmitter {
         await Promise.all(
           event.data.deleted
             .filter(d => d.attachment)
-            .map(entry => this._rs.attachments.delete(entry))
+            .map(entry =>
+              Promise.all([
+                this._rs.attachments.deleteDownloaded(entry), // type: data
+                this._rs.attachments.deleteFromDisk(entry), // type: icon
+              ])
+            )
         );
       }
 
@@ -420,7 +425,7 @@ class QuickSuggest extends EventEmitter {
         this._rs
           .get({ filters: { type: "icon" } })
           .then(icons =>
-            Promise.all(icons.map(i => this._rs.attachments.download(i)))
+            Promise.all(icons.map(i => this._rs.attachments.downloadToDisk(i)))
           ),
       ]);
 
@@ -430,9 +435,7 @@ class QuickSuggest extends EventEmitter {
       this._resultsByKeyword.clear();
 
       for (let record of data) {
-        let { buffer } = await this._rs.attachments.download(record, {
-          useCache: true,
-        });
+        let { buffer } = await this._rs.attachments.download(record);
         let results = JSON.parse(new TextDecoder("utf-8").decode(buffer));
         this._addResults(results);
       }
@@ -482,7 +485,7 @@ class QuickSuggest extends EventEmitter {
     if (!record) {
       return null;
     }
-    return this._rs.attachments.download(record);
+    return this._rs.attachments.downloadToDisk(record);
   }
 }
 

netwerk/dns/PublicSuffixList.jsm

@@ -33,7 +33,9 @@ const PublicSuffixList = {
       .then(async records => {
         if (records.length == 1) {
           // Get the downloaded file URI (most likely to be a no-op here, since file will exist).
-          const fileURI = await this.CLIENT.attachments.download(records[0]);
+          const fileURI = await this.CLIENT.attachments.downloadToDisk(
+            records[0]
+          );
           // Send a signal so that the C++ code loads the updated list on startup.
           this.notifyUpdate(fileURI);
         }
@@ -80,7 +82,7 @@ const PublicSuffixList = {
   async onUpdate({ data: { created, updated, deleted } }) {
     // In theory, this will never happen, we will never delete the record.
     if (deleted.length == 1) {
-      await this.CLIENT.attachments.delete(deleted[0]);
+      await this.CLIENT.attachments.deleteFromDisk(deleted[0]);
     }
     // Handle creation and update the same way
     const changed = created.concat(updated.map(u => u.new));
@@ -94,7 +96,7 @@ const PublicSuffixList = {
     // Download the updated file.
     let fileURI;
     try {
-      fileURI = await this.CLIENT.attachments.download(changed[0]);
+      fileURI = await this.CLIENT.attachments.downloadToDisk(changed[0]);
     } catch (err) {
       Cu.reportError(err);
       return;

netwerk/dns/tests/unit/test_PublicSuffixList.js

@@ -62,26 +62,26 @@ registerCleanupFunction(() => {
 });
 
 /**
- * downloadCalled is used by mockDownload() and resetMockDownload()
+ * downloadToDiskCalled is used by mockDownloadToDisk() and resetMockDownloadToDisk()
  * to keep track weather CLIENT.attachments.download is called or not
- * downloadBackup will help restore CLIENT.attachments.download to original definition
+ * downloadToDiskBackup will help restore CLIENT.attachments.download to original definition
  * notifyUpdateBackup will help restore PublicSuffixList.notifyUpdate to original definition
  */
-let downloadCalled = false;
-const downloadBackup = CLIENT.attachments.download;
+let downloadToDiskCalled = false;
+const downloadToDiskBackup = CLIENT.attachments.downloadToDisk;
 
 // returns a fake fileURI and sends a signal with filePath and no nsifile
-const mockDownload = () => {
-  downloadCalled = false;
-  CLIENT.attachments.download = async rec => {
-    downloadCalled = true;
+function mockDownloadToDisk() {
+  downloadToDiskCalled = false;
+  CLIENT.attachments.downloadToDisk = async rec => {
+    downloadToDiskCalled = true;
     return `file://${mockedFilePath}`; // Create a fake file URI
   };
-};
+}
 
-// resetMockDownload() must be run at the end of the test that uses mockDownload()
-const resetMockDownload = () => {
-  CLIENT.attachments.download = downloadBackup;
+// resetMockDownloadToDisk() must be run at the end of the test that uses mockDownloadToDisk()
+const resetMockDownloadToDisk = () => {
+  CLIENT.attachments.downloadToDisk = downloadToDiskBackup;
 };
 
 add_task(async () => {
@@ -94,7 +94,7 @@ add_task(async () => {
     attachment: {},
   });
 
-  mockDownload();
+  mockDownloadToDisk();
 
   const promiseSignal = TestUtils.topicObserved(SIGNAL);
   await PublicSuffixList.init();
@@ -106,13 +106,13 @@ add_task(async () => {
     "File path sent when record is in DB."
   );
   await CLIENT.db.clear(); // Clean up the mockDownloaded record
-  resetMockDownload();
+  resetMockDownloadToDisk();
 });
 
 add_task(async () => {
   info("File path sent when record updated.");
 
-  mockDownload();
+  mockDownloadToDisk();
 
   const promiseSignal = TestUtils.topicObserved(SIGNAL);
   await PublicSuffixList.init();
@@ -124,53 +124,53 @@ add_task(async () => {
     mockedFilePath,
     "File path sent when record updated."
   );
-  resetMockDownload();
+  resetMockDownloadToDisk();
 });
 
 add_task(async () => {
   info("Attachment downloaded when record created.");
 
-  mockDownload();
+  mockDownloadToDisk();
 
   await PublicSuffixList.init();
   await CLIENT.emit("sync", { data: PAYLOAD_CREATED_RECORDS });
 
   Assert.equal(
-    downloadCalled,
+    downloadToDiskCalled,
     true,
     "Attachment downloaded when record created."
   );
-  resetMockDownload();
+  resetMockDownloadToDisk();
 });
 
 add_task(async () => {
   info("Attachment downloaded when record updated.");
 
-  mockDownload();
+  mockDownloadToDisk();
 
   await PublicSuffixList.init();
   await CLIENT.emit("sync", { data: PAYLOAD_UPDATED_RECORDS });
 
   Assert.equal(
-    downloadCalled,
+    downloadToDiskCalled,
     true,
     "Attachment downloaded when record updated."
   );
-  resetMockDownload();
+  resetMockDownloadToDisk();
 });
 
 add_task(async () => {
   info("No download when more than one record is changed.");
 
-  mockDownload();
+  mockDownloadToDisk();
 
   await PublicSuffixList.init();
   await CLIENT.emit("sync", { data: PAYLOAD_UPDATED_AND_CREATED_RECORDS });
 
   Assert.equal(
-    downloadCalled,
+    downloadToDiskCalled,
     false,
     "No download when more than one record is changed."
   );
-  resetMockDownload();
+  resetMockDownloadToDisk();
 });

security/manager/ssl/RemoteSecuritySettings.jsm

@@ -610,7 +610,7 @@ class CRLiteFilters {
     for (let filter of filtersToDownload) {
       try {
         // If we've already downloaded this, the backend should just grab it from its cache.
-        let localURI = await this.client.attachments.download(filter);
+        let localURI = await this.client.attachments.downloadToDisk(filter);
         let buffer = await (await fetch(localURI)).arrayBuffer();
         let bytes = new Uint8Array(buffer);
         log.debug(`Downloaded ${filter.details.name}: ${bytes.length} bytes`);

services/settings/Attachments.jsm

@@ -116,50 +116,37 @@ class Downloader {
    *                        If omitted, the attachmentId option must be set.
    * @param {Object} options Some download options.
    * @param {Number} options.retries Number of times download should be retried (default: `3`)
-   * @param {Number} options.checkHash Check content integrity (default: `true`)
+   * @param {Boolean} options.checkHash Check content integrity (default: `true`)
    * @param {string} options.attachmentId The attachment identifier to use for
    *                                      caching and accessing the attachment.
-   *                                      (default: record.id)
-   * @param {Boolean} options.useCache Whether to use a cache to read and store
-   *                                   the attachment. (default: false)
+   *                                      (default: `record.id`)
    * @param {Boolean} options.fallbackToCache Return the cached attachment when the
    *                                          input record cannot be fetched.
-   *                                          (default: false)
+   *                                          (default: `false`)
    * @param {Boolean} options.fallbackToDump Use the remote settings dump as a
    *                                         potential source of the attachment.
-   *                                         (default: false)
+   *                                         (default: `false`)
    * @throws {Downloader.DownloadError} if the file could not be fetched.
    * @throws {Downloader.BadContentError} if the downloaded content integrity is not valid.
    * @returns {Object} An object with two properties:
-   *   buffer: ArrayBuffer with the file content.
-   *   record: Record associated with the bytes.
-   *   _source: identifies the source of the result. Used for testing.
+   *   `buffer` `ArrayBuffer`: the file content.
+   *   `record` `Object`: record associated with the attachment.
+   *   `_source` `String`: identifies the source of the result. Used for testing.
    */
   async download(record, options) {
     let {
       retries,
       checkHash,
       attachmentId = record?.id,
-      useCache = false,
       fallbackToCache = false,
       fallbackToDump = false,
     } = options || {};
-
-    if (!useCache) {
-      // For backwards compatibility.
-      // WARNING: Its return type is different from what's documented.
-      // See downloadToDisk's documentation.
-      return this.downloadToDisk(record, options);
-    }
-
-    if (!this.cacheImpl) {
-      throw new Error("useCache is true but there is no cacheImpl!");
-    }
-
     if (!attachmentId) {
       // Check for pre-condition. This should not happen, but it is explicitly
       // checked to avoid mixing up attachments, which could be dangerous.
-      throw new Error("download() was called without attachmentId or recordID");
+      throw new Error(
+        "download() was called without attachmentId or `record.id`"
+      );
     }
 
     const dumpInfo = new LazyRecordAndBuffer(() =>
@@ -184,7 +171,7 @@ class Downloader {
     }
 
     // Check if the requested attachment has already been cached.
-    if (useCache && record) {
+    if (record) {
       if (await cacheInfo.isMatchingRequestedRecord(record)) {
         try {
           return { ...(await cacheInfo.getResult()), _source: "cache_match" };
@@ -206,12 +193,10 @@ class Downloader {
           checkHash,
         });
         const blob = new Blob([newBuffer]);
-        if (useCache) {
-          // Caching is optional, don't wait for the cache before returning.
-          this.cacheImpl
-            .set(attachmentId, { record, blob })
-            .catch(e => Cu.reportError(e));
-        }
+        // Store in cache but don't wait for it before returning.
+        this.cacheImpl
+          .set(attachmentId, { record, blob })
+          .catch(e => Cu.reportError(e));
         return { buffer: newBuffer, record, _source: "remote_match" };
       } catch (e) {
         // No network, corrupted content, etc.
@@ -263,6 +248,30 @@ class Downloader {
   }
 
   /**
+   * Delete the record attachment downloaded locally.
+   * No-op if the attachment does not exist.
+   *
+   * @param record A Remote Settings entry with attachment.
+   * @param {Object} options Some options.
+   * @param {string} options.attachmentId The attachment identifier to use for
+   *                                      accessing and deleting the attachment.
+   *                                      (default: `record.id`)
+   */
+  async deleteDownloaded(record, options) {
+    let { attachmentId = record?.id } = options || {};
+    if (!attachmentId) {
+      // Check for pre-condition. This should not happen, but it is explicitly
+      // checked to avoid mixing up attachments, which could be dangerous.
+      throw new Error(
+        "deleteDownloaded() was called without attachmentId or `record.id`"
+      );
+    }
+    return this.cacheImpl.delete(attachmentId);
+  }
+
+  /**
+   * @deprecated See https://bugzilla.mozilla.org/show_bug.cgi?id=1634127
+   *
    * Download the record attachment into the local profile directory
    * and return a file:// URL that points to the local path.
    *
@@ -308,7 +317,7 @@ class Downloader {
           checkHash: false, // Hash will be checked on file.
           retries: 0, // Already in a retry loop.
         });
-        await OS.File.writeAtomic(localFilePath, buffer, {
+        await IOUtils.write(localFilePath, new Uint8Array(buffer), {
           tmpPath: `${localFilePath}.tmp`,
         });
       } catch (e) {
@@ -326,7 +335,7 @@ class Downloader {
    * @param {Object} record A Remote Settings entry with attachment.
    * @param {Object} options Some download options.
    * @param {Number} options.retries Number of times download should be retried (default: `3`)
-   * @param {Number} options.checkHash Check content integrity (default: `true`)
+   * @param {Boolean} options.checkHash Check content integrity (default: `true`)
    * @throws {Downloader.DownloadError} if the file could not be fetched.
    * @throws {Downloader.BadContentError} if the downloaded content integrity is not valid.
    * @returns {ArrayBuffer} the file content.
@@ -361,12 +370,18 @@ class Downloader {
   }
 
   /**
+   * @deprecated See https://bugzilla.mozilla.org/show_bug.cgi?id=1634127
+   *
    * Delete the record attachment downloaded locally.
+   * This is the counterpart of `downloadToDisk()`.
+   * Use `deleteDownloaded()` if `download()` was used to retrieve
+   * the attachment.
+   *
    * No-op if the related file does not exist.
    *
    * @param record A Remote Settings entry with attachment.
    */
-  async delete(record) {
+  async deleteFromDisk(record) {
     const {
       attachment: { filename },
     } = record;
@@ -375,14 +390,10 @@ class Downloader {
       ...this.folders,
       filename
     );
-    await OS.File.remove(path, { ignoreAbsent: true });
+    await IOUtils.remove(path);
     await this._rmDirs();
   }
 
-  async deleteCached(attachmentId) {
-    return this.cacheImpl.delete(attachmentId);
-  }
-
   async _baseAttachmentsURL() {
     if (!this._cdnURL) {
       const server = Utils.SERVER_URL;
@@ -457,7 +468,7 @@ class Downloader {
       OS.Constants.Path.localProfileDir,
       ...this.folders
     );
-    await OS.File.makeDir(dirPath, { from: OS.Constants.Path.localProfileDir });
+    await IOUtils.makeDirectory(dirPath, { createAncestors: true });
   }
 
   async _rmDirs() {
@@ -467,7 +478,7 @@ class Downloader {
         ...this.folders.slice(0, i)
       );
       try {
-        await OS.File.removeEmptyDir(dirPath, { ignoreAbsent: true });
+        await IOUtils.remove(dirPath);
       } catch (e) {
         // This could fail if there's something in
         // the folder we're not permitted to remove.

services/settings/RemoteSettingsClient.jsm

@@ -216,7 +216,11 @@ class AttachmentDownloader extends Downloader {
   async deleteAll() {
     let allRecords = await this._client.db.list();
     return Promise.all(
-      allRecords.filter(r => !!r.attachment).map(r => this.delete(r))
+      allRecords
+        .filter(r => !!r.attachment)
+        .map(r =>
+          Promise.all([this.deleteDownloaded(r), this.deleteFromDisk(r)])
+        )
     );
   }
 }

services/settings/docs/index.rst

@@ -117,27 +117,25 @@ Remote files are not downloaded automatically. In order to keep attachments in s
         .filter(d => d.attachment);
 
       // Remove local files of deleted records
-      await Promise.all(toDelete.map(entry => client.attachments.delete(entry)));
-      // Download attachments
-      const fileURLs = await Promise.all(
-        toDownload.map(entry => client.attachments.download(entry, { retries: 2 }))
+      await Promise.all(
+        toDelete.map(record => client.attachments.deleteDownloaded(record))
       );
 
-      // Open downloaded files...
+      // Download new attachments
       const fileContents = await Promise.all(
-        fileURLs.map(async url => {
-          const r = await fetch(url);
-          return r.blob();
-        })
+        toDownload.map(async record => {
+          const { buffer } = await client.attachments.download(record);
+          return buffer;
+        });
       );
     });
 
 The provided helper will:
   - fetch the remote binary content
-  - write the file in the profile folder
+  - write the file in the local IndexedDB
   - check the file size
   - check the content SHA256 hash
-  - do nothing if the file is already present and sound locally.
+  - do nothing if the attachment was already present and sound locally.
 
 .. important::
 
@@ -149,27 +147,22 @@ The provided helper will:
 
 .. note::
 
-    The ``download()`` method does not return a file path but instead a ``file://`` URL which points to the locally-downloaded file.
-    This will allow us to package attachments as part of a Firefox release (see `Bug 1542177 <https://bugzilla.mozilla.org/show_bug.cgi?id=1542177>`_)
-    and return them to calling code as ``resource://`` from within a package archive.
+    The ``download()`` method supports the following options:
+
+    - ``retries`` (default: ``3``): number of retries on network errors
+    - ``fallbackToCache`` (default: ``false``): allows callers to fall back to the cached file and record, if the requested record's attachment fails to download.
+      This enables callers to always have a valid pair of attachment and record,
+      provided that the attachment has been retrieved at least once.
+    - ``fallbackToDump`` (default: ``false``): activates a fallback to a dump that has been
+      packaged with the client, when other ways to load the attachment have failed.
+      See :ref:`services/packaging-attachments <services/packaging-attachments>` for more information.
 
 .. note::
 
-    By default, the ``download()`` method is prone to leaving extraneous files in the profile directory
-    (see `Bug 1634127 <https://bugzilla.mozilla.org/show_bug.cgi?id=1634127>`_).
-    Pass the ``useCache`` option to use an IndexedDB-based cache, and unlock the following features:
+    A ``downloadAsBytes()`` method returning an ``ArrayBuffer`` is also available, if writing the attachment locally is not necessary.
 
-    The ``fallbackToCache`` option allows callers to fall back to the cached file and record, if the requested record's attachment fails to download.
-    This enables callers to always have a valid pair of attachment and record,
-    provided that the attachment has been retrieved at least once.
-
-    The ``fallbackToDump`` option activates a fallback to a dump that has been
-    packaged with the client, when other ways to load the attachment have failed.
-    See :ref:`services/packaging-attachments <services/packaging-attachments>` for more information.
-
-.. note::
-
-    A ``downloadAsBytes()`` method returning an ``ArrayBuffer`` is also available, if writing the attachment into the user profile is not necessary.
+    Some ``downloadToDisk()`` and ``deleteFromDisk()`` methods are also available but generally discouraged, since they are prone to leaving extraneous files
+    in the profile directory (see `Bug 1634127 <https://bugzilla.mozilla.org/show_bug.cgi?id=1634127>`_).
 
 
 .. _services/initial-data:

services/settings/test/unit/test_attachments_downloader.js

@@ -85,21 +85,34 @@ function run_test() {
 
 async function clear_state() {
   downloader = new Downloader("main", "some-collection");
-  await downloader.delete(RECORD);
+  const dummyCacheImpl = {
+    get: async attachmentId => {},
+    set: async (attachmentId, attachment) => {},
+    delete: async attachmentId => {},
+  };
+  // The download() method requires a cacheImpl, but the Downloader
+  // class does not have one. Define a dummy no-op one.
+  Object.defineProperty(downloader, "cacheImpl", {
+    value: dummyCacheImpl,
+    // Writable to allow specific tests to override cacheImpl.
+    writable: true,
+  });
+
+  await downloader.deleteDownloaded(RECORD);
 }
 
 add_task(clear_state);
 
 add_task(async function test_download_writes_file_in_profile() {
-  const fileURL = await downloader.download(RECORD);
+  const fileURL = await downloader.downloadToDisk(RECORD);
   const localFilePath = pathFromURL(fileURL);
 
   Assert.equal(
     fileURL,
     PROFILE_URL + "/settings/main/some-collection/test_file.pem"
   );
-  Assert.ok(await OS.File.exists(localFilePath));
-  const stat = await OS.File.stat(localFilePath);
+  Assert.ok(await IOUtils.exists(localFilePath));
+  const stat = await IOUtils.stat(localFilePath);
   Assert.equal(stat.size, 1597);
 });
 add_task(clear_state);
@@ -113,39 +126,38 @@ add_task(async function test_download_as_bytes() {
 add_task(clear_state);
 
 add_task(async function test_file_is_redownloaded_if_size_does_not_match() {
-  const fileURL = await downloader.download(RECORD);
+  const fileURL = await downloader.downloadToDisk(RECORD);
   const localFilePath = pathFromURL(fileURL);
-  await OS.File.writeAtomic(localFilePath, "bad-content", {
-    encoding: "utf-8",
-  });
-  let stat = await OS.File.stat(localFilePath);
+  await IOUtils.writeUTF8(localFilePath, "bad-content");
+  let stat = await IOUtils.stat(localFilePath);
   Assert.notEqual(stat.size, 1597);
 
-  await downloader.download(RECORD);
+  await downloader.downloadToDisk(RECORD);
 
-  stat = await OS.File.stat(localFilePath);
+  stat = await IOUtils.stat(localFilePath);
   Assert.equal(stat.size, 1597);
 });
 add_task(clear_state);
 
 add_task(async function test_file_is_redownloaded_if_corrupted() {
-  const fileURL = await downloader.download(RECORD);
+  const fileURL = await downloader.downloadToDisk(RECORD);
   const localFilePath = pathFromURL(fileURL);
-  const byteArray = await OS.File.read(localFilePath);
+  const byteArray = await IOUtils.read(localFilePath);
   byteArray[0] = 42;
-  await OS.File.writeAtomic(localFilePath, byteArray);
-  let content = await OS.File.read(localFilePath, { encoding: "utf-8" });
+  await IOUtils.write(localFilePath, byteArray);
+  let content = await IOUtils.readUTF8(localFilePath);
   Assert.notEqual(content.slice(0, 5), "-----");
 
-  await downloader.download(RECORD);
+  await downloader.downloadToDisk(RECORD);
 
-  content = await OS.File.read(localFilePath, { encoding: "utf-8" });
+  content = await IOUtils.readUTF8(localFilePath);
   Assert.equal(content.slice(0, 5), "-----");
 });
 add_task(clear_state);
 
 add_task(async function test_download_is_retried_3_times_if_download_fails() {
   const record = {
+    id: "abc",
     attachment: {
       ...RECORD.attachment,
       location: "404-error.pem",
@@ -173,6 +185,7 @@ add_task(clear_state);
 
 add_task(async function test_download_is_retried_3_times_if_content_fails() {
   const record = {
+    id: "abc",
     attachment: {
       ...RECORD.attachment,
       hash: "always-wrong",
@@ -197,38 +210,41 @@ add_task(async function test_download_is_retried_3_times_if_content_fails() {
 add_task(clear_state);
 
 add_task(async function test_delete_removes_local_file() {
-  const fileURL = await downloader.download(RECORD);
+  const fileURL = await downloader.downloadToDisk(RECORD);
   const localFilePath = pathFromURL(fileURL);
-  Assert.ok(await OS.File.exists(localFilePath));
+  Assert.ok(await IOUtils.exists(localFilePath));
 
-  downloader.delete(RECORD);
+  await downloader.deleteFromDisk(RECORD);
 
-  Assert.ok(!(await OS.File.exists(localFilePath)));
-  Assert.ok(
-    !(await OS.File.exists(
-      OS.Path.join(OS.Constants.Path.localProfileDir, ...downloader.folders)
-    ))
+  Assert.ok(!(await IOUtils.exists(localFilePath)));
+  // And removes parent folders.
+  const parentFolder = OS.Path.join(
+    OS.Constants.Path.localProfileDir,
+    ...downloader.folders
   );
+  Assert.ok(!(await IOUtils.exists(parentFolder)));
 });
 add_task(clear_state);
 
 add_task(async function test_delete_all() {
   const client = RemoteSettings("some-collection");
   await client.db.create(RECORD);
-  const fileURL = await downloader.download(RECORD);
+  await downloader.download(RECORD);
+  const fileURL = await downloader.downloadToDisk(RECORD);
   const localFilePath = pathFromURL(fileURL);
-  Assert.ok(await OS.File.exists(localFilePath));
+  Assert.ok(await IOUtils.exists(localFilePath));
 
   await client.attachments.deleteAll();
 
-  Assert.ok(!(await OS.File.exists(localFilePath)));
+  Assert.ok(!(await IOUtils.exists(localFilePath)));
+  Assert.ok(!(await client.attachments.cacheImpl.get(RECORD.id)));
 });
 add_task(clear_state);
 
 add_task(async function test_downloader_is_accessible_via_client() {
   const client = RemoteSettings("some-collection");
 
-  const fileURL = await client.attachments.download(RECORD);
+  const fileURL = await client.attachments.downloadToDisk(RECORD);
 
   Assert.equal(
     fileURL,
@@ -332,9 +348,7 @@ async function doTestDownloadCacheImpl({ simulateCorruption }) {
   };
   Object.defineProperty(downloader, "cacheImpl", { value: cacheImpl });
 
-  let downloadResult = await downloader.download(RECORD, {
-    useCache: true,
-  });
+  let downloadResult = await downloader.download(RECORD);
   Assert.equal(downloadResult._source, "remote_match", "expected source");
   Assert.equal(downloadResult.buffer.byteLength, 1597, "expected result");
   Assert.equal(readCount, 1, "expected cache read attempts");
@@ -430,7 +444,7 @@ add_task(async function test_download_cached() {
   checkInfo(info5, "cache_fallback", "fallbackToCache ignores bad record");
 
   // Bye bye cache.
-  await client.attachments.deleteCached(attachmentId);
+  await client.attachments.deleteDownloaded({ id: attachmentId });
   await Assert.rejects(
     downloadWithCache(null, { attachmentId, fallbackToCache: true }),
     /DownloadError: Could not download dummy filename/,
@@ -473,7 +487,6 @@ add_task(async function test_download_from_dump() {
   // If record matches, should happen before network request.
   const dump1 = await client.attachments.download(RECORD_OF_DUMP, {
     // Note: attachmentId not set, so should fall back to record.id.
-    useCache: true,
     fallbackToDump: true,
   });
   checkInfo(dump1, "dump_match");
@@ -481,7 +494,6 @@ add_task(async function test_download_from_dump() {
   // If no record given, should try network first, but then fall back to dump.
   const dump2 = await client.attachments.download(null, {
     attachmentId: RECORD_OF_DUMP.id,
-    useCache: true,
     fallbackToDump: true,
   });
   checkInfo(dump2, "dump_fallback");
@@ -493,7 +505,6 @@ add_task(async function test_download_from_dump() {
   });
   // The dump should take precedence over the cache.
   const dump3 = await client.attachments.download(RECORD_OF_DUMP, {
-    useCache: true,
     fallbackToCache: true,
     fallbackToDump: true,
   });
@@ -503,7 +514,6 @@ add_task(async function test_download_from_dump() {
   // as a fallback (when the cache and dump are identical).
   const dump4 = await client.attachments.download(null, {
     attachmentId: RECORD_OF_DUMP.id,
-    useCache: true,
     fallbackToCache: true,
     fallbackToDump: true,
   });
@@ -522,7 +532,6 @@ add_task(async function test_download_from_dump() {
   // When the record is not given, use the cache if it has a more recent record.
   const dump5 = await client.attachments.download(null, {
     attachmentId: RECORD_OF_DUMP.id,
-    useCache: true,
     fallbackToCache: true,
     fallbackToDump: true,
   });
@@ -530,24 +539,21 @@ add_task(async function test_download_from_dump() {
 
   // When a record is given, use whichever that has the matching last_modified.
   const dump6 = await client.attachments.download(RECORD_OF_DUMP, {
-    useCache: true,
     fallbackToCache: true,
     fallbackToDump: true,
   });
   checkInfo(dump6, "dump_match");
   const dump7 = await client.attachments.download(RECORD_NEWER_THAN_DUMP, {
-    useCache: true,
     fallbackToCache: true,
     fallbackToDump: true,
   });
   checkInfo(dump7, "cache_match", RECORD_NEWER_THAN_DUMP);
 
-  await client.attachments.deleteCached(RECORD_OF_DUMP.id);
+  await client.attachments.deleteDownloaded(RECORD_OF_DUMP);
 
   await Assert.rejects(
     client.attachments.download(null, {
       attachmentId: "filename-without-meta.txt",
-      useCache: true,
       fallbackToDump: true,
     }),
     /DownloadError: Could not download filename-without-meta.txt/,
@@ -557,7 +563,6 @@ add_task(async function test_download_from_dump() {
   await Assert.rejects(
     client.attachments.download(null, {
       attachmentId: "filename-without-content.txt",
-      useCache: true,
       fallbackToDump: true,
     }),
     /Could not download resource:\/\/rs-downloader-test\/settings\/dump-bucket\/dump-collection\/filename-without-content\.txt(?!\.meta\.json)/,

toolkit/modules/Region.jsm

@@ -446,7 +446,7 @@ class RegionDetector {
     const toDelete = deleted.filter(d => d.attachment);
     // Remove local files of deleted records
     await Promise.all(
-      toDelete.map(entry => this._rsClient.attachments.delete(entry))
+      toDelete.map(entry => this._rsClient.attachments.deleteDownloaded(entry))
     );
     await this._ensureRegionFilesDownloaded();
   }
@@ -464,10 +464,7 @@ class RegionDetector {
       log.info("_ensureRegionFilesDownloaded: Nothing to download");
       return;
     }
-    let opts = { useCache: true };
-    await Promise.all(
-      records.map(r => this._rsClient.attachments.download(r, opts))
-    );
+    await Promise.all(records.map(r => this._rsClient.attachments.download(r)));
     log.info("_ensureRegionFilesDownloaded complete");
     this._regionFilesReady = true;
   }
@@ -480,9 +477,7 @@ class RegionDetector {
    */
   async _fetchAttachment(id) {
     let record = (await this._rsClient.get({ filters: { id } })).pop();
-    let { buffer } = await this._rsClient.attachments.download(record, {
-      useCache: true,
-    });
+    let { buffer } = await this._rsClient.attachments.download(record);
     let text = new TextDecoder("utf-8").decode(buffer);
     return JSON.parse(text);
   }

toolkit/mozapps/extensions/Blocklist.jsm

@@ -976,7 +976,6 @@ const ExtensionBlocklistMLBF = {
       _source: rsAttachmentSource,
     } = await this._client.attachments.download(record, {
       attachmentId: this.RS_ATTACHMENT_ID,
-      useCache: true,
       fallbackToCache: true,
       fallbackToDump: true,
     });