Bug 1410366 - Stop socket server from listening for new connections if told so. draft
authorHenrik Skupin <mail@hskupin.info>
Mon, 23 Oct 2017 14:08:15 +0200
changeset 684879 cf180cd1542145fd30ef2f87b8694b759e75f50d
parent 684511 ce1a86d3b4db161c95d1147676bbed839d7a4732
child 684880 212d6ee38cf1d0ea8c378bf5d485fa6641a260c1
child 685477 1c6c13c3aa111e5a239efd209ccfb8de43d9fe31
push id85745
push userbmo:hskupin@gmail.com
push dateMon, 23 Oct 2017 17:40:49 +0000
bugs1410366
milestone58.0a1
Bug 1410366 - Stop socket server from listening for new connections if told so. Simply checking '_acceptConnections' when clients are trying to connect to Marionette, and revoking the connection request inside of 'onSocketAccepted' is plainly wrong, given that a connection is already present. Instead put the socket server into close state, which means it does no longer listen for new connection attempts until new connections are accepted again. MozReview-Commit-ID: JIpOgOjnpDY
testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
testing/marionette/server.js
--- a/testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
+++ b/testing/marionette/harness/marionette_harness/tests/unit/test_marionette.py
@@ -1,15 +1,16 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import time
 
 from marionette_driver import errors
+from marionette_driver.marionette import Marionette
 from marionette_harness import MarionetteTestCase, run_if_manage_instance, skip_if_mobile
 
 
 class TestMarionette(MarionetteTestCase):
 
     def test_correct_test_name(self):
         """Test that the correct test name gets set."""
         expected_test_name = '{module}.py {cls}.{func}'.format(
@@ -25,16 +26,36 @@ class TestMarionette(MarionetteTestCase)
     def test_wait_for_port_non_existing_process(self):
         """Test that wait_for_port doesn't run into a timeout if instance is not running."""
         self.marionette.quit()
         self.assertIsNotNone(self.marionette.instance.runner.returncode)
         start_time = time.time()
         self.assertFalse(self.marionette.wait_for_port(timeout=5))
         self.assertLess(time.time() - start_time, 5)
 
+    def test_disable_enable_new_connections(self):
+        # Do not re-create socket if it already exists
+        self.marionette._send_message("acceptConnections", {"value": True})
+
+        try:
+            # Disabling new connections does not affect existing ones...
+            self.marionette._send_message("acceptConnections", {"value": False})
+            self.assertEqual(1, self.marionette.execute_script("return 1"))
+
+            # but only new connection attempts
+            marionette = Marionette(host=self.marionette.host, port=self.marionette.port)
+            self.assertFalse(marionette.wait_for_port(timeout=1.0),
+                             "Unexpected connection with acceptConnections=false")
+
+            self.marionette._send_message("acceptConnections", {"value": True})
+            marionette.wait_for_port(timeout=1.0)
+
+        finally:
+            self.marionette._send_message("acceptConnections", {"value": True})
+
 
 class TestContext(MarionetteTestCase):
 
     def setUp(self):
         MarionetteTestCase.setUp(self)
         self.marionette.set_context(self.marionette.CONTEXT_CONTENT)
 
     def get_context(self):
--- a/testing/marionette/server.js
+++ b/testing/marionette/server.js
@@ -304,17 +304,16 @@ server.TCPListener = class {
    *     Port for server to listen to.
    */
   constructor(port) {
     this.port = port;
     this.socket = null;
     this.conns = new Set();
     this.nextConnID = 0;
     this.alive = false;
-    this._acceptConnections = false;
     this.alteredPrefs = new Set();
   }
 
   /**
    * Function produces a GeckoDriver.
    *
    * Determines the application to initialise the driver with.
    *
@@ -322,23 +321,32 @@ server.TCPListener = class {
    *     A driver instance.
    */
   driverFactory() {
     Preferences.set(PREF_CONTENT_LISTENER, false);
     return new GeckoDriver(Services.appinfo.ID, this);
   }
 
   set acceptConnections(value) {
-    if (!value) {
-      logger.info("New connections will no longer be accepted");
-    } else {
-      logger.info("New connections are accepted again");
+    if (value) {
+      if (!this.socket) {
+        const flags = KeepWhenOffline | LoopbackOnly;
+        const backlog = 1;
+        this.socket = new ServerSocket(this.port, flags, backlog);
+        this.port = this.socket.port;
+
+        this.socket.asyncListen(this);
+        logger.debug("New connections are accepted");
+      }
+
+    } else if (this.socket) {
+      this.socket.close();
+      this.socket = null;
+      logger.debug("New connections will no longer be accepted");
     }
-
-    this._acceptConnections = value;
   }
 
   /**
    * Bind this listener to |port| and start accepting incoming socket
    * connections on |onSocketAccepted|.
    *
    * The marionette.port preference will be populated with the value
    * of |this.port|.
@@ -356,55 +364,45 @@ server.TCPListener = class {
         if (!Preferences.isSet(k)) {
           logger.debug(`Setting recommended pref ${k} to ${v}`);
           Preferences.set(k, v);
           this.alteredPrefs.add(k);
         }
       }
     }
 
-    const flags = KeepWhenOffline | LoopbackOnly;
-    const backlog = 1;
-    this.socket = new ServerSocket(this.port, flags, backlog);
-    this.socket.asyncListen(this);
-    this.port = this.socket.port;
+    // Start socket server and listening for connection attempts
+    this.acceptConnections = true;
+
     Preferences.set(PREF_PORT, this.port);
+    env.set(ENV_ENABLED, "1");
 
     this.alive = true;
-    this._acceptConnections = true;
-    env.set(ENV_ENABLED, "1");
   }
 
   stop() {
     if (!this.alive) {
       return;
     }
 
-    this._acceptConnections = false;
-
-    this.socket.close();
-    this.socket = null;
-
     for (let k of this.alteredPrefs) {
       logger.debug(`Resetting recommended pref ${k}`);
       Preferences.reset(k);
     }
     this.alteredPrefs.clear();
 
     Services.obs.notifyObservers(this, NOTIFY_RUNNING);
 
+    // Shutdown server socket, and no longer listen for new connections
+    this.acceptConnections = false;
+
     this.alive = false;
   }
 
   onSocketAccepted(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 conn = new server.TCPConnection(
         this.nextConnID++, transport, this.driverFactory.bind(this));
     conn.onclose = this.onConnectionClosed.bind(this);
     this.conns.add(conn);