Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2562893002:
Adds ScopedPortExceptions to EmbeddedTestServer. (Closed)
|
Created:
4 years ago by rohitrao (ping after 24h) Modified:
4 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRetry until EmbeddedTestServer is listening on a valid port.
The net stack will not send HTTP requests to a specific set of
blacklisted ports, but the OS may assign one of those ports to
the EmbeddedTestServer. This was causing spurious net_unittests
failures on iOS when it happened. This CL modifies the
EmbeddedTestServer to retry until it is assigned a
non-blacklisted port.
BUG=607630
Committed: https://crrev.com/a501df838c21aa2087b0546dfa8ecce12b5fa08c
Cr-Commit-Position: refs/heads/master@{#437602}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Disable by default. #Patch Set 3 : Remove Disable(). #Patch Set 4 : Another approach #Messages
Total messages: 34 (16 generated)
svaldez@chromium.org changed reviewers: + svaldez@chromium.org
https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/embedded_test_server.cc:239: void EmbeddedTestServer::SetPortExceptionEnabled(bool enabled) { Might as well default this to true and then remove the exception for tests that want the INVALID_PORT failure. https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/embedded_test_server.h:307: Stray?
The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated to add the exception by default, with a method to disable it if desired. I didn't include the ability to enable/disable the exception dynamically, because I didn't think any tests would need that. Running this through the trybots now, will update any tests that fail. https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/embedded_test_server.cc (right): https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/embedded_test_server.cc:239: void EmbeddedTestServer::SetPortExceptionEnabled(bool enabled) { On 2016/12/09 15:26:53, svaldez wrote: > Might as well default this to true and then remove the exception for tests that > want the INVALID_PORT failure. Done. https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... File net/test/embedded_test_server/embedded_test_server.h (right): https://codereview.chromium.org/2562893002/diff/1/net/test/embedded_test_serv... net/test/embedded_test_server/embedded_test_server.h:307: On 2016/12/09 15:26:53, svaldez wrote: > Stray? Done.
lgtm, though you'll need someone for an owners stamp.
On 2016/12/09 16:08:53, rohitrao wrote:
> Updated to add the exception by default, with a method to disable it if
desired.
> I didn't include the ability to enable/disable the exception dynamically,
> because I didn't think any tests would need that.
>
> Running this through the trybots now, will update any tests that fail.
Thinking about this more, I'm not sure I expect any tests to fail.
The list of restricted ports is hard-coded in net/base/port_util.cc, and the
EmbeddedTestServer always calls
listen_socket_->ListenWithAddressAndPort("127.0.0.1", 0, 10).
This should mean that there is no way for a test to force the server onto a
restricted port, so no tests that use EmbeddedTestServer can reliably test any
behavior related to restricted ports.
rohitrao@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke for OWNERS approval If no tests currently need the DisableUnsafePortException() method, should I just remove it?
On 2016/12/09 16:30:19, rohitrao wrote: > +mmenke for OWNERS approval > > If no tests currently need the DisableUnsafePortException() method, should I > just remove it? In that case its probably fine, didn't realize that tests didn't add exceptions to the port_util list, and the ones I was thinking of don't actually use ETS, they just hardcode the connection info.
On 2016/12/09 16:30:19, rohitrao wrote: > +mmenke for OWNERS approval > > If no tests currently need the DisableUnsafePortException() method, should I > just remove it? Note that the ERR_UNSAFE_PORT comes from the client side of the connection, not the server side. My best guess (If the errors are coming from tests that use the EmbeddedTestServer) is that we're getting port 0xFFFF, which is the only ephemeral port we don't allow, because WebKit uses it to mean an invalid port number. Maybe if we get that port, try again? I'd rather no use port restriction magic, particularly for ports that aren't safe to use with WebKit.
Removed the Disable() method and updated the code a bit to make it look more like the code in net/test/spawned_test_server/base_test_server.cc.
On 2016/12/09 16:44:06, rohitrao wrote: > Removed the Disable() method and updated the code a bit to make it look more > like the code in net/test/spawned_test_server/base_test_server.cc. Why is are tests explicitly using ports we don't allow HTTP for HTTP connections? This seems weird.
> Note that the ERR_UNSAFE_PORT comes from the client side of the connection,
not
> the server side. My best guess (If the errors are coming from tests that use
> the EmbeddedTestServer) is that we're getting port 0xFFFF, which is the only
> ephemeral port we don't allow, because WebKit uses it to mean an invalid port
> number.
>
> Maybe if we get that port, try again? I'd rather no use port restriction
magic,
> particularly for ports that aren't safe to use with WebKit.
We also disallow:
2049, // nfs
3659, // apple-sasl / PasswordServer
4045, // lockd
6000, // X11
6665, // Alternate IRC [Apple addition]
6666, // Alternate IRC [Apple addition]
6667, // Standard IRC [Apple addition]
6668, // Alternate IRC [Apple addition]
6669, // Alternate IRC [Apple addition]
I suspect that iOS is occasionally picking one of these ports, rather than
handing back 0xFFFF. I could add diagnostic logging into
embedded_test_server.cc to print out when it chooses an invalid port, if you'd
like to be sure of what's going on.
The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/09 16:47:10, rohitrao wrote: > > Note that the ERR_UNSAFE_PORT comes from the client side of the connection, > not > > the server side. My best guess (If the errors are coming from tests that use > > the EmbeddedTestServer) is that we're getting port 0xFFFF, which is the only > > ephemeral port we don't allow, because WebKit uses it to mean an invalid port > > number. > > > > Maybe if we get that port, try again? I'd rather no use port restriction > magic, > > particularly for ports that aren't safe to use with WebKit. > > We also disallow: > 2049, // nfs > > 3659, // apple-sasl / PasswordServer > > 4045, // lockd > > 6000, // X11 > > 6665, // Alternate IRC [Apple addition] > > 6666, // Alternate IRC [Apple addition] > > 6667, // Standard IRC [Apple addition] > > 6668, // Alternate IRC [Apple addition] > > 6669, // Alternate IRC [Apple addition] > > I suspect that iOS is occasionally picking one of these ports, rather than > handing back 0xFFFF. I could add diagnostic logging into > embedded_test_server.cc to print out when it chooses an invalid port, if you'd > like to be sure of what's going on. Ah, right, for some reason, I was thinking that all the others were in the reserved range. Anyhow, could we just keep making sockets until we get a non-reserved one (Could keep the old sockets around - I don't think it's needed, but doesn't hurt to be paranoid). I'm not a big fan of the exceptions magic, and picking a new port is more robust against other changes (Like, say, using a separate network process)
On 2016/12/09 16:47:10, rohitrao wrote:
> > Note that the ERR_UNSAFE_PORT comes from the client side of the connection,
> not
> > the server side. My best guess (If the errors are coming from tests that
use
> > the EmbeddedTestServer) is that we're getting port 0xFFFF, which is the only
> > ephemeral port we don't allow, because WebKit uses it to mean an invalid
port
> > number.
> >
> > Maybe if we get that port, try again? I'd rather no use port restriction
> magic,
> > particularly for ports that aren't safe to use with WebKit.
>
> We also disallow:
> 2049, // nfs
>
> 3659, // apple-sasl / PasswordServer
>
> 4045, // lockd
>
> 6000, // X11
>
> 6665, // Alternate IRC [Apple addition]
>
> 6666, // Alternate IRC [Apple addition]
>
> 6667, // Standard IRC [Apple addition]
>
> 6668, // Alternate IRC [Apple addition]
>
> 6669, // Alternate IRC [Apple addition]
>
> I suspect that iOS is occasionally picking one of these ports, rather than
> handing back 0xFFFF. I could add diagnostic logging into
> embedded_test_server.cc to print out when it chooses an invalid port, if you'd
> like to be sure of what's going on.
Talking with Matt, he thinks that retrying to open a server port until we hit a
valid one might be better.
Looping around in InitializeAndListen might be right
// Don't loop if we explicitly pass this port in.
isValidPort = (port_ != 0);
do {
// lines 95..120
isValidPort |= net::IsPortAllowedForScheme(port_, url:kHttpScheme)
} while (!isValidPort);
> Looping around in InitializeAndListen might be right
>
> // Don't loop if we explicitly pass this port in.
> isValidPort = (port_ != 0);
> do {
> // lines 95..120
> isValidPort |= net::IsPortAllowedForScheme(port_, url:kHttpScheme)
> } while (!isValidPort);
Should I cap it at some max number of tries?
The CQ bit was checked by rohitrao@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated to try up to 5 times.
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Adds ScopedPortExceptions to EmbeddedTestServer. BUG=607630 ========== to ========== Retry until EmbeddedTestServer is listening on a valid port. The net stack will not send HTTP requests to a specific set of blacklisted ports, but the OS may assign one of those ports to the EmbeddedTestServer. This was causing spurious net_unittests failures on iOS when it happened. This CL modifies the EmbeddedTestServer to retry until it is assigned a non-blacklisted port. BUG=607630 ==========
The CQ bit was checked by rohitrao@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from svaldez@chromium.org Link to the patchset: https://codereview.chromium.org/2562893002/#ps60001 (title: "Another approach")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481308932857330,
"parent_rev": "31ed9e4f2dd8460389b9f1bb063e18d781073525", "commit_rev":
"f7ca9b6a095a061dea1b02e8775745b9dd158093"}
Message was sent while issue was closed.
Description was changed from ========== Retry until EmbeddedTestServer is listening on a valid port. The net stack will not send HTTP requests to a specific set of blacklisted ports, but the OS may assign one of those ports to the EmbeddedTestServer. This was causing spurious net_unittests failures on iOS when it happened. This CL modifies the EmbeddedTestServer to retry until it is assigned a non-blacklisted port. BUG=607630 ========== to ========== Retry until EmbeddedTestServer is listening on a valid port. The net stack will not send HTTP requests to a specific set of blacklisted ports, but the OS may assign one of those ports to the EmbeddedTestServer. This was causing spurious net_unittests failures on iOS when it happened. This CL modifies the EmbeddedTestServer to retry until it is assigned a non-blacklisted port. BUG=607630 Review-Url: https://codereview.chromium.org/2562893002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Retry until EmbeddedTestServer is listening on a valid port. The net stack will not send HTTP requests to a specific set of blacklisted ports, but the OS may assign one of those ports to the EmbeddedTestServer. This was causing spurious net_unittests failures on iOS when it happened. This CL modifies the EmbeddedTestServer to retry until it is assigned a non-blacklisted port. BUG=607630 Review-Url: https://codereview.chromium.org/2562893002 ========== to ========== Retry until EmbeddedTestServer is listening on a valid port. The net stack will not send HTTP requests to a specific set of blacklisted ports, but the OS may assign one of those ports to the EmbeddedTestServer. This was causing spurious net_unittests failures on iOS when it happened. This CL modifies the EmbeddedTestServer to retry until it is assigned a non-blacklisted port. BUG=607630 Committed: https://crrev.com/a501df838c21aa2087b0546dfa8ecce12b5fa08c Cr-Commit-Position: refs/heads/master@{#437602} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a501df838c21aa2087b0546dfa8ecce12b5fa08c Cr-Commit-Position: refs/heads/master@{#437602} |
