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

feat: allow port option when run with target chromium #3188

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions src/cmd/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ export default async function run(
firefoxApkComponent,
// Chromium CLI options.
chromiumBinary,
chromiumPort,
chromiumProfile,
},
{
Expand Down Expand Up @@ -186,6 +187,7 @@ export default async function run(
const chromiumRunnerParams = {
...commonRunnerParams,
chromiumBinary,
chromiumPort,
chromiumProfile,
};

Expand Down
16 changes: 16 additions & 0 deletions src/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,22 @@ export class MultiExtensionsReloadError extends WebExtError {
}
}

export class UnusablePortError extends WebExtError {
constructor(message) {
super(message);
}
}
export class PortInUseError extends UnusablePortError {
constructor(message = 'The requested port is in use by another process') {
super(message);
}
}
export class PortInvalidError extends UnusablePortError {
constructor(message = 'The requested port cannot be used.') {
super(message);
}
}

/*
* Sugar-y way to catch only instances of a certain error.
*
Expand Down
16 changes: 15 additions & 1 deletion src/extension-runners/chromium.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import { createLogger } from '../util/logger.js';
import { TempDir } from '../util/temp-dir.js';
import isDirectory from '../util/is-directory.js';
import fileExists from '../util/file-exists.js';
import { validatePort } from '../util/verify-chromium-port.js';
import { PortInUseError, PortInvalidError } from '../errors.js';

const log = createLogger(import.meta.url);

Expand Down Expand Up @@ -151,6 +153,18 @@ export class ChromiumExtensionRunner {
chromeFlags.push(...this.params.args);
}

const { chromiumPort } = this.params;
if (chromiumPort) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you intentionally excluding --chromium-port=0 here? The validation helpers that you have added are not 100% correct when 0 is passed.

The expected behavior when 0 is passed is to select a random port (which chrome-launcher does indeed).

log.debug('(port: %d)', chromiumPort);
const isPortUsable = await validatePort(chromiumPort).catch((error) => {
throw new PortInvalidError(error.message);
});

if (!isPortUsable) {
throw new PortInUseError();
}
}

// eslint-disable-next-line prefer-const
let { userDataDir, profileDirName } =
await ChromiumExtensionRunner.getProfilePaths(
Expand Down Expand Up @@ -210,8 +224,8 @@ export class ChromiumExtensionRunner {
userDataDir,
// Ignore default flags to keep the extension enabled.
ignoreDefaultFlags: true,
port: chromiumPort,
});

this.chromiumInstance.process.once('close', () => {
this.chromiumInstance = null;

Expand Down
52 changes: 52 additions & 0 deletions src/util/verify-chromium-port.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { createServer } from 'node:http';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { createServer } from 'node:http';
import { createServer } from 'node:net';

We don't really need http capabilities, node:net is enough and we don't need node:http.


import { UnusablePortError } from '../errors.js';

/**
* Determine if a port is available
* @param {number} port Port to test
* */
async function isPortAvailable(port) {
return new Promise((resolve) => {
const server = createServer();
server.once('listening', () => {
Copy link
Member

Choose a reason for hiding this comment

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

For simplicity, you could also consider passing the callback as a parameter to the listen function, which is equivalent to manually registering a listening listener beforehand.

server.close(() => resolve(true));
});
server.once('error', () => resolve(false));
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that the only error is that the port is in use (in which case e.code === 'EADDRINUSE'). This is not correct. E.g. ports 1-1023 require privileges, and when not granted (which is the most common case), EACCES is raised.

server.listen(port);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
server.listen(port);
server.listen(port, '127.0.0.1');

By default, listen() listens on :: and/or 0.0.0.0, which would enable external clients on the network to connect to the server. This may also trigger a firewall notification. This all is undesirable.

To avoid these issues, explicitly pass in the localhost IP, i.e. 127.0.0.1.

});
}

/**
* Validate that requested port is a valid port
* @param {number} port Debugging port
* @returns {boolean}
Copy link
Member

Choose a reason for hiding this comment

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

This isPortValid helperand the exportedisPortValid` helper document a boolean return value, but throw errors when the port is invalid. This is not documented, and strange.

To avoid the strange "true / false / Error` tristate, you could consider dropping the boolean return values and just have error vs non-Error.

// Raises an error if the port is invalid for any reason
await ensureValidPort(port);

Another option is to have a boolean return value (port available or not) and throw one error (PortNotAvailableError), with the diagnostic information exported via the log object (example of createLogger).

*/
function isPortValid(port) {
if (!port) {
return false;
}
Comment on lines +26 to +28
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (!port) {
return false;
}

The caller is expected to pass a port, this branch is unreachable. For simplicity let's just drop it (an error would be raised if port is somehow omitted).

if (Number.isNaN(port) || !Number.isInteger(port)) {
throw new UnusablePortError(`Port provided is not an integer (${port})`);
}
if (port < 0 || port > 65535) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (port < 0 || port > 65535) {
if (port <= 0 || port > 65535) {

0 should be excluded here. It is not reachable and if it were, it would result in a random port being chosen.

throw new UnusablePortError(`Invalid port number: ${port}`);
}

return true;
}

/**
* Validate user-supplied port to ensure it is suitable for use
* @param {number} port User-supplied port request
* @param {string} chromiumBinary Chromium binary being requested for launch
* @param {string[]} chromeFlags Array of flags requested for launch
Comment on lines +42 to +43
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param {string} chromiumBinary Chromium binary being requested for launch
* @param {string[]} chromeFlags Array of flags requested for launch

These parameters don't exist.

* @returns {boolean} Whether requested port is usable
*/
export async function validatePort(port) {
if (!isPortValid(port)) {
return false;
}

return isPortAvailable(port);
}
15 changes: 15 additions & 0 deletions tests/unit/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,15 @@ export class StubChildProcess extends EventEmitter {
stderr = new EventEmitter();
stdout = new EventEmitter();
kill = sinon.spy(() => {});

constructor(params = {}) {
Copy link
Member

Choose a reason for hiding this comment

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

Drop this all - it is not needed by StubChildProcess and should not be added.

super();

// mimic chrome-launch and set a port property, defaulting to a random port
this.port = params.chromiumPort
? params.chromiumPort
: getRandomInt(1024, 65536);
}
}

export function createFakeProcess() {
Expand Down Expand Up @@ -336,3 +345,9 @@ export function mockModule({
export function resetMockModules() {
td.reset();
}

export function getRandomInt(min, max) {
const _min = Math.ceil(min);
const _max = Math.floor(max);
return Math.floor(Math.random() * (_max - _min) + _min);
}
32 changes: 31 additions & 1 deletion tests/unit/test-extension-runners/test.chromium.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import isDirectory from '../../../src/util/is-directory.js';

function prepareExtensionRunnerParams({ params } = {}) {
const fakeChromeInstance = {
process: new StubChildProcess(),
process: new StubChildProcess(params),
Copy link
Member

Choose a reason for hiding this comment

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

If you want to more realistically mimic the result of chromium-launch, then you can assign the result to fakeChromeInstance instead of forwarding it through StubChildProcess.

Here is the type from chrome-launcher: https://github.com/GoogleChrome/chrome-launcher/blob/v1.1.1/src/chrome-launcher.ts#L85

kill: sinon.spy(async () => {}),
};
const runnerParams = {
Expand Down Expand Up @@ -615,6 +615,36 @@ describe('util/extension-runners/chromium', async () => {
}),
);

it('does pass port to chrome', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

This unit test only tests the good-weather behavior. You should also test the bad weather behavior, i.e. the error conditions.

Here are the relevant cases:

  • port is available.
  • port is clearly invalid, e.g. -1 and 99999
  • edge case: port 0. Ordinarily this means to select a random port.
  • port is unavailable, which the test can guarantee by reserving a port.

const port = 9222;
Copy link
Member

Choose a reason for hiding this comment

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

This unit test will fail if 9222 is used at any point. You should choose a free port here.

Note that we already have a utility to find a free port, at

export function findFreeTcpPort() {

const { params } = prepareExtensionRunnerParams({
params: { chromiumPort: port },
});

const runnerInstance = new ChromiumExtensionRunner(params);
await runnerInstance.run();

sinon.assert.calledOnce(params.chromiumLaunch);
sinon.assert.calledWithMatch(params.chromiumLaunch, {
port,
});
assert.isDefined(
runnerInstance.chromiumInstance,
'returned process instance',
);
assert.isDefined(
runnerInstance.chromiumInstance.process.port,
'process instance has port value',
);
Comment on lines +635 to +638
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert.isDefined(
runnerInstance.chromiumInstance.process.port,
'process instance has port value',
);

This assertion is not useful. It merely tests the behavior of the internal stub logic.

assert.equal(
runnerInstance.chromiumInstance.process.port,
Copy link
Member

Choose a reason for hiding this comment

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

port is not a member of process. Fix the stub above and read it off chromiumInstance.port.

port,
'process instance configured with correct port',
);

await runnerInstance.exit();
});

describe('reloadAllExtensions', () => {
let runnerInstance;
let wsClient;
Expand Down