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
Conversation
96e6eb1
to
33d7604
Compare
There was a problem hiding this 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,
-
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 ?
-
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.
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. When it happens the app crashes and the whole test suite simply stops. Bummer.
I seems like nobody else wants the flag to work, and I don't need it in Electron 3 =) |
33d7604
to
ab25d10
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Release Notes Persisted
|
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
npm test
passesRelease Notes
Notes: Fixed the --explicitly-allowed-ports switch.