Issue 56043: Use HTTP status return code to make SDCH handling more robust...

Issue 56043: Use HTTP status return code to make SDCH handling more robust... (Closed)

Created:
11 years, 9 months ago by jar (doing other things)

Modified:
9 years, 7 months ago

Reviewers:
huanr, Lincoln, wtc

CC:
chromium-reviews_googlegroups.com, kmixter

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Use HTTP status return code to make SDCH handling more robust. At least one proxy replaces the SDCH content with an error page (of HTML, without SDCH compression). We can identify that scenario by spotting the 40x HTTP return code (and the fact that the content is not SDCH encoded, even though we advertised SDCH and a dictionary to the server). This change list adds the ability to access the return code via the FilterContext. The bulk of the change is centered on getting that access method to be const in all derived classes. bug=8916 r=wtc,huanr,openvcdiff Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=12784

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Patch Set 7 : '' #

Total comments: 23

Patch Set 8 : '' #

Patch Set 9 : '' #

Total comments: 2

Patch Set 10 : '' #

Created: 11 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+224 lines, -17 lines)			Patch
M	chrome/common/net/url_request_intercept_job.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/common/net/url_request_intercept_job.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	net/base/filter.h	View	1 2 3 4 5 6 7	1 chunk	+4 lines, -0 lines	0 comments	Download
M	net/base/filter_unittest.h	View	1 2 3 4 5 6 7	4 chunks	+6 lines, -1 line	0 comments	Download
M	net/base/gzip_filter_unittest.cc	View	5 6 7	1 chunk	+3 lines, -0 lines	0 comments	Download
M	net/base/sdch_filter.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+23 lines, -4 lines	0 comments	Download
M	net/base/sdch_filter_unittest.cc	View	3 4 5 6 7 8	3 chunks	+180 lines, -4 lines	0 comments	Download
M	net/url_request/url_request_http_job.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	net/url_request/url_request_http_job.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	net/url_request/url_request_job.h	View	1 2 3 4 5 6 7	2 chunks	+2 lines, -2 lines	0 comments	Download
M	net/url_request/url_request_new_ftp_job.h	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download
M	net/url_request/url_request_new_ftp_job.cc	View	1 2 3 4 5 6 7	1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

jar (doing other things)

Many files... but actually a small issue (mostly one line changes in files). Wtc: Please ...

11 years, 9 months ago (2009-03-28 21:14:48 UTC) #1

Lincoln

I'm assuming you meant GetResponseCode() rather than GetStatusCode() in your review request comments. http://codereview.chromium.org/56043/diff/2001/81 File ...

11 years, 8 months ago (2009-03-30 15:18:07 UTC) #2

wtc

LGTM. I mainly reviewed the portions that move GetResponseCode to the FilterContext interface and make ...

11 years, 8 months ago (2009-03-30 17:28:30 UTC) #3

jar (doing other things)

Comments and changes per lincoln's suggestions and corrections. Nice job Lincoln!! http://codereview.chromium.org/56043/diff/2001/81 File net/base/gzip_filter_unittest.cc (right): ...

11 years, 8 months ago (2009-03-30 17:39:47 UTC) #4

Comments and changes per lincoln's suggestions and corrections.

Nice job Lincoln!!

http://codereview.chromium.org/56043/diff/2001/81
File net/base/gzip_filter_unittest.cc (right):

http://codereview.chromium.org/56043/diff/2001/81#newcode283
Line 283: // error page (As is done by BlueCoat proxies :-/ ).
On 2009/03/30 15:18:08, Lincoln wrote:
> If the reader wants more details on this behavior, will he/she be able to find
> the related bug report through the patch set?  It might be helpful to provide
a
> link to more information in the comment.

Done.

http://codereview.chromium.org/56043/diff/2001/77
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/56043/diff/2001/77#newcode231
Line 231: if (possible_pass_through_&& successful_response) {
On 2009/03/30 17:28:30, wtc wrote:
> Nit: add a space before &&.

Done.

http://codereview.chromium.org/56043/diff/2001/77#newcode251
Line 251: SdchManager::BlacklistDomain(url_);  // Get yet another factor of 2.
On 2009/03/30 15:18:08, Lincoln wrote:
> Is this change related to the change that checks the response code, or could
it
> be submitted separately?  I would prefer to see the interface to


> BlacklistDomain() changed (to specify the amount of back-off) rather than see
> this function make the assumption that calling BlacklistDomain() twice has a
> particular undocumented effect.


Good suggestionS.  I pulled it out of this CL, and will use an argument when I
do land it.

http://codereview.chromium.org/56043/diff/2001/77#newcode253
Line 253: // We don't have the dictionary that was demanded.
On 2009/03/30 15:18:08, Lincoln wrote:
> This sentence no longer describes 100% of the cases that fall into the "else"
> clause.  You should either create a new "if" clause for "!successful_response"
> or update the comment.

I updated the comment.

http://codereview.chromium.org/56043/diff/2001/80
File net/base/sdch_filter_unittest.cc (right):

http://codereview.chromium.org/56043/diff/2001/80#newcode150
Line 150: TEST_F(SdchFilterTest, EmptyInputOk) {
On 2009/03/30 15:18:08, Lincoln wrote:
> Thanks for breaking up the test case!

Done.

http://codereview.chromium.org/56043/diff/2001/80#newcode178
Line 178: filter_context.SetResponseCode(200);
On 2009/03/30 15:18:08, Lincoln wrote:
> No SetMimeType() for these cases?

With the tentative case, I'm willing to pass through any mime type.  Basically,
the content-encoding *didn't* say sdch, but I'm trying to "sniff out" SDCH in
case it was there.  When it is not there, I just pass through, and I don't care
if it is text/html or other.  By default, it is not text/html.  I could provide
yet another test case to prove that in the text/html I *still* pass through...
but this case seemed to cover the situation.

http://codereview.chromium.org/56043/diff/2001/80#newcode203
Line 203: EXPECT_TRUE(sizeof(output_buffer) > output_bytes_or_buffer_size);
On 2009/03/30 15:18:08, Lincoln wrote:
> This should be ASSERT_TRUE.  The following line might cause a segmentation
fault
> if this condition is false.

<DOH>  good catch... I meant to come back and change this (after looking up the
exact macro to use)... but I forgot to come back.

http://codereview.chromium.org/56043/diff/2001/80#newcode243
Line 243: // We should have read a long and complicated meta-refresh request.
On 2009/03/30 15:18:08, Lincoln wrote:
> Is this comment valid?  Unlike ErrorOnBadReturnCode(), I don't see this test
> checking for the contents of the meta-refresh request.

Comment is incorrect.  This case is the pass-through case, and I didn't modify
the comment correctly as I mutated the test.

http://codereview.chromium.org/56043/diff/2001/80#newcode285
Line 285: "<head><META HTTP-EQUIV=\"Refresh\" CONTENT=\"0\"></head>",
On 2009/03/30 15:18:08, Lincoln wrote:
> Why does this test expect a meta-refresh request if the MIME type is
"anything"
> rather than "text/html"?  Shouldn't the meta-refresh appear only for
text/html?

Excellent catch.  As per comments, and your comment, this should NOT return meta
refresh.  There was a bug in the filter using FindFirstOf rather than Find.

I corrected the implementation bug, and broke this test case into two parts, one
with and one without HTML context.

thanks!

http://codereview.chromium.org/56043/diff/2001/80#newcode300
Line 300: // Supply bogus data (which doesnt't yet specify a full dictionary
hash).
On 2009/03/30 15:18:08, Lincoln wrote:
> typo: doesnt't => doesn't

Done.

huanr

LGTM http://codereview.chromium.org/56043/diff/2005/106 File net/base/sdch_filter.cc (right): http://codereview.chromium.org/56043/diff/2005/106#newcode249 Line 249: } else { The addition of successful_response ...

11 years, 8 months ago (2009-03-30 17:56:14 UTC) #6

jar (doing other things)

Per discussion with Hunar, added additional comments to the flow in the error recovery portion ...

11 years, 8 months ago (2009-03-30 18:00:09 UTC) #7

jar (doing other things)

11 years, 8 months ago (2009-03-30 19:22:55 UTC) #8

Post landing responses.

http://codereview.chromium.org/56043/diff/2001/73
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/56043/diff/2001/73#newcode108
Line 108: virtual int GetResponseCode() const { return -1; }
On 2009/03/30 17:28:30, wtc wrote:
> Move this method down to the "FilterContext methods" section
> below.

Done (in separate tiny secondary CL)

http://codereview.chromium.org/56043/diff/2005/106
File net/base/sdch_filter.cc (right):

http://codereview.chromium.org/56043/diff/2005/106#newcode249
Line 249: } else {
On 2009/03/30 17:56:14, huanr wrote:
> The addition of successful_response makes the condition of this else clause
> complicated. It would be helpful to comment that this is the most expensive
> recovery path.

Done.

Expand Messages | Collapse Messages