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

rbrishabh · 2019-07-28T14:07:37Z

Hi @rpl !
Good day to you!
Here is a fix for this bug. I created a pull request because this way it's easier to discuss what changes I need to do.

I ran the changes locally, and it seems to be working fine. But now, I believe, some test cases, and some irrelevant code still exists in files like for e.g: remote.js

I guess there is some cleanup required here.

Can you help me with what other changes are required here?

Thanks! =)

rpl

Besides working out the issues with eslint and flow as described below, I spotted in the travis CI job logs that this change is also introducing a "timing out" failure on one of the functional tests.

The above failure is related to the nodejs script "fake-firefox-binary.js", it is used by the web-ext run functional tests as a fake Firefox binary (currently it always uses the 6005 port, but it should now read the command line parameter passed by web-ext to specify the expected TCP port).

On the unit tests side I would expect most of the defaultRemotePortFinder test cases to fail:

https://github.com/mozilla/web-ext/blob/master/tests/unit/test-firefox/test.firefox.js#L978-L1047

It looks like the test case related to resolving to a free port is something that we should keep (and rework based on the new implementation strategy if necessary), but the ones related to the "retries" scenarios do not seem to be needed anymore (because the new version would not need to retry a given number of times to find a free port).

rpl · 2019-08-02T16:20:37Z

src/firefox/index.js

-  portToTry?: number,
-  retriesLeft?: number,
-  connectToFirefox?: FirefoxConnectorFn,
-|};

 export type RemotePortFinderFn =


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

rpl · 2019-08-02T16:22:06Z

src/firefox/index.js

@@ -14,7 +15,6 @@ import {isErrorWithCode, UsageError, WebExtError} from '../errors';
 import {getPrefs as defaultPrefGetter} from './preferences';
 import {getManifestId} from '../util/manifest';
 import {createLogger} from '../util/logger';
-import {connect as defaultFirefoxConnector, REMOTE_PORT} from './remote';
 // Import flow types
 import type {FirefoxConnectorFn} from './remote';


Based on the travis CI job failure messages we have to fix an eslint error (otherwise npm run test will not be running the unit tests neither), e.g. eslint is complaining about the now unused import type {FirefoxConnectorFn} from ....

rpl · 2019-08-02T16:34:04Z

src/firefox/index.js

+  return new Promise((resolve) => {
+    const srv = net.createServer();
+    // $FLOW_FIXME: flow has his own opinions on this method signature.
+    srv.listen(0, () => {


I think that it may be good to actually keep only a single definition of the "helper function that finds a free tcp port" and share it with the firefox-android.js module (e.g. we could move it to "src/firefox/remote.js" given that it is only going to be used for the firefox desktop and firefox android extension runners).

rpl · 2019-08-02T17:09:07Z

As a side note: I'm having the feeling that for some web-ext users it may still be useful to know which TCP port is going to be used by the Firefox instance, especially when web-ext is used as a library.
We may take a look into this as part of a separate followup.

One option may be to make the selected port more easily available in the value resolved by the run command (which would help if web-ext is used as a nodejs library).
Another option could be to support it as a additional optional cli parameter (and just fail, instead of trying to find a free port, is the specified port is already being used).

@Rob--W what do you think about it?

Rob--W · 2019-08-02T17:20:29Z

As a side note: I'm having the feeling that for some web-ext users it may still be useful to know which TCP port is going to be used by the Firefox instance, especially when web-ext is used as a library.
We may take a look into this as part of a separate followup.

Since the ports are already unpredicable, there is no loss of functionality when we choose a completely random port (as done in this PR). So it's indeed not necessary to add that functionality now, and we can decide to not implement it until someone files a feature request.

rbrishabh · 2019-08-02T18:15:30Z

@rpl , thanks for all the comments! Alright, I think I got it.

I am looking into this!

Will add more commits soon.

coveralls · 2019-08-08T12:31:13Z

Coverage remained the same at 100.0% when pulling 0a5acc3 on rbrishabh:issue-port into 961da0c on mozilla:master.

rbrishabh · 2019-08-08T12:35:22Z

Hi @rpl !

I made some changes.. I request you to review them once.
Also, I'm sorry for so many commits, I am not able to run yarn test on my machine, as I get this error everytime:

Running flow check...
{ Error: spawnSync flow ENOENT
    at Object.spawnSync (internal/child_process.js:1002:20)
    at spawnSync (child_process.js:614:24)
    at exports.flowCheck (C:\Users\Rishabh\Desktop\firefox\add-ons\web-ext\scripts\lib\flow.js:7:17)
    at Object.<anonymous> (C:\Users\Rishabh\Desktop\firefox\add-ons\web-ext\scripts\test:13:6)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:831:12)
  errno: 'ENOENT',
  code: 'ENOENT',
  syscall: 'spawnSync flow',
  path: 'flow',
  spawnargs: [ 'check' ] }

And I couldn't really find a way around it, so I kept pushing my code, depending on Travis CI to run the tests for me.

Let me know if this is headed in the correct direction!

Edit: I tried to squash all commits, but couldn't! I don't know how to go about that! :(

Thanks =)

rpl

Also, I'm sorry for so many commits, I am not able to run yarn test on my machine, as I get this error everytime:
...
And I couldn't really find a way around it, so I kept pushing my code, depending on Travis CI to run the tests for me.

uhm... would you mind to try with npm?
I'll give a try to yarn locally as soon as I can, I don't use yarn daily and so it may be an issue related to the differences between yarn and npm.

in the worst case just prevent the test command from running flow (you just need to comment this 3 line) and run the flow binary manually to run the type checking.

It is important to be able to run the test locally as much as possible while developing a change on a project, waiting for the CI job on every change is going to make the feedback loop slow and painful ;-)

Edit: I tried to squash all commits, but couldn't! I don't know how to go about that! :(

to squash all commits you can use git interactive rebase, here are some docs:

as I mentioned in one of the comment below, it is useful to try the new concepts in an isolated project (especially while learning new git commands ;-)), and so you may want to create an empty git repository, some fake commits and branches and then try out git rebase in a safe environment where you don't have the fear of losing or breaking something ;-)

rpl · 2019-08-16T21:39:38Z

src/extension-runners/firefox-android.js

-        resolve(freeTcpPort);
-      });
-    });
+  async chooseLocalTcpPort(): Promise<number> {


There is only one call to the chooseLocalTcpPort at line 571, you can replace that call with the new helper and remove this method.

rpl · 2019-08-16T21:42:57Z

src/firefox/index.js


-  throw new WebExtError('Too many retries on port search');
+export async function defaultRemotePortFinder(): Promise<number> {


You should be able to rename findFreeTcpPort() as part of its import and remove this function completely:

import {findFreeTcpPort as defaultRemotePortFinder} from './remote';

rpl · 2019-08-16T21:51:02Z

tests/functional/fake-firefox-binary.js

@@ -23,6 +25,10 @@ function toRDP(msg) {
 }

 // Start a TCP Server
+let port;
+(async () => {
+  port = await findFreeTcpPort();


the fake-firefox-binary.js should behave as the real firefox binary (just as much as needed by the test scenario).

A firefox binary will receive the port that web-ext run did choose and passed to it as one of its parameters, which should be passed as firefox -start-debugger-server PORT (the fx-runner dependency turns the listen option into the -start-debugger-server option here)

and so the fake-firefox-binary will have to do something similar to be able to "mock a real firefox binary" for the integration tests.

In a nodejs script the command line arguments are available as process.argv

Hmmm, I need to pass a port to fake-firefox-binary as one of it's parameters, and for that I need to use process.argv for that.

I still don't understand how to go about that! A little more help please! =)

rpl · 2019-08-16T21:55:16Z

tests/unit/test-firefox/test.firefox.js

    }

    it('resolves to an open port', () => {
-      const connectToFirefox = sinon.spy(
+      sinon.spy(


This spy doesn't seem to be used (to be fair it doesn't seem that we were checking the sinon spy with any assertion, but it was providing a fake connectToFirefox function to the previous version of the findRemotePort helper).

Besides that, chooseLocalTCPPort is defined in remote.js and so it should be tested as part of test.remote.js and not test.firefox.js.

rpl · 2019-08-16T22:04:43Z

tests/unit/test-firefox/test.firefox.js

    it('throws on unexpected errors', async () => {
-      const connectToFirefox = sinon.spy(async () => {
+      sinon.spy(async () => {


uhm... this test doesn't seem to be testing anything useful.

To learn more about what is the role of mocha, sinon and chai, you may want to take a look at they docs:

sinon: https://sinonjs.org/#get-started / https://sinonjs.org/how-to/ https://sinonjs.org/releases/v7.4.1/

chai: https://www.chaijs.com/api/

mocha: https://mochajs.org/#getting-started

While learning new concepts and tools is helpful (and often needed) to try them in an isolated test project (believe me on this, I also do it very often, it helps to focus on the new things one at the time and be able to build a mental model of how they work and when/why you will want to use them or not use them ;-))

At a first glance one of the articles linked in the sinon howtos page seems helpful to get started with mocha+sinon+chai (and get an initial picture of how they works):

https://scotch.io/tutorials/how-to-test-nodejs-apps-using-mocha-chai-and-sinonjs

Another important thing while writing tests, at least from my point of view, is "keeping an eye on the reasons why you are working on that test" (every test should have a clear role), e.g. asking yourself questions like:

what is exactly the scenario that we are going to write a test for?
(build a picture / a mental model of it)

why we have to test that scenario?
(e.g. it makes us sure that a certain behavior doesn't regress, or it may cover some part of the code that is not executed by any of the other tests... etc.)

is the test going to fail if the code is broken?
(in my opinion this is one of the most important ones, the tests are not meant to be "green" all the time, they are meant to be "red" when something is broken)

is the test going to fail intermittently without any code being changed or broken?
(this is something that becomes more and more important over the time, if you can't trust that a test only fails when something is actually broken, it is not going to be useful enough because we will still need to investigate it manually every time it fails to know if something is actually broken)

I read through the docs and they were really helpful! Thank you! :D

rbrishabh · 2019-08-21T14:17:47Z

@rpl Are you like the best mentor ever or what? Hahaha I'm very happy with the great explanation!

I will look into this and get back ASAP!

rbrishabh · 2019-08-22T09:34:45Z

I just squashed all commits =) ! I am now looking at the changes you mentioned!

rbrishabh · 2019-08-22T10:23:21Z

I made some of the changes you requested, and also squashed all commits!

Edit: I ran npm install again locally in wsl and this time when I ran npm test I could actually test everything. All the test cases seem to pass without any trouble.

Obviously, as you said:

(in my opinion this is one of the most important ones, the tests are not meant to be "green" all the time, they are meant to be "red" when something is broken)

I think I did go wrong somewhere!

Apart from that, I still couldn't understand what changes I have to make in fake-firefox-binary.js. I am still scratching my head over that..

Could you please give me a little more explanation?

Thanks as always!

Also the travis-ci tests are still failing for some reason! I read the logs but couldn't understand much.

rbrishabh · 2019-09-02T17:42:18Z

Hey @rpl ! It's been a while since I heard from you on this!
I was wondering if this needs any changes!

Thanks!

rpl · 2019-09-02T19:43:02Z

Hey @rpl ! It's been a while since I heard from you on this!
I was wondering if this needs any changes!

@rbrishabh I'm really sorry for the lag time, I tried to get back to this before I was going to be offline for the rest of the last week, but I haven't been able to reach it :-(

There are definitely more changes needed on this PR.

(and it will also need to be rebased on top of the changes introduced last week from landing #1392, there was a small change introduced in that PR which is conflicting with the change applied in this PR on src/extension-runners/firefox-android.js).

would you mind to rebase the PR and look in the changes needed on fake-firefox-binary.js (I'm going to describe them in a bit more detail below) in the meantime?

About the changes needed to fake-firefox-binary.js to make sure it looks for a port passed over the Firefox cli parameters:

As I mentioned in #1669 (comment), we expect web-ext run to choose a free port and then pass it to Firefox as one of its command line parameters, like in:

firefox -start-debugger-server TCPPORT

And we want fake-firefox-binary.js to "emulate" (the "minimum amount of") this behavior for the functional test, which means that if we execute the fake-firefox-binary.js script as in:

./tests/functional/fake-firefox-binary.js -start-debugger-server 6666

we expect the fake firefox binary to create a TCP server listening to the 6666 tcp port.

To achieve that we have to change fake-firefox-binary.js to implement (the bare minimum) of the Firefox behavior, e.g. the updated fake-firefox-binary.js script may look like:

// Find the debugger server port passed as a command line argument:
//   fake-firefox-binary.js -start-debugger-server PORTNUMBER
function getPortFromArgs() {
   ...
}

// Start a TCP Server
net.createServer(function(socket) {
...
}).listen(getPortFromArgs());

getPortFromArgs should:

look for the index of '-start-debugger-server' in the process.argv array (e.g. using something like process.argv.indexOf('-start-debugger-server'))
then try to parse the option that follows as a number and return that number,
if there isn't such option or it is not a number just exit the fake-firefox-binary.js with an error (so that the functional test case is going to fail if web-ext run is not working as expected, e.g. it doesn't pass a port or it doesn't pass the port number that is then used to connect to the firefox instance).

afk-mario · 2019-09-02T22:14:22Z

Maybe something like this is what you need ?

function getPortFromArgs() {
  const index = process.argv.indexOf('-start-debugger-server');
  const port = process.argv[index + 1];

  if (index === -1) {
    throw new Error('missing debugger server port parameter');
  }

  if (isNaN(port)) {
    throw new Error('debugger server port is not a number');
  }

  return port;
}

Sorry for burst in just had some free time and wanted to see how hard was to contribute to the project

rbrishabh · 2019-09-03T13:00:11Z

uhuh! Well, that's really my best attempt at rebasing this! No matter how I try to resolve conflicts locally, I have to resolve them again here, which adds another commit of merging the branch.

Anyways @rpl , how does this look?

rbrishabh · 2019-09-03T15:38:25Z

I see now that the build fails because of the commit message I wrote, I read the documentation for writing correct commit messages. I will do that.

rpl

@rbrishabh Thanks a lot for taking care of rebasing the PR and applying the changes needed on the fake-firefox-binary nodejs script! This version now looks pretty near to be ready!

I'm also going to give this PR a try locally and double-check if I may have missed something during the review, but in the meantime follows some additional review comments for some small nits to fix that I already noticed.

src/extension-runners/firefox-android.js

tests/functional/fake-firefox-binary.js

tests/unit/test-firefox/test.remote.js

…o issue-port

rbrishabh · 2019-09-05T13:02:16Z

Hey @rpl , I made those changes!

The linting of pull request failed becasue PR name had some capitals, which I wasn't aware that it was not allowed. Is there a way to re-run Travis-ci build tests without pushing another commit?

rpl · 2019-09-05T13:35:47Z

Hey @rpl , I made those changes!

The linting of pull request failed becasue PR name had some capitals, which I wasn't aware that it was not allowed. Is there a way to re-run Travis-ci build tests without pushing another commit?

It can be re-run from travis, but it is possible that the re-run button is only available to users with enough privileges on the related github repository.

Anyway, don't worry, I've already re-executed it on travis for you.

rbrishabh · 2019-09-05T17:41:09Z

Ah okay!

Please check this locally and do let me know if any other changes are required!

As always, thanks a ton for the amazing guidance!

rbrishabh · 2019-09-16T18:29:45Z

Hey @rpl !
Could you find any time to check this out locally?

Thanks!

rpl · 2019-09-16T22:33:54Z

Hey @rpl !
Could you find any time to check this out locally?

I should be able to spend a bit of time on web-ext during this week and test this change locally (we are going to release a new version on npm pretty soon and I'd like to include this change in the new version).

rbrishabh · 2019-09-17T06:15:16Z

Alright, that sounds great!

rpl

@rbrishabh 👍 this version looks good (I also gave it a try locally, just as a double-check, and everything worked as expected).

Follows some additional review comments related to some nits (small cleanups and tweaks) and some improvements to the test case (a couple of pretty small ones that we can easily apply before landing this change, and a couple more that we could defer as further improvements to a follow up)

I'm also approving this version, because the changes described below are going to be pretty small, and they don't impact the implementation (and so it is also fine if you can't work on it right now, we will just apply those changes on top or as part of the landed patch).

src/firefox/remote.js

tests/functional/fake-firefox-binary.js

tests/unit/test-firefox/test.remote.js

rbrishabh

Hey @rpl!

As always, thank you for the wonderful guidance!

I made the changes you requested before the next version is released. I am not sure about a thing or two which I am commenting below, except for that, please let me know how this looks!

Also, I am not sure if the statements I added inside it(...) for test cases are 100% appropriate and correct, so if that needs any changes, let me know!

Thanks again!

tests/unit/test-firefox/test.remote.js

rpl

@rbrishabh this version requires some changes as described in more details below.
Let me know if you have any further questions or doubts about the new round of review comments below.

src/extension-runners/chromium.js

tests/unit/test-firefox/test.remote.js

rbrishabh · 2019-09-24T02:18:02Z

@rbrishabh this version requires some changes as described in more details below.
Let me know if you have any further questions or doubts about the new round of review comments below.

Nope! Zilch doubts about that! Thank you so much for this! =)

rpl

@rbrishabh looks great!
Thanks a lot for contributing this change!

rbrishabh · 2019-09-24T12:40:39Z

Thank you so much for the amazing guidance!
@rpl

rpl requested changes Aug 2, 2019

View reviewed changes

rpl reviewed Aug 16, 2019

View reviewed changes

rbrishabh force-pushed the issue-port branch from d512374 to 5c409a3 Compare August 22, 2019 09:32

rbrishabh force-pushed the issue-port branch 3 times, most recently from a597b66 to 3dd8d50 Compare September 3, 2019 12:53

fix: Fixed method for finding default port

bb0b5ad

rbrishabh force-pushed the issue-port branch from 58688e1 to bb0b5ad Compare September 3, 2019 15:40

Merge branch 'master' into issue-port

12c0bd2

rpl requested changes Sep 4, 2019

View reviewed changes

rbrishabh added 3 commits September 5, 2019 15:33

fix: fixed some small nits

0d4a807

Merge branch 'issue-port' of https://github.com/rbrishabh/web-ext int…

5b1f22d

…o issue-port

Merge branch 'issue-port' of https://github.com/rbrishabh/web-ext int…

3c7ecdd

…o issue-port

rbrishabh changed the title ~~Fixes method for finding default port~~ Fix: Fixed method for finding default port Sep 5, 2019

Merge branch 'issue-port' of https://github.com/rbrishabh/web-ext int…

302ecd1

…o issue-port

rbrishabh changed the title ~~Fix: Fixed method for finding default port~~ fix: fixed method for finding default port Sep 5, 2019

rpl approved these changes Sep 18, 2019

View reviewed changes

rbrishabh mentioned this pull request Sep 19, 2019

Enable "devtools.browserconsole.contentMessages" before running Firefox #1702

Closed

rbrishabh added 2 commits September 21, 2019 00:04

fix: made requested changes

dcab7ab

fix: fixed port for chromium

ec507cc

rbrishabh commented Sep 20, 2019

View reviewed changes

tests/unit/test-firefox/test.remote.js Outdated Show resolved Hide resolved

tests/unit/test-firefox/test.remote.js Outdated Show resolved Hide resolved

rbrishabh requested a review from rpl September 20, 2019 19:36

rpl requested changes Sep 23, 2019

View reviewed changes

Requested changes

0a5acc3

rpl approved these changes Sep 24, 2019

View reviewed changes

rpl changed the title ~~fix: fixed method for finding default port~~ fix: Choose a random free tcp port to be used for the Firefox Desktop remote debugging server Sep 24, 2019

rpl merged commit f2043c7 into mozilla:master Sep 24, 2019

rbrishabh deleted the issue-port branch September 24, 2019 16:30


		throw new WebExtError('Too many retries on port search');
		export async function defaultRemotePortFinder(): Promise<number> {

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

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

Conversation

rbrishabh commented Jul 28, 2019

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Aug 2, 2019

Rob--W commented Aug 2, 2019

rbrishabh commented Aug 2, 2019

coveralls commented Aug 8, 2019 • edited Loading

rbrishabh commented Aug 8, 2019 • edited Loading

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbrishabh commented Aug 21, 2019

rbrishabh commented Aug 22, 2019

rbrishabh commented Aug 22, 2019 • edited Loading

rbrishabh commented Sep 2, 2019

rpl commented Sep 2, 2019

afk-mario commented Sep 2, 2019

rbrishabh commented Sep 3, 2019

rbrishabh commented Sep 3, 2019

rpl left a comment

Choose a reason for hiding this comment

rbrishabh commented Sep 5, 2019 • edited Loading

rpl commented Sep 5, 2019

rbrishabh commented Sep 5, 2019

rbrishabh commented Sep 16, 2019

rpl commented Sep 16, 2019 • edited Loading

rbrishabh commented Sep 17, 2019

rpl left a comment

Choose a reason for hiding this comment

rbrishabh left a comment

Choose a reason for hiding this comment

rpl left a comment

Choose a reason for hiding this comment

rbrishabh commented Sep 24, 2019

rpl left a comment

Choose a reason for hiding this comment

rbrishabh commented Sep 24, 2019

coveralls commented Aug 8, 2019 •

edited

Loading

rbrishabh commented Aug 8, 2019 •

edited

Loading

rbrishabh commented Aug 22, 2019 •

edited

Loading

rbrishabh commented Sep 5, 2019 •

edited

Loading

rpl commented Sep 16, 2019 •

edited

Loading