feat: allow port option when run with target chromium #3188
Conversation
dever23b
changed the title
|
Oh, I apologize-- I got ahead of myself and failed to check that other PR. I see your concern regarding a potential to attach to an instance that doesn't have the extension loaded. Good thinking! I can try to work on a mitigation for that. Since I haven't gotten started yet, I don't know if what I'm asking for might be more effort than it's worth to write, but are you hard set on aborting if the port is in use? I'm wondering if it would be permissible to attach to an existing instance on that port, if I could find a way to verify that the existing instance was launched with the same |
|
Alright, sorry for the abundance of commits. I didn't realize that this PR would cause everything to automatically sync from my fork. I believe the current state meets your stated concerns, @Rob--W. Would you mind giving this another review? |
| @@ -0,0 +1,52 @@ | |||
| import { createServer } from 'node:http'; | |||
There was a problem hiding this comment.
| 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.
| server.close(() => resolve(true)); | ||
| }); | ||
| server.once('error', () => resolve(false)); | ||
| server.listen(port); |
There was a problem hiding this comment.
| 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.
| async function isPortAvailable(port) { | ||
| return new Promise((resolve) => { | ||
| const server = createServer(); | ||
| server.once('listening', () => { |
There was a problem hiding this comment.
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.
| @@ -151,6 +153,18 @@ export class ChromiumExtensionRunner { | |||
| chromeFlags.push(...this.params.args); | |||
| } | |||
|
|
|||
| const { chromiumPort } = this.params; | |||
| if (chromiumPort) { | |||
There was a problem hiding this comment.
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).
| server.once('listening', () => { | ||
| server.close(() => resolve(true)); | ||
| }); | ||
| server.once('error', () => resolve(false)); |
There was a problem hiding this comment.
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.
| @@ -615,6 +615,36 @@ describe('util/extension-runners/chromium', async () => { | |||
| }), | |||
| ); | |||
|
|
|||
| it('does pass port to chrome', async () => { | |||
| const port = 9222; | |||
There was a problem hiding this comment.
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
Line 233 in 1c01518
| @@ -23,7 +23,7 @@ import isDirectory from '../../../src/util/is-directory.js'; | |||
|
|
|||
| function prepareExtensionRunnerParams({ params } = {}) { | |||
| const fakeChromeInstance = { | |||
| process: new StubChildProcess(), | |||
| process: new StubChildProcess(params), | |||
There was a problem hiding this comment.
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
| 'process instance has port value', | ||
| ); | ||
| assert.equal( | ||
| runnerInstance.chromiumInstance.process.port, |
There was a problem hiding this comment.
port is not a member of process. Fix the stub above and read it off chromiumInstance.port.
| assert.isDefined( | ||
| runnerInstance.chromiumInstance.process.port, | ||
| 'process instance has port value', | ||
| ); |
There was a problem hiding this comment.
| 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.
| @@ -615,6 +615,36 @@ describe('util/extension-runners/chromium', async () => { | |||
| }), | |||
| ); | |||
|
|
|||
| it('does pass port to chrome', async () => { | |||
There was a problem hiding this comment.
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.
This suggestion allows us to create a dependable debug workflow to attach an IDE to the browser that gets spawned with the
runcommand.By default, the underlying
chrome-launcherflow will choose a random debugging port when launching the browser. This is not ideal for those of us hoping to attach our IDE/other debugging tool to the browser. Ideally, we launch the browser with a predictable port that we can configure into our debugging flow. The underlyingchrome-launcheractually already supports this; however,web-extdoes not currently provide any way to pass the required option through. This PR bridges that gap.