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: Make --explicitly-allowed-ports work with NetworkService. #17642

Merged
merged 1 commit into from Apr 2, 2019

Conversation

alexeykuzmin
Copy link
Contributor

@alexeykuzmin alexeykuzmin commented Apr 1, 2019

https://chromium-review.googlesource.com/c/1400042
Landed in 73.0.3666.0, no need to backport to any other version.

Description of Change

Checklist

Release Notes

Notes: Fixed the --explicitly-allowed-ports switch.

@alexeykuzmin alexeykuzmin requested a review from a team as a code owner April 1, 2019 11:23
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 1, 2019
@alexeykuzmin alexeykuzmin requested a review from a team as a code owner April 1, 2019 11:50
@alexeykuzmin alexeykuzmin changed the base branch from master to 4-1-x April 1, 2019 11:50
@alexeykuzmin alexeykuzmin reopened this Apr 1, 2019
@alexeykuzmin alexeykuzmin changed the title backport: Make --explicitly-allowed-ports work with NetworkService. fix: Make --explicitly-allowed-ports work with NetworkService. Apr 1, 2019
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Patch LGTM, Couple of questions for clarification,

  1. This switch never worked in electron because the network code that lives in the browser process didn't get configured with the api as it lived only in the //chrome layer until this patch. So what triggered the backport for this particular release ?

  2. Which brings me to the second question, do we want to fix this for older versions too ? We actually don't need this patch for that case, calling net::SetExplicitlyAllowedPorts based on the switch inside browser process should do the trick.

@alexeykuzmin
Copy link
Contributor Author

alexeykuzmin commented Apr 2, 2019

@deepak1556

This switch never worked in electron

That's basically the reason, I wanted to use it and found out it doesn't work.

I need the flag to explicitly allow some ports on Windows CI machines I use internally at MS.
Some Electron tests start a Node server on a random port and Windows sometimes assigns port 2049 which is in the list of "restricted" ports in Chromium.
https://chromium.googlesource.com/chromium/src/+/master/net/base/port_util.cc#22

When it happens the app crashes and the whole test suite simply stops. Bummer.

do we want to fix this for older versions too ?

I seems like nobody else wants the flag to work, and I don't need it in Electron 3 =)
So I would say there's no need to backport the change anywhere else.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 2, 2019
@jkleinsc jkleinsc requested a review from deepak1556 April 2, 2019 16:18
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Thanks!

@MarshallOfSound MarshallOfSound merged commit dc95941 into 4-1-x Apr 2, 2019
@release-clerk
Copy link

release-clerk bot commented Apr 2, 2019

Release Notes Persisted

Fixed the --explicitly-allowed-ports switch.

@MarshallOfSound MarshallOfSound deleted the fix-explicitly-allowed-ports branch April 2, 2019 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants