Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2512263002:
Pass raw response with Context-Encoding sdch if support is not configured. (Closed)
DescriptionPass raw response with Context-Encoding sdch if support is not configured.
BUG=666936
Committed: https://crrev.com/8c7da89358dd21e60be2725220696f29ef94c234
Cr-Commit-Position: refs/heads/master@{#435295}
Patch Set 1 #Patch Set 2 : Added tests for explicit Accept-Encoding header value. #Patch Set 3 : Fix net unittests. #
Total comments: 13
Patch Set 4 : Address Helen's comments. #
Messages
Total messages: 18 (10 generated)
Description was changed from ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG= ========== to ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG=666936 ==========
mef@chromium.org changed reviewers: + xunjieli@chromium.org
I am fine with this approach, given that this is the behavior before the filter refactoring. I am a bit concerned about masking the error though. In the longer term, CrNet could use the same check as Cronet on Android to prevent clients from adding Accept-Encodings. I think now URLRequestHttpJobSetUpSourceTest.SdchNotAdvertisedGotMalformedSdchResponse will fail. Could you update that test? +rdsmith, FHI.
Thanks, Helen! I've fixed the test without SdchManager, and I've added another one with SdchManager, but I don't see decoding error neither with nor without my change. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:811: // EXPECT_EQ(ERR_CONTENT_DECODING_INIT_FAILED, delegate.request_status()); I would've expected an error if SDCH manager is present, but server response doesn't specify valid dictionary. Currently it passes through raw response data with or without my change to URLRequestHttpJob::SetUpSourceStream(), so I don't think that this CL masks any errors.
Looks good. thanks for handling this. I will sign off after another pass. https://codereview.chromium.org/2512263002/diff/40001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2512263002/diff/40001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:199: TEST_F(HttpTest, AcceptEncodingSdch) { I think this deserves a comment and maybe also mention the fact that in the future we might remove the ability of adding "Accept-Encoding". https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:165: SdchNotAdvertisedGotMalformedSdchResponse) { Could you change the name of this test to something like: SdchNotAdvertisedGotSdchResponse ? https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:187: // Pass through the raw response tje same way as if received unknown encoding. nit: typo in "the" https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:781: // Received a malformed SDCH encoded response when there is no SdchManager. Could you adjust this comment to something like: // Received a malformed SDCH encoded response that has no valid dictionary id. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:811: // EXPECT_EQ(ERR_CONTENT_DECODING_INIT_FAILED, delegate.request_status()); On 2016/11/29 22:51:51, mef wrote: > I would've expected an error if SDCH manager is present, but server response > doesn't specify valid dictionary. > > Currently it passes through raw response data with or without my change to > URLRequestHttpJob::SetUpSourceStream(), so I don't think that this CL masks any > errors. This is expected. SdchPolicyDelegate::OnDictionaryIdError() once detects that the response is malformed, it will issue a pass-through, so we end up receiving the raw response. As you can see here, we already masking quite a few errors in sdch, so this won't be a precedent. The one that I mentioned in my previous CL is about Cronet clients changing Accept-Encodings, which I think can be problematic depending on what exactly Cronet clients do. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:812: // Pass through the raw response tje same way as if received unknown encoding. Could you remove this line and the line above? Maybe mention the fact that the malformed response is passed through undecoded.
The CQ bit was checked by mef@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...
Thanks, PTAL. https://codereview.chromium.org/2512263002/diff/40001/components/cronet/ios/t... File components/cronet/ios/test/cronet_http_test.mm (right): https://codereview.chromium.org/2512263002/diff/40001/components/cronet/ios/t... components/cronet/ios/test/cronet_http_test.mm:199: TEST_F(HttpTest, AcceptEncodingSdch) { On 2016/11/29 23:30:42, xunjieli wrote: > I think this deserves a comment and maybe also mention the fact that in the > future we might remove the ability of adding "Accept-Encoding". Done. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... File net/url_request/url_request_http_job_unittest.cc (right): https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:165: SdchNotAdvertisedGotMalformedSdchResponse) { On 2016/11/29 23:30:42, xunjieli wrote: > Could you change the name of this test to something like: > > SdchNotAdvertisedGotSdchResponse ? Done. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:187: // Pass through the raw response tje same way as if received unknown encoding. On 2016/11/29 23:30:42, xunjieli wrote: > nit: typo in "the" Done. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:781: // Received a malformed SDCH encoded response when there is no SdchManager. On 2016/11/29 23:30:42, xunjieli wrote: > Could you adjust this comment to something like: > > // Received a malformed SDCH encoded response that has no valid dictionary id. Done. https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:811: // EXPECT_EQ(ERR_CONTENT_DECODING_INIT_FAILED, delegate.request_status()); On 2016/11/29 23:30:42, xunjieli wrote: > On 2016/11/29 22:51:51, mef wrote: > > I would've expected an error if SDCH manager is present, but server response > > doesn't specify valid dictionary. > > > > Currently it passes through raw response data with or without my change to > > URLRequestHttpJob::SetUpSourceStream(), so I don't think that this CL masks > any > > errors. > > This is expected. SdchPolicyDelegate::OnDictionaryIdError() once detects that > the response is malformed, it will issue a pass-through, so we end up receiving > the raw response. > > As you can see here, we already masking quite a few errors in sdch, so this > won't be a precedent. The one that I mentioned in my previous CL is about Cronet > clients changing Accept-Encodings, which I think can be problematic depending on > what exactly Cronet clients do. Acknowledged. Thanks for explanation! https://codereview.chromium.org/2512263002/diff/40001/net/url_request/url_req... net/url_request/url_request_http_job_unittest.cc:812: // Pass through the raw response tje same way as if received unknown encoding. On 2016/11/29 23:30:42, xunjieli wrote: > Could you remove this line and the line above? > Maybe mention the fact that the malformed response is passed through undecoded. Done.
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 mef@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": 60001, "attempt_start_ts": 1480524784708210,
"parent_rev": "00c13465343fe426bda33b717686d7b0d9896430", "commit_rev":
"7ceacadd8f4c71061e7697f450f2991c2e2b6bad"}
Message was sent while issue was closed.
Description was changed from ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG=666936 ========== to ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG=666936 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG=666936 ========== to ========== Pass raw response with Context-Encoding sdch if support is not configured. BUG=666936 Committed: https://crrev.com/8c7da89358dd21e60be2725220696f29ef94c234 Cr-Commit-Position: refs/heads/master@{#435295} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8c7da89358dd21e60be2725220696f29ef94c234 Cr-Commit-Position: refs/heads/master@{#435295} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
