Bug 1410366 - Stop socket server from listening for new connections if told so. r=ato
☠☠ backed out by 5861384d1db5 ☠ ☠
authorHenrik Skupin <mail@hskupin.info>
Mon, 23 Oct 2017 14:08:15 +0200
changeset 387768 f4152054eb1bf12de73edfd9fd1ad5ce9302d891
parent 387767 94a7d2b657c6354b29a6602470775696dce60b40
child 387769 43e45144dd9fffa826b7b4d8289f77ddeea172c6
push id96497
push userarchaeopteryx@coole-files.de
push dateTue, 24 Oct 2017 09:57:53 +0000
treeherdermozilla-inbound@7de3cc48b5b1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersato
bugs1410366
milestone58.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 1410366 - Stop socket server from listening for new connections if told so. r=ato 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);