Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2192193002:
Remove a DCHECK on ClientSocketHandle state from proxy code. (Closed)
DescriptionRemove a DCHECK on ClientSocketHandle state from proxy code.
This was causing issues for one of our fuzzers.
The DCHECK made sure the ClientSocketHandle was initialized, which was
not the state in tests. Normally, handles set initialized to true
when they are assigned a socket, and the connection callback is invoked.
When the Handle is torn down, if the Handle was initialized, the socket
is returned to the socket pool. A lot of tests bypass all this, not
using socket pools at all, and just assigning a socket to the
SocketHandle. This results in is_connected being false, which was
triggering the DCHECK.
We could instead make sure that is_initialized is set to true in tests,
but this has minimal value - in production, it's set to true if and only
if a socket is set and the callback invoked, and set to false only when
the socket is destroyed or returned to the socket pool. If the socket is
null, we'll crash very soon with an equally useful crash stack, anyways.
In production, if the connection callback wasn't invoked, the
HttpProxyClientSocket's state machine will catch the issue, anyways.
BUG=632608
Committed: https://crrev.com/893b812617b63d2b71e38bf2a504d2c5d4657c23
Cr-Commit-Position: refs/heads/master@{#408670}
Patch Set 1 #
Messages
Total messages: 15 (10 generated)
The CQ bit was checked by mmenke@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...
Description was changed from ========== Remove a DCHECK on ClientSocketHandle state from proxy code. The DCHECK makes sure the ClientSocketHandle was initialized, which is not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ========== to ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK makes sure the ClientSocketHandle was initialized, which is not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ==========
Description was changed from ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK makes sure the ClientSocketHandle was initialized, which is not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ========== to ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK made sure the ClientSocketHandle was initialized, which was not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ==========
mmenke@chromium.org changed reviewers: + eroman@chromium.org
Eric: Mind reviewing? Asanka's off. Description's kind of long, but the TLDR is the DCHECK doesn't get us much in production, and tests that manually set a ClientSocketHandle's socket don't initialize the ClientSocketHandle, so the DCHECK fails in one of the fuzzers. A lot of tests do this, they just don't test enough code to run into this DCHECK. We could modify ClientSocketHandle so that is_initialized_ could be set when a SocketPool isn't in use, but that seems more effort than it's worth. I think it may make more sense to work towards getting rid of is_initialized, acutally - it's only used for a couple DCHECKs, and by a bunch of tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by mmenke@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK made sure the ClientSocketHandle was initialized, which was not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ========== to ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK made sure the ClientSocketHandle was initialized, which was not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK made sure the ClientSocketHandle was initialized, which was not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 ========== to ========== Remove a DCHECK on ClientSocketHandle state from proxy code. This was causing issues for one of our fuzzers. The DCHECK made sure the ClientSocketHandle was initialized, which was not the state in tests. Normally, handles set initialized to true when they are assigned a socket, and the connection callback is invoked. When the Handle is torn down, if the Handle was initialized, the socket is returned to the socket pool. A lot of tests bypass all this, not using socket pools at all, and just assigning a socket to the SocketHandle. This results in is_connected being false, which was triggering the DCHECK. We could instead make sure that is_initialized is set to true in tests, but this has minimal value - in production, it's set to true if and only if a socket is set and the callback invoked, and set to false only when the socket is destroyed or returned to the socket pool. If the socket is null, we'll crash very soon with an equally useful crash stack, anyways. In production, if the connection callback wasn't invoked, the HttpProxyClientSocket's state machine will catch the issue, anyways. BUG=632608 Committed: https://crrev.com/893b812617b63d2b71e38bf2a504d2c5d4657c23 Cr-Commit-Position: refs/heads/master@{#408670} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/893b812617b63d2b71e38bf2a504d2c5d4657c23 Cr-Commit-Position: refs/heads/master@{#408670} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
