Issue 5230 - Properly download Chromium on macOS

chromium_process.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* eslint-env node */
 /* eslint no-console: "off" */
 
 "use strict";
 
 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 DecompressZip = require('decompress-zip');

[hub, 2017/05/10 14:15:27]

I had to use a different module that actually work on macOS. Also I don't stream
so one can cache the archive.

[Wladimir Palant, 2017/05/16 14:21:15]

If we don't need streaming support then we should go with a mature and widely
used module here. In my understanding, yauzl is the usual choice.

[hub, 2017/05/16 15:49:30]

yauzl doesn't have an easy "extract archive API". There is "extract-zip" for
that https://www.npmjs.com/package/extract-zip.

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

You are right. extract-zip sounds good.

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

Done.

+const extractZip = require("extract-zip");
 
 const CHROMIUM_REVISION = 467222;
 
 function rmdir(dirPath)
 {
   for (let file of fs.readdirSync(dirPath))
   {
     let filePath = path.join(dirPath, file);
     try
     {
@@ skipping 45 matching lines @@
     "linux": ["Linux_x64", "chrome-linux.zip"],
     "darwin": ["Mac", "chrome-mac.zip"]
   };
 
   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];
-  const archive = path.join(chromiumDownloadCache,

[Wladimir Palant, 2017/05/16 14:21:15]

I'm not a big fan of using `const` for things that aren't inherently constant
but merely aren't being changed by the current implementation.

[hub, 2017/05/16 15:49:30]

Done.

+
+  return Promise.resolve().then(() =>
+  {
+    let snapshotsDir = path.join(__dirname, "chromium-snapshots");
+    let chromiumDir = path.join(snapshotsDir,
+                                `chromium-${platform}-${CHROMIUM_REVISION}`);
+    if (fs.existsSync(chromiumDir))
+      return chromiumDir;
+
+    if (!fs.existsSync(path.dirname(chromiumDir)))
+      fs.mkdirSync(path.dirname(chromiumDir));
+
+    let [dir, fileName] = buildTypes[platform];
+    let archive = path.join(snapshotsDir, "download-cache",
                             `${CHROMIUM_REVISION}-${fileName}`);
-  if (fs.existsSync(archive))
-  {
-    console.info(`Reusing cached archive ${archive}`);
-    return unzipChromium(archive, chromiumDir);
-  }
+
+    return Promise.resolve()
+      .then(() =>
+      {
+        if (!fs.existsSync(archive))
+        {
+          let url = `https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${CHROMIUM_REVISION}%2F${fileName}?alt=media`;
+          console.info("Downloading Chromium...");
+          return download(url, archive);
+        }
+        console.info(`Reusing cached archive ${archive}`);
+      })
+      .then(() => unzipArchive(archive, chromiumDir))
+      .then(() => chromiumDir);
+  }).then(dir => getChromiumExecutable(dir));
+}
+
+function download(url, destFile)
+{
   return new Promise((resolve, reject) =>
   {
-    console.info("Downloading Chromium...");
-    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(chromiumDownloadCache))
-      fs.mkdirSync(chromiumDownloadCache);
-    const writable = fs.createWriteStream(archive);

[Wladimir Palant, 2017/05/16 14:21:15]

Same as above, I'd rather not have this marked as const.

[hub, 2017/05/16 15:49:30]

Done.

+    let cacheDir = path.dirname(destFile);
+    if (!fs.existsSync(cacheDir))
+      fs.mkdirSync(cacheDir);
+    let tempDest = destFile + "-" + process.pid;
+    let writable = fs.createWriteStream(tempDest);
 
     https.get(url, response =>
     {
       if (response.statusCode != 200)
       {
-        reject(new Error(`Unexpected server response: ${response.statusCode}`));
+        reject(
+          new Error(`Unexpected server response: ${response.statusCode}`));
         response.resume();
         return;
       }
 
       response.pipe(writable)
-              .on("error", reject )

[Wladimir Palant, 2017/05/16 14:21:15]

What happens with the write stream if an error occurs? Will we have some
half-written archive on disk that will prevent successful installations for all
eternity? We should definitely clean up here. However, we should also make sure
that we don't leave an invalid archive in the cache. Usually this means
downloading to a temporary file in the same directory and only renaming if the
download succeeds. We need to check response code as well now - this wasn't
important before but now the user might get stuck in an invalid state.

Nit: I'm pretty sure that ESLint will object against the extra space added here.
Please make sure to run it.

[hub, 2017/05/16 15:49:30]

Good idea let's unlink the file.

[hub, 2017/05/16 15:49:30]

If I run eslint on that file I get:
  229:46  error  Parsing error: Unexpected token client

it seems to not understand the async keyword. So it never catches the error
above.

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

You should update eslint-config-eyeo package, support for ECMAScript 2017
features has been enabled.

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

Acknowledged.

-              .on("close", () => {
+              .on("error", error =>
+              {
                 writable.close();
-                unzipChromium(archive, chromiumDir).catch(reject);

[Wladimir Palant, 2017/05/16 14:21:15]

Where is resolve being called here?

[hub, 2017/05/16 15:49:30]

Not sure what happens here because it works, but I'll make sure it called.

+                fs.unlinkSync(tempDest);
+                reject(error);
+              })
+              .on("close", () =>
+              {
+                writable.close();
+                fs.renameSync(tempDest, destFile);
+                resolve();
               });
     }).on("error", reject);

[Wladimir Palant, 2017/05/16 14:21:15]

Given how complicated the download logic gets, I think it would help to wrap it
in a generic helper returning a promise.

   });
 }
 
-function unzipChromium(archive, chromiumDir)

[Wladimir Palant, 2017/05/16 14:21:15]

Nit: this is a generic function, so name and parameters should be generic as
well. How about unzipArchive(archive, targetDir)?

[hub, 2017/05/16 15:49:30]

Done.

+function unzipArchive(archive, destDir)
 {
   return new Promise((resolve, reject) =>
   {
-    let unzipper = new DecompressZip(archive);
-    unzipper.on('extract', () => {
-      resolve(getChromiumExecutable(chromiumDir));

[Wladimir Palant, 2017/05/16 14:21:15]

The getChromiumExecutable() call should stay in ensureChromium(), the function
here should merely do what its name says - unzip, then resolve promise.

[hub, 2017/05/16 15:49:30]

I'll resolve the promise with the destination path and call ensureChromium()
above in the resolution of the unzipArchive

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

This doesn't make much sense either - the destination path is simply the
parameter that the caller passed in, it cannot change. I suggest resolving
without any value.

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

Done.

+    extractZip(archive, {dir: destDir}, err =>
+    {
+      if (err)
+        reject(err);
+      else
+        resolve();
     });
-    unzipper.on('error', reject);
-    unzipper.extract({path: chromiumDir});
   });
 }
 
 function startChromium(chromiumPath)
 {
   fs.chmodSync(chromiumPath, fs.constants.S_IRWXU);
 
   let dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "chromium-data"));
   let child = null;
   return {
@@ skipping 132 matching lines @@
     {
       child.kill();
       return result;
     }).catch(error =>
     {
       child.kill();
       throw error;
     });
   });
 };