Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. draft
authorHenrik Skupin <mail@hskupin.info>
Mon, 17 Oct 2016 13:19:19 +0200
changeset 425963 71e3298f3556e28ee5d35b12faa07bce179e99f6
parent 424766 f03e2740d604d339ed553dad62a3fc54c317f8fa
child 534027 8c59497e1efbf8abbfc5e2c163cc495b4dfc9a0d
push id32548
push userbmo:hskupin@gmail.com
push dateMon, 17 Oct 2016 11:20:10 +0000
bugs1309556
milestone52.0a1
Bug 1309556 - Ensure to correctly shutdown the application for quit/restart when callbacks are used. 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
@@ -1056,17 +1056,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.
@@ -1080,19 +1079,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
@@ -1115,19 +1116,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(self.wait_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;