feat: add chromium debugger port option #2565
Conversation
Codecov ReportBase: 99.52% // Head: 99.52% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #2565 +/- ##
=======================================
Coverage 99.52% 99.52%
=======================================
Files 32 32
Lines 1697 1697
=======================================
Hits 1689 1689
Misses 8 8
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
@fuji44 do you know how we could merge this one? |
|
@Rob--W can we merge this branch? |
|
@saadbahir |
| @@ -232,6 +233,7 @@ export class ChromiumExtensionRunner { | |||
| userDataDir, | |||
| // Ignore default flags to keep the extension enabled. | |||
| ignoreDefaultFlags: true, | |||
| port: this.params.chromiumDebuggerPort, | |||
There was a problem hiding this comment.
Passing this flag without validation is dangerous, because it may break the functionality of web-ext.
web-ext has to launch a new Chrome browser instance, because the extension and auxiliary extension are installed via a command-line flag, --load-extension to be specific.
When chrome-launcher receives a given port, it tries to connect to that port, and if it exists, it will not start a new Chrome browser, but connect to an existing one instead: https://github.com/GoogleChrome/chrome-launcher/blob/v0.15.1/src/chrome-launcher.ts#L248-L264
IF we want to support taking a fixed flag as input, then we should validate that the port is unused, and throw an explicit error if it is used.
There was a problem hiding this comment.
I agree with the basic idea that not validating external parameters is dangerous, but I don't think it applies in this case. web-ext is just passing a port number, and it is essentially the browser's responsibility to validate the port number. Well, chrome doesn't seem to check it.
Even if you implement the verification code, wouldn't it be enough to verify that the specified number is within the TCP port range?
Verifying whether a port is in use may be useful, but it is a "nice to have" feature.
Considering that the user of web-ext is a developer, it is common sense not to use duplicate ports, and even if duplicate ports are used by mistake, they will immediately consider the possibility.
So the bottom line is that for most web developers, the ability to verify that a port is in use is an unnecessary feature. This is undesirable because it adds complexity to the code and reduces maintainability.
There was a problem hiding this comment.
I explained why the port has to be unused in my previous comment.
Summarized, web-ext would not work and be unreliable. The web-ext program expects the chrome-launcher library to launch the browser with the extension loaded, but if the port is in use, it won't launch Chrome, and therefore the core feature of web-ext run will be stuck waiting for the browser+extension to start. Explicitly failing with a useful message is more desirable than getting stuck without further information.
There was a problem hiding this comment.
When I get time to work on it, I will check the stacking behavior.
| @@ -329,6 +329,26 @@ describe('run', () => { | |||
| ); | |||
| }); | |||
|
|
|||
| it('provides a chromiumDebuggerPort option to the Chromium runner', async () => { | |||
There was a problem hiding this comment.
This test is not enough for test coverage. It merely verifies that the CLI parameter is passed to the extension runner.
You need to add more tests, to verify that the port is received correctly in https://github.com/mozilla/web-ext/blob/005bed86dff796cca48281b1928f7109b0972c80/tests/unit/test-extension-runners/test.chromium.js . And the check requested in the other comment (i.e. validity of port) should be tested too.
Co-authored-by: Rob Wu <rob@robwu.nl>
|
It is not good to leave it there forever, so I am going to close it and work on it again when I have more free time. |
Fixes #2284
This PR adds an option to specify the debugger port number for chromium-based browsers.
The motivation is to make debugger attachments declarative and manageable.
Note: It seems that the unit test is failing in an area unrelated to my modification.