fix: Choose a random free tcp port to be used for the Firefox Desktop…

… remote debugging server (#1669) This patch changes how web-ext run choose a free tcp port for the Firefox Desktop remote debugging server. This change aims to reduce the chances of reusing a port that may still being in use, and then fix (or at least reduce the incidence of) #1509
mozilla · Sep 24, 2019 · f2043c7 · f2043c7
1 parent 1e6e51b
commit f2043c7
Show file tree

Hide file tree

Showing 6 changed files with 72 additions and 139 deletions.
diff --git a/src/extension-runners/firefox-android.js b/src/extension-runners/firefox-android.js
@@ -5,7 +5,6 @@
  * in a Firefox for Android instance.
  */
 
-import net from 'net';
 import path from 'path';
 import readline from 'readline';
 
@@ -22,6 +21,7 @@ import {
 import * as defaultFirefoxApp from '../firefox';
 import {
   connectWithMaxRetries as defaultFirefoxConnector,
+  findFreeTcpPort,
 } from '../firefox/remote';
 import {createLogger} from '../util/logger';
 import {isTTY, setRawMode} from '../util/stdin';
@@ -567,7 +567,7 @@ export class FirefoxAndroidExtensionRunner {
 
     log.debug(`RDP Socket File selected: ${this.selectedRDPSocketFile}`);
 
-    const tcpPort = await this.chooseLocalTcpPort();
+    const tcpPort = await findFreeTcpPort();
 
     // Log the choosen tcp port at info level (useful to the user to be able
     // to connect the Firefox DevTools to the Firefox for Android instance).
@@ -586,17 +586,6 @@ export class FirefoxAndroidExtensionRunner {
     this.selectedTCPPort = tcpPort;
   }
 
-  chooseLocalTcpPort(): Promise<number> {
-    return new Promise((resolve) => {
-      const srv = net.createServer();
-      // $FLOW_FIXME: flow has his own opinions on this method signature.
-      srv.listen(0, () => {
-        const freeTcpPort = srv.address().port;
-        srv.close(() => resolve(freeTcpPort));
-      });
-    });
-  }
-
   async rdpInstallExtensions() {
     const {
       selectedTCPPort,

diff --git a/src/firefox/index.js b/src/firefox/index.js
@@ -13,10 +13,9 @@ import isDirectory from '../util/is-directory';
 import {isErrorWithCode, UsageError, WebExtError} from '../errors';
 import {getPrefs as defaultPrefGetter} from './preferences';
 import {getManifestId} from '../util/manifest';
+import {findFreeTcpPort as defaultRemotePortFinder} from './remote';
 import {createLogger} from '../util/logger';
-import {connect as defaultFirefoxConnector, REMOTE_PORT} from './remote';
 // Import flow types
-import type {FirefoxConnectorFn} from './remote';
 import type {
   PreferencesAppName,
   PreferencesGetterFn,
@@ -36,48 +35,9 @@ export const defaultFirefoxEnv = {
 
 // defaultRemotePortFinder types and implementation.
 
-export type RemotePortFinderParams = {|
-  portToTry?: number,
-  retriesLeft?: number,
-  connectToFirefox?: FirefoxConnectorFn,
-|};
 
 export type RemotePortFinderFn =
-  (params?: RemotePortFinderParams) => Promise<number>;
-
-export async function defaultRemotePortFinder(
-  {
-    portToTry = REMOTE_PORT,
-    retriesLeft = 10,
-    connectToFirefox = defaultFirefoxConnector,
-  }: RemotePortFinderParams = {}
-): Promise<number> {
-  log.debug(`Checking if remote Firefox port ${portToTry} is available`);
-
-  let client;
-
-  while (retriesLeft >= 0) {
-    try {
-      client = await connectToFirefox(portToTry);
-      log.debug(`Remote Firefox port ${portToTry} is in use ` +
-                `(retries remaining: ${retriesLeft})`);
-    } catch (error) {
-      if (isErrorWithCode('ECONNREFUSED', error)) {
-        // The connection was refused so this port is good to use.
-        return portToTry;
-      }
-
-      throw error;
-    }
-
-    client.disconnect();
-    portToTry++;
-    retriesLeft--;
-  }
-
-  throw new WebExtError('Too many retries on port search');
-}
-
+  () => Promise<number>;
 
 // Declare the needed 'fx-runner' module flow types.
 

diff --git a/src/firefox/remote.js b/src/firefox/remote.js
@@ -1,4 +1,6 @@
 /* @flow */
+import net from 'net';
+
 import defaultFirefoxConnector from '@cliqz-oss/node-firefox-connect';
 // RemoteFirefox types and implementation
 import type FirefoxClient from '@cliqz-oss/firefox-client';
@@ -13,10 +15,6 @@ import {
 
 const log = createLogger(__filename);
 
-// The default port that Firefox's remote debugger will listen on and the
-// client will connect to.
-export const REMOTE_PORT = 6005;
-
 export type FirefoxConnectorFn =
   (port?: number) => Promise<FirefoxClient>;
 
@@ -192,7 +190,7 @@ export type ConnectOptions = {|
 |};
 
 export async function connect(
-  port: number = REMOTE_PORT,
+  port: number,
   {connectToFirefox = defaultFirefoxConnector}: ConnectOptions = {}
 ): Promise<RemoteFirefox> {
   log.debug(`Connecting to Firefox on port ${port}`);
@@ -249,3 +247,14 @@ export async function connectWithMaxRetries(
   log.debug('Connecting to the remote Firefox debugger');
   return establishConnection();
 }
+
+export function findFreeTcpPort(): Promise<number> {
+  return new Promise((resolve) => {
+    const srv = net.createServer();
+    // $FLOW_FIXME: flow has his own opinions on this method signature.
+    srv.listen(0, () => {
+      const freeTcpPort = srv.address().port;
+      srv.close(() => resolve(freeTcpPort));
+    });
+  });
+}
diff --git a/tests/functional/fake-firefox-binary.js b/tests/functional/fake-firefox-binary.js
@@ -22,7 +22,19 @@ function toRDP(msg) {
   return [data.length, ':', data].join('');
 }
 
-// Start a TCP Server
+// Get the debugger server port from the cli arguments
+function getPortFromArgs() {
+  const index = process.argv.indexOf('-start-debugger-server');
+  if (index === -1) {
+    throw new Error('The -start-debugger-server parameter is not present.');
+  }
+  const port = process.argv[index + 1];
+  if (isNaN(port)) {
+    throw new Error(`Value of port must be a number. ${port} is not a number.`);
+  }
+
+  return parseInt(port, 10);
+}
 net.createServer(function(socket) {
   socket.on('data', function(data) {
     if (String(data) === toRDP(REQUEST_LIST_TABS)) {
@@ -43,4 +55,4 @@ net.createServer(function(socket) {
   });
 
   socket.write(toRDP(REPLY_INITIAL));
-}).listen(6005);
+}).listen(getPortFromArgs());
diff --git a/tests/unit/test-firefox/test.firefox.js b/tests/unit/test-firefox/test.firefox.js
@@ -14,14 +14,10 @@ import {withTempDir} from '../../../src/util/temp-dir';
 import {
   basicManifest,
   fixturePath,
-  fake,
   makeSureItFails,
   manifestWithoutApps,
-  TCPConnectError,
   ErrorWithCode,
 } from '../helpers';
-import {RemoteFirefox} from '../../../src/firefox/remote';
-import type {RemotePortFinderParams} from '../../../src/firefox/index';
 
 const {defaultFirefoxEnv} = firefox;
 
@@ -975,76 +971,4 @@ describe('firefox', () => {
 
   });
 
-  describe('defaultRemotePortFinder', () => {
-
-    function findRemotePort({...args}: RemotePortFinderParams = {}) {
-      return firefox.defaultRemotePortFinder({...args});
-    }
-
-    it('resolves to an open port', () => {
-      const connectToFirefox = sinon.spy(
-        () => Promise.reject(new TCPConnectError()));
-      return findRemotePort({connectToFirefox})
-        .then((port) => {
-          assert.isNumber(port);
-        });
-    });
-
-    it('returns a port on first try', () => {
-      const connectToFirefox = sinon.spy(() => new Promise(
-        (resolve, reject) => {
-          reject(
-            new TCPConnectError('first call connection fails - port is free')
-          );
-        }));
-      return findRemotePort({connectToFirefox, retriesLeft: 2})
-        .then((port) => {
-          sinon.assert.calledOnce(connectToFirefox);
-          assert.isNumber(port);
-        });
-    });
-
-    it('cancels search after too many fails', () => {
-      const client = fake(RemoteFirefox.prototype);
-      const connectToFirefox = sinon.spy(() => new Promise(
-        (resolve) => resolve(client)));
-      return findRemotePort({connectToFirefox, retriesLeft: 2})
-        .catch((err) => {
-          assert.equal(err, 'WebExtError: Too many retries on port search');
-          sinon.assert.calledThrice(connectToFirefox);
-        });
-    });
-
-    it('retries port discovery after first failure', () => {
-      const client = fake(RemoteFirefox.prototype);
-      let callCount = 0;
-      const connectToFirefox = sinon.spy(() => {
-        callCount++;
-        return new Promise((resolve, reject) => {
-          if (callCount === 2) {
-            reject(new TCPConnectError('port is free'));
-          } else {
-            resolve(client);
-          }
-        });
-      });
-      return findRemotePort({connectToFirefox, retriesLeft: 2})
-        .then((port) => {
-          assert.isNumber(port);
-          sinon.assert.calledTwice(connectToFirefox);
-        });
-    });
-
-    it('throws on unexpected errors', async () => {
-      const connectToFirefox = sinon.spy(async () => {
-        throw new Error('Unexpected connect error');
-      });
-
-      await assert.isRejected(findRemotePort({connectToFirefox}),
-                              /Unexpected connect error/);
-
-      sinon.assert.calledOnce(connectToFirefox);
-    });
-
-  });
 });
diff --git a/tests/unit/test-firefox/test.remote.js b/tests/unit/test-firefox/test.remote.js
@@ -1,4 +1,6 @@
 /* @flow */
+import net from 'net';
+
 import {describe, it} from 'mocha';
 import {assert} from 'chai';
 import sinon from 'sinon';
@@ -13,6 +15,7 @@ import {
   connect as defaultConnector,
   connectWithMaxRetries,
   RemoteFirefox,
+  findFreeTcpPort,
 } from '../../../src/firefox/remote';
 import {
   fakeFirefoxClient,
@@ -25,7 +28,7 @@ describe('firefox.remote', () => {
 
   describe('connect', () => {
 
-    function prepareConnection(port = undefined, options = {}) {
+    function prepareConnection(port = 6005, options = {}) {
       options = {
         connectToFirefox:
           sinon.spy(() => Promise.resolve(fakeFirefoxClient())),
@@ -375,4 +378,40 @@ describe('firefox.remote', () => {
 
   });
 
+  describe('findFreeTcpPort', () => {
+    async function promiseServerOnPort(port): Promise<net.Server> {
+      return new Promise((resolve) => {
+        const srv = net.createServer();
+        srv.listen(port, () => {
+          resolve(srv);
+        });
+      });
+    }
+
+    it('resolves to a free tcp port', async () => {
+      const port = await findFreeTcpPort();
+      assert.isNumber(port);
+      // Expect a port that is not in the reserved range.
+      assert.isAtLeast(port, 1024);
+
+      // The TCP port can be used to successfully start a TCP server.
+      const srv = await promiseServerOnPort(port);
+      assert.equal(srv.address().port, port);
+
+      // Check that calling tcp port again doesn't return the
+      // previous port (as it is not free anymore).
+      const newPort = await findFreeTcpPort();
+      assert.notStrictEqual(port, newPort);
+      assert.isAtLeast(port, 1024);
+
+      // The new TCP port can be used to successfully start a TCP server.
+      const srv2 = await promiseServerOnPort(newPort);
+      assert.equal(srv2.address().port, newPort);
+
+      srv.close();
+      srv2.close();
+    });
+
+  });
+
 });