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 684708 26f58ca470b06b15abfe36b86c0dc3449131dbe3
parent 684511 ce1a86d3b4db161c95d1147676bbed839d7a4732
child 684709 e9ca77a1bd021843be4d8a1959883e77588ecd14
push id85696
push userbmo:hskupin@gmail.com
push dateMon, 23 Oct 2017 12:59:14 +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,32 @@ 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")
+        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
@@ -322,23 +322,46 @@ 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) {
+    if (value) {
+      logger.info("New connections are accepted");
+      this.connect();
+
+    } else {
       logger.info("New connections will no longer be accepted");
-    } else {
-      logger.info("New connections are accepted again");
+      this.disconnect();
+    }
+  }
+
+  connect() {
+    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 && this.socket.asyncListen(this);
     }
 
-    this._acceptConnections = value;
+    this._acceptConnections = true;
+  }
+
+  disconnect() {
+    if (this.socket) {
+      this.socket.close();
+      this.socket = null;
+    }
+
+    this._acceptConnections = false;
   }
 
   /**
    * 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,46 +379,38 @@ 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;
+    this.connect();
+
     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);
 
+    this.disconnect();
     this.alive = false;
   }
 
   onSocketAccepted(serverSocket, clientSocket) {
     if (!this._acceptConnections) {
       logger.warn("New connections are currently not accepted");
       return;
     }