[net/cache] Avoid the cache for responses exceeding 2GB.

[net/cache] Skip the cache if the response is expected to exceed 2GB.

The state machine in http_cache_transaction doesn't handle partial
requests that are larger than numeric_limits<int32_t>::max(). If there
is a truncated partial cache entry for a large response, then the cache
will try to issue partial requests of size at most
numeric_limits<int32_t>::max() bytes until the entire resource is
fetched.

However, if StopCaching() is called, as it often is for response of this
size that are downloaded, then the cache transaction switches to pass
through mode. This trips a bug in the state machine where the response
would be considered complete once the current partial request completes.

There are multiple issues in the cache transaction that needs
addressing, but for now we are going to skip the cache entirely if we
are likely to hit this case. The partial cache entry is most likely the
result of a MIME sniffing attempt and would be insignificant compared to
the resource size. Hence skipping the cache in this case isn't expected
to regress on performance.

This change adds several unit tests that drive a request with a response
size of 5GB. These tests are expected to be slower than the average net
unit test, but shouldn't cause trouble for the bots.

BUG=89567
Committed: https://crrev.com/b5fb4b4394a9c60ee711ee9f434072a7e7aee513
Cr-Commit-Position: refs/heads/master@{#402774}

[asanka, 2016-06-22 02:12:38]

asanka@chromium.org changed reviewers:
+ mmenke@chromium.org

[asanka, 2016-06-22 02:15:46]

mmenke: Would you do the honors? Some background in
https://bugs.chromium.org/p/chromium/issues/detail?id=89567#c132

The longer term fix would be something like
https://codereview.chromium.org/1230113012 but in general our cache API is not
equipped to deal with such huge cache entries anyway.

[mmenke, 2016-06-22 15:50:25]

mmenke@chromium.org changed reviewers:
+ gavinp@chromium.org

[mmenke, 2016-06-22 15:50:26]

[+gavinp]

Some quick comments, but I think Gavin would be a better reviewer here - I
really don't know this class's state machine.  I'd love to take this opportunity
to learn it a bit better, but I don't have the time this week.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1178: const int64_t full_response_length =
response_.headers->GetContentLength();
nit:  const is generally not used for runtime constants in chrome, except for
class members (x2).

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1184: // The SM's handling of StopCaching
unfortunately doesn't deal well with
Please don't use uncommon abbreviations in comments (x2).  I assume "SM" means
"state machine", rather than "salty mackerel", but that wasn't immediately
obvious.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1192: // Does not yield the cache lock. If
another transaction starts for this
"Does not yield the cache lock" -> "Does not yield to the cache lock"?

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1194: // fallback to the network after the
timeout.
fallback -> fall back ("fallback" is a noun)

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1195: DCHECK(!partial_.get());
nit:  .get() not needed.

[asanka, 2016-06-22 16:31:12]

Patchset #3 (id:40001) has been deleted

[asanka, 2016-06-22 16:31:22]

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1178: const int64_t full_response_length =
response_.headers->GetContentLength();
On 2016/06/22 at 15:50:26, mmenke wrote:
> nit:  const is generally not used for runtime constants in chrome, except for
class members (x2).

Done.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1184: // The SM's handling of StopCaching
unfortunately doesn't deal well with
On 2016/06/22 at 15:50:26, mmenke wrote:
> Please don't use uncommon abbreviations in comments (x2).  I assume "SM" means
"state machine", rather than "salty mackerel", but that wasn't immediately
obvious.

I was trying to be cool and use abbreviations found elsewhere in the source
file. :P Reworded.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1192: // Does not yield the cache lock. If
another transaction starts for this
On 2016/06/22 at 15:50:25, mmenke wrote:
> "Does not yield the cache lock" -> "Does not yield to the cache lock"?

Reworded to say 'Does not release the cache entry'.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1194: // fallback to the network after the
timeout.
On 2016/06/22 at 15:50:26, mmenke wrote:
> fallback -> fall back ("fallback" is a noun)

Done.

https://codereview.chromium.org/2089783002/diff/20001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:1195: DCHECK(!partial_.get());
On 2016/06/22 at 15:50:26, mmenke wrote:
> nit:  .get() not needed.

Done.

[gavinp, 2016-06-28 14:24:43]

lgtm, none of my nits are serious.

There's a theme around casting and the 2GB barrier lately.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7100: 5000000000LL;  // Five beeeeeelllliooonn
bytes!
Nit: can we write this as 5000 * 1000 * 1000 just to make it quicker to read?
Then we can also remove the colloquial comment, even!

(Avoid colloquial stuff.)

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7103: const int64_t
HttpCacheHugeResourceTest::kTotalSize;
Is this necessary?

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7116: response_status->assign("HTTP/1.1 200
Success");
Nit: I know this was probably cut and paste, but you could just use the
assignment operator here.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7295: // This test simulates reading Gigabytes
of data. Buffer size is set to 10MB
Nit: I don't believe we need to capitalise gigabytes here.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7296: // to reduce the number of reads and
speedup the test.
Nit: speedup is a noun, I believe you want "speed up" here.

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:30: const int kMaxMockCacheEntrySize = 100000000;
Nit: I have trouble counting zeroes this quickly. Could we write this as 100 *
1000 * 1000, for speedy reading?

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:152: DCHECK(offset + buf_len <
kMaxMockCacheEntrySize);
DCHECK_LT?

[mmenke, 2016-06-28 14:31:18]

nit:  Your description is wrong.  This CL avoids the cache for responses that
are at least 2GB, not greater than 2GB.  (Yea, not exactly a big deal, but...)

[asanka, 2016-06-28 16:52:41]

Description was changed from

==========
[net/cache] Avoid the cache for responses exceeding 2GB.

BUG=89567
==========

to

==========
[net/cache] Skip the cache if the response is expected to exceed 2GB.

The state machine in http_cache_transaction doesn't handle partial
requests that are larger than numeric_limits<int32_t>::max(). If there
is a truncated partial cache entry for a large response, then the cache
will try to issue partial requests of size at most
numeric_limits<int32_t>::max() bytes until the entire resource is
fetched.

However, if StopCaching() is called, as it often is for response of this
size that are downloaded, then the cache transaction switches to pass
through mode. This trips a bug in the state machine where the response
would be considered complete once the current partial request completes.

There are multiple issues in the cache transaction that needs
addressing, but for now we are going to skip the cache entirely if we
are likely to hit this case. The partial cache entry is most likely the
result of a MIME sniffing attempt and would be insignificant compared to
the resource size. Hence skipping the cache in this case isn't expected
to regress on performance.

This change adds several unit tests that drive a request with a response
size of 5GB. These tests are expected to be slower than the average net
unit test, but shouldn't cause trouble for the bots.

BUG=89567
==========

[asanka, 2016-06-28 16:55:38]

Thanks!

mmenke, gavinp: I updated the CL description to be more.. descriptive. PTAL?

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7100: 5000000000LL;  // Five beeeeeelllliooonn
bytes!
On 2016/06/28 at 14:24:43, gavinp wrote:
> Nit: can we write this as 5000 * 1000 * 1000 just to make it quicker to read?
Then we can also remove the colloquial comment, even!
> 
> (Avoid colloquial stuff.)

Rewritten as 5'00'00'00'000LL :P

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7103: const int64_t
HttpCacheHugeResourceTest::kTotalSize;
On 2016/06/28 at 14:24:43, gavinp wrote:
> Is this necessary?

Apparently EXPECT_EQ(kTotalSize, ...) results in a reference to the kTotalSize,
which requires kTotalSize to be defined.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7116: response_status->assign("HTTP/1.1 200
Success");
On 2016/06/28 at 14:24:43, gavinp wrote:
> Nit: I know this was probably cut and paste, but you could just use the
assignment operator here.

Done.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7295: // This test simulates reading Gigabytes
of data. Buffer size is set to 10MB
On 2016/06/28 at 14:24:43, gavinp wrote:
> Nit: I don't believe we need to capitalise gigabytes here.

Done.

https://codereview.chromium.org/2089783002/diff/60001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:7296: // to reduce the number of reads and
speedup the test.
On 2016/06/28 at 14:24:43, gavinp wrote:
> Nit: speedup is a noun, I believe you want "speed up" here.

Done.

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
File net/http/mock_http_cache.cc (right):

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:30: const int kMaxMockCacheEntrySize = 100000000;
On 2016/06/28 at 14:24:43, gavinp wrote:
> Nit: I have trouble counting zeroes this quickly. Could we write this as 100 *
1000 * 1000, for speedy reading?

Done. :-)

https://codereview.chromium.org/2089783002/diff/60001/net/http/mock_http_cach...
net/http/mock_http_cache.cc:152: DCHECK(offset + buf_len <
kMaxMockCacheEntrySize);
On 2016/06/28 at 14:24:43, gavinp wrote:
> DCHECK_LT?

Done here and on L223.

[asanka, 2016-06-29 11:01:34]

The CQ bit was checked by asanka@chromium.org

[asanka, 2016-06-29 11:01:34]

The patchset sent to the CQ was uploaded after l-g-t-m from gavinp@chromium.org
Link to the patchset: https://codereview.chromium.org/2089783002/#ps100001
(title: "Fix build for configurations where DCHECK is disabled.")

[commit-bot: I haz the power, 2016-06-29 11:02:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-29 11:06:24]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-06-29 11:08:02]

Description was changed from

==========
[net/cache] Skip the cache if the response is expected to exceed 2GB.

The state machine in http_cache_transaction doesn't handle partial
requests that are larger than numeric_limits<int32_t>::max(). If there
is a truncated partial cache entry for a large response, then the cache
will try to issue partial requests of size at most
numeric_limits<int32_t>::max() bytes until the entire resource is
fetched.

However, if StopCaching() is called, as it often is for response of this
size that are downloaded, then the cache transaction switches to pass
through mode. This trips a bug in the state machine where the response
would be considered complete once the current partial request completes.

There are multiple issues in the cache transaction that needs
addressing, but for now we are going to skip the cache entirely if we
are likely to hit this case. The partial cache entry is most likely the
result of a MIME sniffing attempt and would be insignificant compared to
the resource size. Hence skipping the cache in this case isn't expected
to regress on performance.

This change adds several unit tests that drive a request with a response
size of 5GB. These tests are expected to be slower than the average net
unit test, but shouldn't cause trouble for the bots.

BUG=89567
==========

to

==========
[net/cache] Skip the cache if the response is expected to exceed 2GB.

The state machine in http_cache_transaction doesn't handle partial
requests that are larger than numeric_limits<int32_t>::max(). If there
is a truncated partial cache entry for a large response, then the cache
will try to issue partial requests of size at most
numeric_limits<int32_t>::max() bytes until the entire resource is
fetched.

However, if StopCaching() is called, as it often is for response of this
size that are downloaded, then the cache transaction switches to pass
through mode. This trips a bug in the state machine where the response
would be considered complete once the current partial request completes.

There are multiple issues in the cache transaction that needs
addressing, but for now we are going to skip the cache entirely if we
are likely to hit this case. The partial cache entry is most likely the
result of a MIME sniffing attempt and would be insignificant compared to
the resource size. Hence skipping the cache in this case isn't expected
to regress on performance.

This change adds several unit tests that drive a request with a response
size of 5GB. These tests are expected to be slower than the average net
unit test, but shouldn't cause trouble for the bots.

BUG=89567

Committed: https://crrev.com/b5fb4b4394a9c60ee711ee9f434072a7e7aee513
Cr-Commit-Position: refs/heads/master@{#402774}
==========

[commit-bot: I haz the power, 2016-06-29 11:08:03]

Patchset 5 (id:??) landed as
https://crrev.com/b5fb4b4394a9c60ee711ee9f434072a7e7aee513
Cr-Commit-Position: refs/heads/master@{#402774}