update chromedriver support with callback-based functions that fit more snugly into the current paradigm

lib/devices/android/android-hybrid.js

@@ -16,7 +16,6 @@ var logger = require('../../server/logger.js').get('appium')
 var androidHybrid = {};
 
 androidHybrid.chromedriver = null;
-androidHybrid.chromedriverStopCbs = {};
 androidHybrid.sessionChromedrivers = {};
 
 androidHybrid.listWebviews = function (cb) {
@@ -134,7 +133,7 @@ androidHybrid.startChromedriverProxy = function (context, cb) {
   }
 };
 
-androidHybrid.setupNewChromedriver = function (context, ocb) {
+androidHybrid.setupNewChromedriver = function (context, cb) {
   var chromeArgs = {
     port: this.args.chromeDriverPort,
     executable: this.args.chromedriverExecutable
@@ -167,21 +166,19 @@ androidHybrid.setupNewChromedriver = function (context, ocb) {
     });
   }
   caps = this.decorateChromeOptions(caps);
-  // see note in chrome.js::createSession for explanation of this pattern
-  this.chromedriver.once(Chromedriver.EVENT_ERROR, ocb);
   this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) {
-    if (msg.state === Chromedriver.STATE_ONLINE) {
-      // save the chromedriver object under the context
-      this.sessionChromedrivers[context] = this.chromedriver;
-      // let whoever called us know that we're done setting up session
-      ocb();
-    } else if (msg.state === Chromedriver.STATE_STOPPED) {
+    if (msg.state === Chromedriver.STATE_STOPPED) {
       // bind our stop/exit handler, passing in context so we know which
-      // one stopped
+      // one stopped unexpectedly
       this.onChromedriverStop(context);
     }
   }.bind(this));
-  this.chromedriver.start(caps);
+  this.chromedriver.start(caps).nodeify(function (err) {
+    if (err) return cb(err);
+    // save the chromedriver object under the context
+    this.sessionChromedrivers[context] = this.chromedriver;
+    cb();
+  }.bind(this));
 };
 
 
@@ -193,42 +190,25 @@ androidHybrid.setupExistingChromedriver = function (context, cb) {
   this.proxyReqRes = this.chromedriver.proxyReq.bind(this.chromedriver);
   this.isProxy = true;
 
-  // make sure that no matter how many times Chromedriver emits a changed
-  // state event, we only call back on the first ONLINE message
-  var restartCb = _.once(function () {
-    // once we're back online, let upstream know
-    this.chromedriverRestartingContext = null;
-    cb();
-  }.bind(this));
-
   // check the status by sending a simple window-based command to ChromeDriver
   // if there is an error, we want to recreate the ChromeDriver session
-  this.chromedriver.hasWorkingWebview().then(function (works) {
+  this.chromedriver.hasWorkingWebview().nodeify(function (err, works) {
+    if (err) return cb(err);
     if (works) return cb();
     logger.debug("ChromeDriver is not associated with a window. " +
                  "Re-initializing the session.");
-    // catch any errors the restart process bubbles up
-    this.chromedriver.once(Chromedriver.EVENT_ERROR, cb);
-    // once the restart is successful, reset context flag and continue
-    this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) {
-      if (msg.state === Chromedriver.STATE_ONLINE) {
-        restartCb();
-      }
-    }.bind(this));
     this.chromedriverRestartingContext = context;
-    this.chromedriver.restart();
-  }.bind(this)).catch(cb);
+    this.chromedriver.restart().nodeify(function (err) {
+      if (err) return cb(err);
+      this.chromedriverRestartingContext = null;
+      cb();
+    }.bind(this));
+  }.bind(this));
 };
 
 androidHybrid.onChromedriverStop = function (context) {
-  logger.debug("Chromedriver for context " + context + " stopped");
-  // chromedriver isn't valid anymore, so remove it from context list
-  if (_.has(this.chromedriverStopCbs, context)) {
-    // if we intentionally stopped this context's chromedriver, we'll have a
-    // callback for it in this.chromedriverStopCbs
-    delete this.sessionChromedrivers[context];
-    this.chromedriverStopCbs[context]();
-  } else if (context === this.curContext) {
+  logger.warn("Chromedriver for context " + context + " stopped unexpectedly");
+  if (context === this.curContext) {
     // if we don't have a stop callback, we exited unexpectedly and so want
     // to shut down the session and respond with an error
     // TODO: this kind of thing should be emitted and handled by a higher-level
@@ -260,10 +240,14 @@ androidHybrid.suspendChromedriverProxy = function (cb) {
 androidHybrid.stopChromedriverProxies = function (ocb) {
   async.eachSeries(Object.keys(this.sessionChromedrivers), function (context, cb) {
     logger.debug("Stopping chromedriver for context " + context);
-    // add a stop cb for this context so we get called back once this context's
-    // chromedriver finishes exiting
-    this.chromedriverStopCbs[context] = _.once(cb);
-    this.sessionChromedrivers[context].stop();
+    // stop listening for the stopped state event
+    this.sessionChromedrivers[context].removeAllListeners(Chromedriver.EVENT_CHANGED);
+    this.sessionChromedrivers[context].stop().nodeify(function (err) {
+      if (err) logger.warn("Error stopping Chromedriver: " + err.message);
+      // chromedriver isn't valid anymore, so remove it from context list
+      delete this.sessionChromedrivers[context];
+      cb();
+    }.bind(this));
   }.bind(this), function (err) {
     // if one of these fails, go back to last proxy state and error out
     this.restoreProxyState();

lib/devices/android/chrome.js

@@ -24,7 +24,7 @@ ChromeAndroid.prototype._androidInit = Android.prototype.init;
 ChromeAndroid.prototype.init = function () {
   this._androidInit();
   this.adb = null;
-  this.stopCb = null;
+  this.onDieCb = null;
   this.setChromedriverMode();
 };
 
@@ -84,7 +84,7 @@ ChromeAndroid.prototype.start = function (cb, onDie) {
   this.adb = new ADB(this.args);
   this.uiautomator = new UiAutomator(this.adb, this.args);
   this.uiautomator.setExitHandler(this.onUiautomatorExit.bind(this));
-  this.stopCb = onDie;
+  this.onDieCb = onDie;
 
   async.series([
     this.initAdb.bind(this),
@@ -124,7 +124,6 @@ ChromeAndroid.prototype.pushAndUnlock = function (cb) {
 };
 
 ChromeAndroid.prototype.createSession = function (cb) {
-  cb = _.once(cb);
   var caps = {
     chromeOptions: {
       androidPackage: this.args.appPackage
@@ -143,24 +142,27 @@ ChromeAndroid.prototype.createSession = function (cb) {
   }
 
   caps = this.decorateChromeOptions(caps);
-  this.chromedriver.once(Chromedriver.EVENT_ERROR, cb);
   this.chromedriver.on(Chromedriver.EVENT_CHANGED, function (msg) {
-    if (msg.state === Chromedriver.STATE_ONLINE) {
-      cb();
-    } else if (msg.state === Chromedriver.STATE_STOPPED) {
-      this.onChromedriverStop();
+    if (msg.state === Chromedriver.STATE_STOPPED) {
+      logger.info("Chromedriver stopped unexpectedly on us, shutting down " +
+                  "then calling back up with the on-die callback");
+      this.onChromedriverStop(this.onDieCb);
     }
   }.bind(this));
-  this.chromedriver.start(caps);
+  this.chromedriver.start(caps).nodeify(cb);
 };
 
 ChromeAndroid.prototype.stop = function (cb) {
-  this.stopCb = cb; // change stopCb from original onDie to this cb
-  this.chromedriver.stop();
+  // stop listening for the stopped state event
+  this.chromedriver.removeAllListeners(Chromedriver.EVENT_CHANGED);
+  // now we can handle the stop on our own
+  this.chromedriver.stop().nodeify(function (err) {
+    if (err) logger.warn("Error stopping Chromedriver: " + err.message);
+    this.onChromedriverStop(cb);
+  }.bind(this));
 };
 
-ChromeAndroid.prototype.onChromedriverStop = function () {
-  var cb = this.stopCb;
+ChromeAndroid.prototype.onChromedriverStop = function (cb) {
   if (this.adb) {
     this.uiautomator.shutdown(function () {
       this.adb.forceStop(this.args.appPackage, function (err) {

lib/server/proxy.js

@@ -42,10 +42,16 @@ module.exports.doProxy = function (req, res) {
   if (req.device.proxyReqRes) {
     // this section is triggered when we have defined the proxy device to use
     // the appium-jsonwp-proxy method proxyReqRes. Ultimately we'll be moving
-    // everything to this paradigm and will delete the code after this block
-    req.device.proxyReqRes(req, res).catch(function (err) {
+    // everything to this paradigm and will delete the code after this block.
+    // proxyReqRes might be a promise or a callback-based function, so handle
+    // both
+    var handler = function (err) {
       logger.error(err.message);
-    });
+    };
+    var p = req.device.proxyReqRes(req, res, handler);
+    if (p.catch) {
+      p.catch(handler);
+    }
     return;
   }
 

package.json

@@ -43,7 +43,7 @@
     "adm-zip": "~0.4.7",
     "appium-adb": "=1.7.5",
     "appium-atoms": "=0.0.5",
-    "appium-chromedriver": "=0.2.0",
+    "appium-chromedriver": "=0.2.2",
     "appium-instruments": "=1.5.4",
     "appium-support": "=0.1.1",
     "appium-uiauto": "=1.10.7",