Bug 1559132 - Add ability to download Remote Settings attachments as bytes r=glasserc
authorMathieu Leplatre <mathieu@mozilla.com>
Mon, 25 Nov 2019 16:13:52 +0000
changeset 503689 e37f837977c24c17a7598ba2de013cc789518b5c
parent 503688 a2e761251f7e5a283bb9e229ec93eeb051bba834
child 503690 a5f2363e47bad050af1a18dfc8de039b44f9deb1
push id101477
push usermleplatre@mozilla.com
push dateMon, 25 Nov 2019 16:42:20 +0000
treeherderautoland@e37f837977c2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglasserc
bugs1559132
milestone72.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1559132 - Add ability to download Remote Settings attachments as bytes r=glasserc Differential Revision: https://phabricator.services.mozilla.com/D53812
services/common/docs/RemoteSettings.rst
services/settings/Attachments.jsm
services/settings/RemoteSettingsWorker.js
services/settings/RemoteSettingsWorker.jsm
services/settings/test/unit/test_attachments_downloader.js
--- a/services/common/docs/RemoteSettings.rst
+++ b/services/common/docs/RemoteSettings.rst
@@ -128,34 +128,40 @@ Remote files are not downloaded automati
           const r = await fetch(url);
           return r.blob();
         })
       );
     });
 
 The provided helper will:
 - fetch the remote binary content
+- write the file in the profile folder
 - check the file size
 - check the content SHA256 hash
 - do nothing if the file is already present and sound locally.
 
 .. important::
 
     The following aspects are not taken care of (yet! help welcome):
 
+    - report Telemetry when download fails
     - check available disk space
     - preserve bandwidth
     - resume downloads of large files
 
 .. notes::
 
     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.
 
+.. notes::
+
+    A ``downloadAsBytes()`` method returning an ``ArrayBuffer`` is also available, if writing the attachment into the user profile is not necessary.
+
 
 .. _services/initial-data:
 
 Initial data
 ------------
 
 It is possible to package a dump of the server records that will be loaded into the local database when no synchronization has happened yet.
 
--- a/services/settings/Attachments.jsm
+++ b/services/settings/Attachments.jsm
@@ -55,43 +55,89 @@ class Downloader {
    * @param {Number} options.retries Number of times download should be retried (default: `3`)
    * @throws {Downloader.DownloadError} if the file could not be fetched.
    * @throws {Downloader.BadContentError} if the downloaded file integrity is not valid.
    * @returns {String} the absolute file path to the downloaded attachment.
    */
   async download(record, options = {}) {
     const { retries = 3 } = options;
     const {
-      attachment: { location, filename, hash, size },
+      attachment: { filename, size, hash },
     } = record;
     const localFilePath = OS.Path.join(
       OS.Constants.Path.localProfileDir,
       ...this.folders,
       filename
     );
     const localFileUrl = `file://${[
       ...OS.Path.split(OS.Constants.Path.localProfileDir).components,
       ...this.folders,
       filename,
     ].join("/")}`;
-    const remoteFileUrl = (await this._baseAttachmentsURL()) + location;
 
     await this._makeDirs();
 
     let retried = 0;
     while (true) {
       if (await RemoteSettingsWorker.checkFileHash(localFileUrl, size, hash)) {
         return localFileUrl;
       }
       // File does not exist or is corrupted.
       if (retried > retries) {
         throw new Downloader.BadContentError(localFilePath);
       }
       try {
-        await this._fetchAttachment(remoteFileUrl, localFilePath);
+        // Download and write on disk.
+        const buffer = await this.downloadAsBytes(record, {
+          checkHash: false, // Hash will be checked on file.
+          retries: 0, // Already in a retry loop.
+        });
+        await OS.File.writeAtomic(localFilePath, buffer, {
+          tmpPath: `${localFilePath}.tmp`,
+        });
+      } catch (e) {
+        if (retried >= retries) {
+          throw e;
+        }
+      }
+      retried++;
+    }
+  }
+
+  /**
+   * Download the record attachment and return its content as bytes.
+   *
+   * @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`)
+   * @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.
+   */
+  async downloadAsBytes(record, options = {}) {
+    const {
+      attachment: { location, hash, size },
+    } = record;
+
+    const remoteFileUrl = (await this._baseAttachmentsURL()) + location;
+
+    const { retries = 3, checkHash = true } = options;
+    let retried = 0;
+    while (true) {
+      try {
+        const buffer = await this._fetchAttachment(remoteFileUrl);
+        if (!checkHash) {
+          return buffer;
+        }
+        if (await RemoteSettingsWorker.checkContentHash(buffer, size, hash)) {
+          return buffer;
+        }
+        // Content is corrupted.
+        throw new Downloader.BadContentError(location);
       } catch (e) {
         if (retried >= retries) {
           throw e;
         }
       }
       retried++;
     }
   }
@@ -126,27 +172,24 @@ class Downloader {
         },
       } = serverInfo;
       // Make sure the URL always has a trailing slash.
       this._cdnURL = base_url + (base_url.endsWith("/") ? "" : "/");
     }
     return this._cdnURL;
   }
 
-  async _fetchAttachment(url, destination) {
+  async _fetchAttachment(url) {
     const headers = new Headers();
     headers.set("Accept-Encoding", "gzip");
     const resp = await fetch(url, { headers });
     if (!resp.ok) {
       throw new Downloader.DownloadError(url, resp);
     }
-    const buffer = await resp.arrayBuffer();
-    await OS.File.writeAtomic(destination, buffer, {
-      tmpPath: `${destination}.tmp`,
-    });
+    return resp.arrayBuffer();
   }
 
   async _makeDirs() {
     const dirPath = OS.Path.join(
       OS.Constants.Path.localProfileDir,
       ...this.folders
     );
     await OS.File.makeDir(dirPath, { from: OS.Constants.Path.localProfileDir });
--- a/services/settings/RemoteSettingsWorker.js
+++ b/services/settings/RemoteSettingsWorker.js
@@ -81,24 +81,35 @@ const Agent = {
   async checkFileHash(fileUrl, size, hash) {
     let resp;
     try {
       resp = await fetch(fileUrl);
     } catch (e) {
       // File does not exist.
       return false;
     }
+    const buffer = await resp.arrayBuffer();
+    const bytes = new Uint8Array(buffer);
+    return this.checkContentHash(bytes, size, hash);
+  },
+
+  /**
+   * Check that the specified content matches the expected size and SHA-256 hash.
+   * @param {ArrayBuffer} buffer binary content
+   * @param {Number} size expected file size
+   * @param {String} size expected file SHA-256 as hex string
+   * @returns {boolean}
+   */
+  async checkContentHash(buffer, size, hash) {
+    const bytes = new Uint8Array(buffer);
     // Has expected size? (saves computing hash)
-    const fileSize = parseInt(resp.headers.get("Content-Length"), 10);
-    if (fileSize !== size) {
+    if (bytes.length !== size) {
       return false;
     }
     // Has expected content?
-    const buffer = await resp.arrayBuffer();
-    const bytes = new Uint8Array(buffer);
     const hashBuffer = await crypto.subtle.digest("SHA-256", bytes);
     const hashBytes = new Uint8Array(hashBuffer);
     const toHex = b => b.toString(16).padStart(2, "0");
     const hashStr = Array.from(hashBytes, toHex).join("");
     return hashStr == hash;
   },
 };
 
--- a/services/settings/RemoteSettingsWorker.jsm
+++ b/services/settings/RemoteSettingsWorker.jsm
@@ -110,16 +110,20 @@ class Worker {
 
   async importJSONDump(bucket, collection) {
     return this._execute("importJSONDump", [bucket, collection]);
   }
 
   async checkFileHash(filepath, size, hash) {
     return this._execute("checkFileHash", [filepath, size, hash]);
   }
+
+  async checkContentHash(buffer, size, hash) {
+    return this._execute("checkContentHash", [buffer, size, hash]);
+  }
 }
 
 // Now, first add a shutdown blocker. If that fails, we must have
 // shut down already.
 // We're doing this here rather than in the Worker constructor because in
 // principle having just 1 shutdown blocker for the entire file should be
 // fine. If we ever start creating more than one Worker instance, this
 // code will need adjusting to deal with that.
--- a/services/settings/test/unit/test_attachments_downloader.js
+++ b/services/settings/test/unit/test_attachments_downloader.js
@@ -87,16 +87,24 @@ add_task(async function test_download_wr
     PROFILE_URL + "/settings/main/some-collection/test_file.pem"
   );
   Assert.ok(await OS.File.exists(localFilePath));
   const stat = await OS.File.stat(localFilePath);
   Assert.equal(stat.size, 1597);
 });
 add_task(clear_state);
 
+add_task(async function test_download_as_bytes() {
+  const bytes = await downloader.downloadAsBytes(RECORD);
+
+  // See *.pem file in tests data.
+  Assert.ok(bytes.byteLength > 1500, `Wrong bytes size: ${bytes.byteLength}`);
+});
+add_task(clear_state);
+
 add_task(async function test_file_is_redownloaded_if_size_does_not_match() {
   const fileURL = await downloader.download(RECORD);
   const localFilePath = pathFromURL(fileURL);
   await OS.File.writeAtomic(localFilePath, "bad-content", {
     encoding: "utf-8",
   });
   let stat = await OS.File.stat(localFilePath);
   Assert.notEqual(stat.size, 1597);
@@ -129,19 +137,19 @@ add_task(async function test_download_is
     attachment: {
       ...RECORD.attachment,
       location: "404-error.pem",
     },
   };
 
   let called = 0;
   const _fetchAttachment = downloader._fetchAttachment;
-  downloader._fetchAttachment = (url, destination) => {
+  downloader._fetchAttachment = async url => {
     called++;
-    return _fetchAttachment(url, destination);
+    return _fetchAttachment(url);
   };
 
   let error;
   try {
     await downloader.download(record);
   } catch (e) {
     error = e;
   }
@@ -154,17 +162,20 @@ add_task(clear_state);
 add_task(async function test_download_is_retried_3_times_if_content_fails() {
   const record = {
     attachment: {
       ...RECORD.attachment,
       hash: "always-wrong",
     },
   };
   let called = 0;
-  downloader._fetchAttachment = () => called++;
+  downloader._fetchAttachment = async () => {
+    called++;
+    return new ArrayBuffer();
+  };
 
   let error;
   try {
     await downloader.download(record);
   } catch (e) {
     error = e;
   }