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

Projects
None yet
4 participants
@alexeykuzmin
Copy link
Contributor

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 electron/wg-upgrades as a code owner Apr 1, 2019

@alexeykuzmin alexeykuzmin force-pushed the fix-explicitly-allowed-ports branch from 96e6eb1 to 33d7604 Apr 1, 2019

@alexeykuzmin alexeykuzmin requested a review from electron/wg-releases as a code owner Apr 1, 2019

@alexeykuzmin alexeykuzmin changed the base branch from master to 4-1-x Apr 1, 2019

@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

@deepak1556
Copy link
Member

deepak1556 left a comment

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

This comment has been minimized.

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.

@alexeykuzmin alexeykuzmin force-pushed the fix-explicitly-allowed-ports branch from 33d7604 to ab25d10 Apr 2, 2019

@electron-cation electron-cation bot removed the new-pr 🌱 label Apr 2, 2019

@jkleinsc jkleinsc requested a review from deepak1556 Apr 2, 2019

@deepak1556
Copy link
Member

deepak1556 left a comment

Thanks!

@MarshallOfSound MarshallOfSound merged commit dc95941 into 4-1-x Apr 2, 2019

8 checks passed

Semantic Pull Request ready to be squashed
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

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 Apr 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.