Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. r=ato
authorHenrik Skupin <mail@hskupin.info>
Mon, 17 Oct 2016 13:19:19 +0200
changeset 318213 46457bc48303a885934f7ba1daa903cf591fdb9c
parent 318212 6fc7fc30b5c5f093b60095b0da5317d38771c5d4
child 318214 5c0780a3a6cb086946c029404ead160f866010e0
push id33218
push userhskupin@mozilla.com
push dateMon, 17 Oct 2016 15:34:03 +0000
treeherderautoland@46457bc48303 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1309556
milestone52.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. r=ato By using a callback the usual shutdown logic from quitApplication() is not executed and as such will create a race-condition for the client when trying to re-connect to the server. To fix that we have to stop the server from accepting new connections until the application has been completely shutdown. Also delete_session() has to be called for the default in_app shutdown logic and when using a callback. MozReview-Commit-ID: GmIM2GGwQ2P
testing/marionette/client/marionette_driver/marionette.py
testing/marionette/driver.js
testing/marionette/harness/marionette/tests/unit/test_quit_restart.py
testing/marionette/server.js
--- a/testing/marionette/client/marionette_driver/marionette.py
+++ b/testing/marionette/client/marionette_driver/marionette.py
@@ -1099,17 +1099,16 @@ class Marionette(object):
                                  createInstance(Components.interfaces.nsISupportsPRBool);
                 Services.obs.notifyObservers(cancelQuit, "quit-application-requested", null);
                 return cancelQuit.data;
                 """)
             if canceled:
                 raise errors.MarionetteException("Something canceled the quit application request")
 
         self._send_message("quitApplication", {"flags": list(flags)})
-        self.delete_session(in_app=True)
 
     @do_process_check
     def quit(self, in_app=False, callback=None):
         """Terminate the currently running instance.
 
         This command will delete the active marionette session. It also allows
         manipulation of eg. the profile data while the application is not running.
         To start the application again, start_session() has to be called.
@@ -1123,19 +1122,21 @@ class Marionette(object):
         if not self.instance:
             raise errors.MarionetteException("quit() can only be called "
                                              "on Gecko instances launched by Marionette")
 
         self.reset_timeouts()
 
         if in_app:
             if callable(callback):
+                self._send_message("acceptConnections", {"value": False})
                 callback()
             else:
                 self._request_in_app_shutdown()
+            self.delete_session(in_app=True)
 
             # Give the application some time to shutdown
             self.instance.runner.wait(timeout=self.DEFAULT_SHUTDOWN_TIMEOUT)
         else:
             self.delete_session()
             self.instance.close()
 
     @do_process_check
@@ -1158,19 +1159,21 @@ class Marionette(object):
                                              "on Gecko instances launched by Marionette")
         session_id = self.session_id
 
         if in_app:
             if clean:
                 raise ValueError("An in_app restart cannot be triggered with the clean flag set")
 
             if callable(callback):
+                self._send_message("acceptConnections", {"value": False})
                 callback()
             else:
                 self._request_in_app_shutdown("eRestart")
+            self.delete_session(in_app=True)
 
             try:
                 self.raise_for_port()
             except socket.timeout:
                 if self.instance.runner.returncode is not None:
                     exc, val, tb = sys.exc_info()
                     self.cleanup()
                     raise exc, "Requested restart of the application was aborted", tb
--- a/testing/marionette/driver.js
+++ b/testing/marionette/driver.js
@@ -86,22 +86,22 @@ this.Context.fromString = function(s) {
  * browsing context's content frame message listener via ListenerProxy.
  *
  * Throughout this prototype, functions with the argument {@code cmd}'s
  * documentation refers to the contents of the {@code cmd.parameters}
  * object.
  *
  * @param {string} appName
  *     Description of the product, for example "B2G" or "Firefox".
- * @param {function()} stopSignal
- *     Signal to stop the Marionette server.
+ * @param {MarionetteServer} server
+ *     The instance of Marionette server.
  */
-this.GeckoDriver = function(appName, stopSignal) {
+this.GeckoDriver = function(appName, server) {
   this.appName = appName;
-  this.stopSignal_ = stopSignal;
+  this._server = server;
 
   this.sessionId = null;
   this.wins = new browser.Windows();
   this.browsers = {};
   // points to current browser
   this.curBrowser = null;
   this.context = Context.CONTENT;
   this.scriptTimeout = 30000;  // 30 seconds
@@ -2555,30 +2555,54 @@ GeckoDriver.prototype.sendKeysToDialog =
 GeckoDriver.prototype._checkIfAlertIsPresent = function() {
   if (!this.dialog || !this.dialog.ui) {
     throw new NoAlertOpenError(
         "No tab modal was open when attempting to get the dialog text");
   }
 };
 
 /**
+ * Enables or disables accepting new socket connections.
+ *
+ * By calling this method with `false` the server will not accept any further
+ * connections, but existing connections will not be forcible closed. Use `true`
+ * to re-enable accepting connections.
+ *
+ * Please note that when closing the connection via the client you can end-up in
+ * a non-recoverable state if it hasn't been enabled before.
+ *
+ * This method is used for custom in application shutdowns via marionette.quit()
+ * or marionette.restart(), like File -> Quit.
+ *
+ * @param {boolean} state
+ *     True if the server should accept new socket connections.
+ */
+GeckoDriver.prototype.acceptConnections = function(cmd, resp) {
+  if (typeof cmd.parameters.value != "boolean") {
+    throw InvalidArgumentError("Value has to be of type 'boolean'");
+  }
+
+  this._server.acceptConnections = cmd.parameters.value;
+}
+
+/**
  * Quits Firefox with the provided flags and tears down the current
  * session.
  */
 GeckoDriver.prototype.quitApplication = function(cmd, resp) {
   if (this.appName != "Firefox") {
     throw new WebDriverError("In app initiated quit only supported in Firefox");
   }
 
   let flags = Ci.nsIAppStartup.eAttemptQuit;
   for (let k of cmd.parameters.flags || []) {
     flags |= Ci.nsIAppStartup[k];
   }
 
-  this.stopSignal_();
+  this._server.acceptConnections = false;
   resp.send();
 
   this.sessionTearDown();
   Services.startup.quit(flags);
 };
 
 /**
  * Helper function to convert an outerWindowID into a UID that Marionette
@@ -2771,10 +2795,11 @@ GeckoDriver.prototype.commands = {
   "setScreenOrientation": GeckoDriver.prototype.setScreenOrientation,
   "getWindowSize": GeckoDriver.prototype.getWindowSize,
   "setWindowSize": GeckoDriver.prototype.setWindowSize,
   "maximizeWindow": GeckoDriver.prototype.maximizeWindow,
   "dismissDialog": GeckoDriver.prototype.dismissDialog,
   "acceptDialog": GeckoDriver.prototype.acceptDialog,
   "getTextFromDialog": GeckoDriver.prototype.getTextFromDialog,
   "sendKeysToDialog": GeckoDriver.prototype.sendKeysToDialog,
+  "acceptConnections": GeckoDriver.prototype.acceptConnections,
   "quitApplication": GeckoDriver.prototype.quitApplication,
 };
--- a/testing/marionette/harness/marionette/tests/unit/test_quit_restart.py
+++ b/testing/marionette/harness/marionette/tests/unit/test_quit_restart.py
@@ -60,19 +60,18 @@ class TestQuitRestart(MarionetteTestCase
             self.assertEqual(self.marionette.session["processId"], self.pid)
         else:
             self.assertNotEqual(self.marionette.session["processId"], self.pid)
 
         # If a preference value is not forced, a restart will cause a reset
         self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
 
     def test_in_app_restart_with_callback(self):
-        def callback():
-            self.marionette._request_in_app_shutdown(shutdown_flags='eRestart')
-        self.marionette.restart(in_app=True, callback=callback)
+        self.marionette.restart(in_app=True,
+                                callback=lambda: self.shutdown(restart=True))
 
         self.assertEqual(self.marionette.session_id, self.session_id)
 
         # An in-app restart will keep the same process id only on Linux
         if self.marionette.session_capabilities['platformName'] == 'linux':
             self.assertEqual(self.marionette.session["processId"], self.pid)
         else:
             self.assertNotEqual(self.marionette.session["processId"], self.pid)
@@ -87,17 +86,27 @@ class TestQuitRestart(MarionetteTestCase
         with self.assertRaisesRegexp(MarionetteException, "Please start a session"):
             self.marionette.get_url()
 
         self.marionette.start_session()
         self.assertNotEqual(self.marionette.session_id, self.session_id)
         self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
 
     def test_in_app_quit_with_callback(self):
-        self.marionette.quit(in_app=True,
-                             callback=self.marionette._request_in_app_shutdown)
+        self.marionette.quit(in_app=True, callback=self.shutdown)
         self.assertEqual(self.marionette.session, None)
         with self.assertRaisesRegexp(MarionetteException, "Please start a session"):
             self.marionette.get_url()
 
         self.marionette.start_session()
         self.assertNotEqual(self.marionette.session_id, self.session_id)
         self.assertNotEqual(self.marionette.get_pref("browser.startup.page"), 3)
+
+    def shutdown(self, restart=False):
+        self.marionette.set_context("chrome")
+        self.marionette.execute_script("""
+            Components.utils.import("resource://gre/modules/Services.jsm");
+            let flags = Ci.nsIAppStartup.eAttemptQuit
+            if(arguments[0]) {
+              flags |= Ci.nsIAppStartup.eRestart;
+            }
+            Services.startup.quit(flags);
+        """, script_args=[restart])
--- a/testing/marionette/server.js
+++ b/testing/marionette/server.js
@@ -42,16 +42,17 @@ const MANAGE_OFFLINE_STATUS_PREF = "netw
  *     accept all connections.
  */
 this.MarionetteServer = function(port, forceLocal) {
   this.port = port;
   this.forceLocal = forceLocal;
   this.conns = {};
   this.nextConnId = 0;
   this.alive = false;
+  this._acceptConnections = false;
 };
 
 /**
  * Function produces a GeckoDriver.
  *
  * Determines application nameto initialise the driver with.
  *
  * @return {GeckoDriver}
@@ -65,48 +66,64 @@ MarionetteServer.prototype.driverFactory
 
   if (bypassOffline) {
       logger.debug("Bypassing offline status");
       Preferences.set(MANAGE_OFFLINE_STATUS_PREF, false);
       Services.io.manageOfflineStatus = false;
       Services.io.offline = false;
   }
 
-  let stopSignal = () => this.stop();
-  return new GeckoDriver(appName, stopSignal);
+  return new GeckoDriver(appName, this);
 };
 
+MarionetteServer.prototype.__defineSetter__("acceptConnections", function(value) {
+  if (!value) {
+    logger.info("New connections will no longer be accepted");
+  } else {
+    logger.info("New connections are accepted again");
+  }
+
+  this._acceptConnections = value;
+});
+
 MarionetteServer.prototype.start = function() {
   if (this.alive) {
     return;
   }
   let flags = Ci.nsIServerSocket.KeepWhenOffline;
   if (this.forceLocal) {
     flags |= Ci.nsIServerSocket.LoopbackOnly;
   }
   this.listener = new ServerSocket(this.port, flags, 1);
   this.listener.asyncListen(this);
   this.alive = true;
+  this._acceptConnections = true;
 };
 
 MarionetteServer.prototype.stop = function() {
   if (!this.alive) {
     return;
   }
   this.closeListener();
   this.alive = false;
+  this._acceptConnections = false;
 };
 
 MarionetteServer.prototype.closeListener = function() {
   this.listener.close();
   this.listener = null;
 };
 
 MarionetteServer.prototype.onSocketAccepted = function(
     serverSocket, clientSocket) {
+  if (!this._acceptConnections) {
+    logger.warn("New connections are currently not accepted");
+    return;
+  }
+
   let input = clientSocket.openInputStream(0, 0, 0);
   let output = clientSocket.openOutputStream(0, 0, 0);
   let transport = new DebuggerTransport(input, output);
   let connId = "conn" + this.nextConnId++;
 
   let dispatcher = new Dispatcher(connId, transport, this.driverFactory.bind(this));
   dispatcher.onclose = this.onConnectionClosed.bind(this);
   this.conns[connId] = dispatcher;