Issue 5230 - Properly download Chromium on macOS

chromium_process.js

Index: chromium_process.js
===================================================================
--- a/chromium_process.js
+++ b/chromium_process.js
@@ -22,17 +22,17 @@
 
 const childProcess = require("child_process");
 const fs = require("fs");
 const https = require("https");
 const os = require("os");
 const path = require("path");
 
 const remoteInterface = require("chrome-remote-interface");
-const unzip = require("unzip");
+const DecompressZip = require('decompress-zip');
 
 const CHROMIUM_REVISION = 467222;
 
 function rmdir(dirPath)
 {
   for (let file of fs.readdirSync(dirPath))
   {
     let filePath = path.join(dirPath, file);
@@ -88,44 +88,92 @@
   };
 
   if (!buildTypes.hasOwnProperty(platform))
   {
     let err = new Error(`Cannot run browser tests, ${platform} is unsupported`);
     return Promise.reject(err);
   }
 
+  let chromiumDownloadCache = path.join(__dirname, "chromium-snapshots",
+                                        "download-cache");
   let chromiumDir = path.join(__dirname, "chromium-snapshots",
                               `chromium-${platform}-${CHROMIUM_REVISION}`);
   if (fs.existsSync(chromiumDir))
     return Promise.resolve(getChromiumExecutable(chromiumDir));
 
   if (!fs.existsSync(path.dirname(chromiumDir)))
     fs.mkdirSync(path.dirname(chromiumDir));
+
+  let [dir, fileName] = buildTypes[platform];
+  let archive = path.join(chromiumDownloadCache,
+                          `${CHROMIUM_REVISION}-${fileName}`);
+  if (fs.existsSync(archive))
+  {
+    console.info(`Reusing cached archive ${archive}`);
+    return unzipArchive(archive, chromiumDir)
+      .then(dir => {
+        return Promise.resolve(getChromiumExecutable(dir));
+      });

[Wladimir Palant, 2017/05/17 10:37:02]

This is equivalent to:

  .then(dir => getChromiumExecutable(dir));

[hub, 2017/05/17 17:25:02]

Done.

+  }
+
+  let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
+
+  return downloadCacheAndExtract(url, chromiumDownloadCache, archive, chromiumDir);

[Wladimir Palant, 2017/05/17 10:37:02]

The point of using promises is making the logic obvious instead of splitting the
flow into dozens of callbacks. While we have multiple functions here, this is
merely compensating for the fact that Node doesn't provide promise-based APIs -
we have promise-based wrappers for the core APIs. So we should really have a
simple download helper here, without any assumptions about what is being
downloaded:

  return download(url, archive)
    .then(() => unzipArchive(archive, chromiumDir))
    .then(() => getChromiumExecutable(chromiumDir));

You can also avoid duplicating the logic for the cache hit and download
scenarios:

  let retrieveArchive;
  if (fs.existsSync(archive))
    retrieveArchive = Promise.resove();
  else
    retrieveArchive = download(url, archive);
  return retrieveArchive
    .then(...);

[hub, 2017/05/17 17:25:02]

Acknowledged.

+}
+
+function downloadCacheAndExtract(url, downloadCache, destArchive, destDir)
+{
   return new Promise((resolve, reject) =>
   {
     console.info("Downloading Chromium...");
-    let [dir, fileName] = buildTypes[platform];
-    let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
+
+    if (!fs.existsSync(downloadCache))
+      fs.mkdirSync(downloadCache);

[Wladimir Palant, 2017/05/17 10:37:02]

You don't need the downloadCache parameter, just use path.dirname(targetPath).

[hub, 2017/05/17 17:25:02]

Acknowledged.

+    let writable = fs.createWriteStream(destArchive);
+
     https.get(url, response =>
     {
       if (response.statusCode != 200)
       {
         reject(new Error(`Unexpected server response: ${response.statusCode}`));
         response.resume();
         return;
       }
 
-      response.pipe(unzip.Extract({path: chromiumDir}))
-              .on("error", reject)
-              .on("close", () => resolve(getChromiumExecutable(chromiumDir)));
+      response.pipe(writable)
+              .on("error", error => {
+                fs.unlinkSync(destArchive);

[Wladimir Palant, 2017/05/17 10:37:02]

Will unlink work on Windows if the stream is still open?

I don't think that unlinking is sufficient. Your process might crash and then
the cleanup will never run - the result would be a broken installation that
cannot be fixed any more without removing files manually. As I said, we should
download to a file with a different name first and only rename on success - a
rename is an atomic operation as long as the file stays in the same directory.

[hub, 2017/05/17 17:25:02]

From what I can read it will work and the file will really be deleted when it is
closed (immediatly if it is not open anymore).

Will fix including the above.

[Wladimir Palant, 2017/05/18 11:18:27]

Depends on the lock type I think. With an exclusive lock (which is what we have
here) unlinking will actually fail IMHO. We should definitely close this stream
on error, and be it merely for correctness.

[hub, 2017/05/18 13:22:37]

I'll add a "writeable.close()" first, just to make sure.

+                reject(error);
+              })
+              .on("close", () => {
+                writable.close();
+                unzipArchive(destArchive, destDir)
+                  .then(dir => {
+                    resolve(getChromiumExecutable(dir));
+                  })
+                  .catch(reject);
+              });
     }).on("error", reject);
   });
 }
 
+function unzipArchive(archive, destDir)
+{
+  return new Promise((resolve, reject) =>
+  {
+    let unzipper = new DecompressZip(archive);
+    unzipper.on('extract', () => {
+      resolve(destDir);
+    });
+    unzipper.on('error', reject);
+    unzipper.extract({path: destDir});
+  });
+}
+
 function startChromium(chromiumPath)
 {
   fs.chmodSync(chromiumPath, fs.constants.S_IRWXU);
 
   let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data"));
   let child = null;
   return {
     kill: () => child && child.kill(),