Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Choose a random free tcp port to be used for the Firefox Desktop remote debugging server #1669

Merged
merged 9 commits into from
Sep 24, 2019
15 changes: 2 additions & 13 deletions src/extension-runners/firefox-android.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
* in a Firefox for Android instance.
*/

import net from 'net';
import path from 'path';
import readline from 'readline';

Expand All @@ -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';
Expand Down Expand Up @@ -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).
Expand All @@ -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,
Expand Down
44 changes: 2 additions & 42 deletions src/firefox/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -36,48 +35,9 @@ export const defaultFirefoxEnv = {

// defaultRemotePortFinder types and implementation.

export type RemotePortFinderParams = {|
portToTry?: number,
retriesLeft?: number,
connectToFirefox?: FirefoxConnectorFn,
|};

export type RemotePortFinderFn =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is also changing the type signature for the defaultRemotePortFinder method,
which is currently expected to match the flow type signature at line 40 (export type RemotePortFinderFn).

The flow checks only run after the eslint linting step completes successfully, but I'm pretty sure that flow will be the next to complain (e.g. because RemotePortFinderFn is still referring the RemotePortFinderParams type that has been removed).

To learn more about the flow and the kind of checks it does, you may also take a look to the related section in the CONTRIBUTING.md file: https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#check-for-flow-errors).

(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.

Expand Down
13 changes: 13 additions & 0 deletions src/firefox/remote.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -249,3 +251,14 @@ export async function connectWithMaxRetries(
log.debug('Connecting to the remote Firefox debugger');
return establishConnection();
}

rbrishabh marked this conversation as resolved.
Show resolved Hide resolved
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));
});
});
}
16 changes: 14 additions & 2 deletions tests/functional/fake-firefox-binary.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,19 @@ function toRDP(msg) {
return [data.length, ':', data].join('');
}

// Start a TCP Server
// Getting PORT from arguments
rbrishabh marked this conversation as resolved.
Show resolved Hide resolved
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];
rbrishabh marked this conversation as resolved.
Show resolved Hide resolved
if (isNaN(port)) {
throw new Error(`Value of port must be a number. ${port} is not a number.`);
}

return port;
}
net.createServer(function(socket) {
socket.on('data', function(data) {
if (String(data) === toRDP(REQUEST_LIST_TABS)) {
Expand All @@ -43,4 +55,4 @@ net.createServer(function(socket) {
});

socket.write(toRDP(REPLY_INITIAL));
}).listen(6005);
}).listen(getPortFromArgs());
76 changes: 0 additions & 76 deletions tests/unit/test-firefox/test.firefox.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
});

});
});
11 changes: 11 additions & 0 deletions tests/unit/test-firefox/test.remote.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
connect as defaultConnector,
connectWithMaxRetries,
RemoteFirefox,
findFreeTcpPort,
} from '../../../src/firefox/remote';
import {
fakeFirefoxClient,
Expand Down Expand Up @@ -375,4 +376,14 @@ describe('firefox.remote', () => {

});

describe('defaultRemotePortFinder', () => {
rbrishabh marked this conversation as resolved.
Show resolved Hide resolved

it('resolves to an open port', () => {
rbrishabh marked this conversation as resolved.
Show resolved Hide resolved
return findFreeTcpPort()
.then((port) => {
assert.isNumber(port);
});
});
});

});