Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2771413004:
DRP: Add check that ECT header is present when CPAT header is present. (Closed)
DescriptionDRP: Add check that ECT header is present when CPAT header is present.
Add a (d)check in data reduction proxy (DRP) code which verifies that
if the CPAT header is present in the request headers, then ECT header
must be present too.
Other checks added: e.g., chrome-proxy header should only
be sent when the resolved proxy is a data saver proxy.
Finally, the CL also modifies few unittests to avoid tripping the
DCHECKs.
BUG=706650
Review-Url: https://codereview.chromium.org/2771413004
Cr-Commit-Position: refs/heads/master@{#460609}
Committed: https://chromium.googlesource.com/chromium/src/+/bd690b0bbec9e12f8554673c5501a36e4ec2d7b8
Patch Set 1 #
Total comments: 4
Patch Set 2 : dougarnett comments #
Total comments: 4
Patch Set 3 : megjablon comments #
Messages
Total messages: 27 (18 generated)
Description was changed from ========== Test to make sure ECT header is always present. BUG=691135 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. BUG=691135 ==========
tbansal@chromium.org changed reviewers: + dougarnett@chromium.org, megjablon@chromium.org
megjablon, dougarnett: ptal. Thanks.
https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:118: // Verifies that the chrome proxy related request headers are correctly set. correctly => consistently ? Nothing passed in indicates what is correct. https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:119: void VerifyHttpRequestHeaders(const net::HttpRequestHeaders& headers) { Seems hard for readability where used as to intent. Also sounds like it may do more than it does. Consider more explicit name with intent arg like VerifyProxyHeader(bool has_proxy_header, headers)
Description was changed from ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. BUG=691135 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Also, add some additional checks: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. BUG=691135 ==========
ptal. Thanks. https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:118: // Verifies that the chrome proxy related request headers are correctly set. On 2017/03/28 22:07:27, dougarnett wrote: > correctly => consistently ? > > Nothing passed in indicates what is correct. I added more checks to make it more general. Also, passing the intent. So, it looks more like a "correctness" check. https://codereview.chromium.org/2771413004/diff/1/components/data_reduction_p... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:119: void VerifyHttpRequestHeaders(const net::HttpRequestHeaders& headers) { On 2017/03/28 22:07:27, dougarnett wrote: > Seems hard for readability where used as to intent. Also sounds like it may do > more than it does. Consider more explicit name with intent arg like > > VerifyProxyHeader(bool has_proxy_header, headers) Done.
lgtm Thanks, I like that better.
Description was changed from ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Also, add some additional checks: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. BUG=691135 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Also, add some additional checks: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Some tests had to be modified to avoid tripping the DCHECKs. BUG=691135 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Description was changed from ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Also, add some additional checks: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Some tests had to be modified to avoid tripping the DCHECKs. BUG=691135 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Other checks added: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Finally, the CL also modifies few unittests to avoid tripping the DCHECKs. BUG=691135 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:121: void VerifyHttpRequestHeaders(bool via_chrome_proxy, Capitalize Data Saver? https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:636: network_delegate()->NotifyBeforeStartTransaction( Why are these NotifyBeforeStartTransaction calls necessary in the tests? Add a comment?
megjablon: ptal. Thanks. https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc (right): https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:121: void VerifyHttpRequestHeaders(bool via_chrome_proxy, On 2017/03/29 22:20:01, megjablon wrote: > Capitalize Data Saver? Done. https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... File components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc (right): https://codereview.chromium.org/2771413004/diff/20001/components/data_reducti... components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:636: network_delegate()->NotifyBeforeStartTransaction( On 2017/03/29 22:20:01, megjablon wrote: > Why are these NotifyBeforeStartTransaction calls necessary in the tests? Add a > comment? Done. ECT/CPAT headers are added in NotifyBeforeStartTransaction. To avoid tripping the DCHECK, it is now required to call NotifyBeforeStartTransaction().
The CQ bit was checked by tbansal@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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dougarnett@chromium.org Link to the patchset: https://codereview.chromium.org/2771413004/#ps40001 (title: "megjablon comments")
The CQ bit was unchecked by tbansal@chromium.org
Description was changed from ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Other checks added: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Finally, the CL also modifies few unittests to avoid tripping the DCHECKs. BUG=691135 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Other checks added: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Finally, the CL also modifies few unittests to avoid tripping the DCHECKs. BUG=706650 ==========
The CQ bit was checked by tbansal@chromium.org
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": 40001, "attempt_start_ts": 1490835161800530,
"parent_rev": "84f5e2800a49ce3de9f877ea842b104d0484bd52", "commit_rev":
"bd690b0bbec9e12f8554673c5501a36e4ec2d7b8"}
Message was sent while issue was closed.
Description was changed from ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Other checks added: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Finally, the CL also modifies few unittests to avoid tripping the DCHECKs. BUG=706650 ========== to ========== DRP: Add check that ECT header is present when CPAT header is present. Add a (d)check in data reduction proxy (DRP) code which verifies that if the CPAT header is present in the request headers, then ECT header must be present too. Other checks added: e.g., chrome-proxy header should only be sent when the resolved proxy is a data saver proxy. Finally, the CL also modifies few unittests to avoid tripping the DCHECKs. BUG=706650 Review-Url: https://codereview.chromium.org/2771413004 Cr-Commit-Position: refs/heads/master@{#460609} Committed: https://chromium.googlesource.com/chromium/src/+/bd690b0bbec9e12f8554673c5501... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/bd690b0bbec9e12f8554673c5501... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
