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');
+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];
-  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 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}`);
+
+    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...");
-
-    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);
+    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", 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.

+              .on("error", error =>
+              {
+                writable.close();
+                fs.unlinkSync(tempDest);
                 reject(error);
               })
-              .on("close", () => {
+              .on("close", () =>
+              {
                 writable.close();
-                unzipArchive(destArchive, destDir)
-                  .then(dir => {
-                    resolve(getChromiumExecutable(dir));
-                  })
-                  .catch(reject);
+                fs.renameSync(tempDest, destFile);
+                resolve();
               });
     }).on("error", reject);
   });
 }
 
 function unzipArchive(archive, destDir)
 {
   return new Promise((resolve, reject) =>
   {
-    let unzipper = new DecompressZip(archive);
-    unzipper.on('extract', () => {
-      resolve(destDir);
+    extractZip(archive, {dir: destDir}, err =>
+    {
+      if (err)
+        reject(err);
+      else
+        resolve();
     });
-    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 {
@@ skipping 132 matching lines @@
     {
       child.kill();
       return result;
     }).catch(error =>
     {
       child.kill();
       throw error;
     });
   });
 };