Merge pull request #6222 from Rob--W/botio-robustness

Improve robustness of builder (esp. on Windows)

package.json

@@ -7,6 +7,7 @@
     "wintersmith": "~2.0.0",
     "moment": "~2.3.0",
     "underscore": "~1.4.0",
+    "rimraf": "^2.4.1",
     "shelljs": "0.4.0",
     "typogr": "~0.5.0",
     "yargs": "~1.2.1"

test/testutils.js

@@ -21,19 +21,13 @@
 
 var fs = require('fs');
 var path = require('path');
+var rimrafSync = require('rimraf').sync;
 
 exports.removeDirSync = function removeDirSync(dir) {
-  var files = fs.readdirSync(dir);
+  fs.readdirSync(dir); // Will throw if dir is not a directory
-  files.forEach(function (filename) {
+  rimrafSync(dir, {
-    var file = path.join(dir, filename);
+    disableGlob: true,
-    var stats = fs.statSync(file);
-    if (stats.isDirectory()) {
-      removeDirSync(file);
-    } else {
-      fs.unlinkSync(file);
-    }
   });
-  fs.rmdirSync(dir);
 };
 
 exports.copySubtreeSync = function copySubtreeSync(src, dest) {

test/webbrowser.js

@@ -25,6 +25,7 @@ var path = require('path');
 var spawn = require('child_process').spawn;
 var testUtils = require('./testutils.js');
 var shelljs = require('shelljs');
+var crypto = require('crypto');
 
 var tempDirPrefix = 'pdfjs_';
 
@@ -34,8 +35,12 @@ function WebBrowser(name, path) {
   this.tmpDir = null;
   this.profileDir = null;
   this.process = null;
+  this.requestedExit = false;
   this.finished = false;
   this.callback = null;
+  // Used to identify processes whose pid is lost. This string is directly used
+  // as a command-line argument, so it only consists of letters.
+  this.uniqStringId = 'webbrowser' + crypto.randomBytes(32).toString('hex');
 }
 WebBrowser.prototype = {
   start: function (url) {
@@ -43,7 +48,7 @@ WebBrowser.prototype = {
     if (!fs.existsSync(this.tmpDir)) {
       fs.mkdirSync(this.tmpDir);
     }
-    this.process = this.startProcess(url);
+    this.startProcess(url);
   },
   getProfileDir: function () {
     if (!this.profileDir) {
@@ -63,54 +68,128 @@ WebBrowser.prototype = {
   setupProfileDir: function (dir) {
   },
   startProcess: function (url) {
+    console.assert(!this.process, 'startProcess may be called only once');
+
     var args = this.buildArguments(url);
-    var proc = spawn(this.path, args);
+    args = args.concat('--' + this.uniqStringId);
-    proc.on('exit', function (code) {
+    this.process = spawn(this.path, args);
-      this.finished = true;
+    this.process.on('exit', function (code, signal) {
-      this.cleanup(this.callback && this.callback.bind(null, code));
+      this.process = null;
+      var exitInfo = code !== null ? ' with status ' + code :
+        ' in response to signal ' + signal;
+      if (this.requestedExit) {
+        this.log('Browser process exited' + exitInfo);
+      } else {
+        // This was observed on Windows bots with Firefox. Apparently the
+        // Firefox Maintenance Service restarts Firefox shortly after starting
+        // up. When this happens, we no longer know the pid of the process.
+        this.log('Browser process unexpectedly exited' + exitInfo);
+      }
     }.bind(this));
-    return proc;
   },
-  cleanup: function (callback) {
+  cleanup: function () {
+    console.assert(this.requestedExit,
+        'cleanup should only be called after an explicit stop() request');
+
     try {
       testUtils.removeDirSync(this.tmpDir);
-      this.process = null;
-      if (callback) {
-        callback();
-      }
     } catch (e) {
-      console.error('Unable to cleanup after the process: ' + e);
+      if (e.code !== 'ENOENT') {
-      try {
+        this.log('Failed to remove profile directory: ' + e);
-        if (this.process) {
+        if (!this.cleanupFailStart) {
-          this.process.kill('SIGKILL');
+          this.cleanupFailStart = Date.now();
+        } else if (Date.now() - this.cleanupFailStart > 10000) {
+          throw new Error('Failed to remove profile dir within 10 seconds');
         }
-      } catch (e) {}
+        this.log('Retrying in a second...');
+        setTimeout(this.cleanup.bind(this), 1000);
+        return;
+      }
+      // This should not happen, but we just warn instead of failing loudly
+      // because the post-condition of cleanup is that the profile directory is
+      // gone. If the directory does not exists, then this post-condition is
+      // satisfied.
+      this.log('Cannot remove non-existent directory: ' + e);
     }
+    this.finished = true;
+    this.log('Clean-up finished. Going to call callback...');
+    this.callback();
   },
   stop: function (callback) {
-    if (this.finished) {
+    console.assert(this.tmpDir, '.start() must be called before stop()');
-      if (callback) {
+    // Require the callback to ensure that callers do not make any assumptions
-        callback();
+    // on the state of this browser instance until the callback is called.
-      }
+    console.assert(typeof callback === 'function', 'callback is required');
-    } else {
+    console.assert(!this.requestedExit, '.stop() may be called only once');
-      this.callback = callback;
-    }
 
-    if (this.process) {
+    this.requestedExit = true;
-      if (process.platform === 'win32') {
+    if (this.finished) {
-        // process.kill is not reliable on Windows (Firefox sticks around). The
+      this.log('Browser already stopped, invoking callback...');
-        // work-around was to manually open the task manager and terminate the
+      callback();
-        // process. Instead of doing that manually, use taskkill to kill.
+    } else if (this.process) {
-        // (https://technet.microsoft.com/en-us/library/cc725602.aspx)
+      this.log('Going to wait until the browser process has exited.');
-        var result = shelljs.exec('taskkill /f /t /pid ' + this.process.pid);
+      this.callback = callback;
-        if (result.code) {
+      this.process.once('exit', this.cleanup.bind(this));
-          console.error('taskkill failed with exit code ' + result.code);
+      this.process.kill('SIGTERM');
-        }
+    } else {
-      } else {
+      this.log('Process already exited, checking if the process restarted...');
-        this.process.kill('SIGTERM');
+      this.callback = callback;
-      }
+      this.killProcessUnknownPid(this.cleanup.bind(this));
     }
-  }
+  },
+  killProcessUnknownPid: function(callback) {
+    this.log('pid unknown, killing processes matching ' + this.uniqStringId);
+
+    var cmdKillAll, cmdCheckAllKilled, isAllKilled;
+
+    if (process.platform === 'win32') {
+      var wmicPrefix = 'wmic process where "not Name = \'cmd.exe\' ' +
+        'and not Name like \'%wmic%\' ' +
+        'and CommandLine like \'%' + this.uniqStringId + '%\'" ';
+      cmdKillAll = wmicPrefix + 'call terminate';
+      cmdCheckAllKilled = wmicPrefix + 'get CommandLine';
+      isAllKilled = function(exitCode, stdout) {
+        return stdout.indexOf(this.uniqStringId) === -1;
+      }.bind(this);
+    } else {
+      cmdKillAll = 'pkill -f ' + this.uniqStringId;
+      cmdCheckAllKilled = 'pgrep -f ' + this.uniqStringId;
+      isAllKilled = function(pgrepStatus) {
+        return pgrepStatus === 1; // "No process matched.", per man pgrep.
+      };
+    }
+    function execAsyncNoStdin(cmd, onExit) {
+      var proc = shelljs.exec(cmd, {
+        async: true,
+        silent: true,
+      }, onExit);
+      // Close stdin, otherwise wmic won't run.
+      proc.stdin.end();
+    }
+    var killDateStart = Date.now();
+    // Note: First process' output it shown, the later outputs are suppressed.
+    execAsyncNoStdin(cmdKillAll, function checkAlive(exitCode, firstStdout) {
+      execAsyncNoStdin(cmdCheckAllKilled, function(exitCode, stdout) {
+        if (isAllKilled(exitCode, stdout)) {
+          callback();
+        } else if (Date.now() - killDateStart > 10000) {
+          // Should finish termination within 10 (generous) seconds.
+          if (firstStdout) {
+            this.log('Output of first command:\n' + firstStdout);
+          }
+          if (stdout) {
+            this.log('Output of last command:\n' + stdout);
+          }
+          throw new Error('Failed to kill process of ' + this.name);
+        } else {
+          setTimeout(checkAlive.bind(this), 500);
+        }
+      }.bind(this));
+    }.bind(this));
+  },
+  log: function(msg) {
+    console.log('[' + this.name + '] ' + msg);
+  },
 };
 
 var firefoxResourceDir = path.join(__dirname, 'resources', 'firefox');