Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(314)

Issue 23710059: Release the cache entry on deferred redirect. (Closed)

Created:
7 years, 3 months ago by davidben
Modified:
7 years, 3 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Release the cache entry on deferred redirect. If a redirect is deferred in the warm cache case, the HTTP transaction currently doesn't release the cache entry until after the redirect continues. This prevents other requests from accessing the cache entry. Reuse the DoneReading() signal for the URLRequestJob to notify the transaction that the response body is to be discarded. This matches the redirect-specific logic in HttpCache::Transaction which does not cache redirect bodies. In addition, now that this signal exists at a higher level, remove that logic in HttpCache::Transaction. It is now the caller's job to decide which response bodies are and aren't truncated away. Fixup and add new tests for this behavior. BUG=292879 TEST=HttpCache.CachedRedirect, HttpCache.DoneReading, URLRequestJob.RedirectTransactionNotifiedWhenDone Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224161

Patch Set 1 #

Total comments: 13

Patch Set 2 : Disable new tests on Chrome Frame #

Total comments: 20

Patch Set 3 : #

Total comments: 12

Patch Set 4 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+97 lines, -9 lines) Patch
M net/data/url_request_unittest/redirect-test.html.mock-http-headers View 1 chunk +1 line, -0 lines 0 comments Download
M net/http/http_cache.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/http/http_cache_transaction.cc View 1 2 2 chunks +5 lines, -6 lines 0 comments Download
M net/http/http_cache_unittest.cc View 1 2 3 5 chunks +46 lines, -2 lines 0 comments Download
M net/http/http_transaction.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M net/url_request/url_request_job.cc View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M net/url_request/url_request_job_unittest.cc View 1 2 2 chunks +37 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
davidben
7 years, 3 months ago (2013-09-16 20:09:26 UTC) #1
davidben
I wasn't sure whether it would be better to add a new method to HttpTransaction ...
7 years, 3 months ago (2013-09-16 20:15:01 UTC) #2
mmenke
I think this looks pretty good, though want to double-check for any unintended consequences. https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_unittest.cc ...
7 years, 3 months ago (2013-09-16 20:41:07 UTC) #3
davidben
+erikwright for chrome_frame/test/net/fake_external_tab.cc. I don't know anything about Chrome Frame, but the new tests check ...
7 years, 3 months ago (2013-09-16 22:00:00 UTC) #4
mmenke
https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_unittest.cc File net/url_request/url_request_unittest.cc (right): https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_unittest.cc#newcode4207 net/url_request/url_request_unittest.cc:4207: TEST_F(URLRequestTestHTTP, CachedRedirect) { On 2013/09/16 22:00:00, David Benjamin wrote: ...
7 years, 3 months ago (2013-09-16 22:08:59 UTC) #5
davidben
On 2013/09/16 22:08:59, mmenke wrote: > https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_unittest.cc > File net/url_request/url_request_unittest.cc (right): > > https://codereview.chromium.org/23710059/diff/1/net/url_request/url_request_unittest.cc#newcode4207 > ...
7 years, 3 months ago (2013-09-17 18:20:28 UTC) #6
erikwright (departed)
chrome_frame LGTM. Chrome Frame doesn't use the Chrome network stack, so it doesn't use the ...
7 years, 3 months ago (2013-09-17 18:25:35 UTC) #7
rvargas (doing something else)
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode453 net/http/http_cache_transaction.cc:453: DCHECK_NE(mode_, UPDATE); Is this line failing? UPDATE means that ...
7 years, 3 months ago (2013-09-17 19:53:25 UTC) #8
davidben
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode453 net/http/http_cache_transaction.cc:453: DCHECK_NE(mode_, UPDATE); On 2013/09/17 19:53:25, rvargas wrote: > Is ...
7 years, 3 months ago (2013-09-17 22:16:04 UTC) #9
rvargas (doing something else)
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/17 22:16:04, David Benjamin wrote: > On ...
7 years, 3 months ago (2013-09-18 18:37:10 UTC) #10
mmenke
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 18:37:10, rvargas wrote: > On 2013/09/17 ...
7 years, 3 months ago (2013-09-18 18:40:39 UTC) #11
rvargas (doing something else)
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 18:40:39, mmenke wrote: > On 2013/09/18 ...
7 years, 3 months ago (2013-09-18 19:09:11 UTC) #12
davidben
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:09:12, rvargas wrote: > On 2013/09/18 ...
7 years, 3 months ago (2013-09-18 19:35:42 UTC) #13
mmenke
https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:09:12, rvargas wrote: > On 2013/09/18 ...
7 years, 3 months ago (2013-09-18 19:38:30 UTC) #14
rvargas (doing something else)
LGTM https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 19:38:31, mmenke wrote: > On ...
7 years, 3 months ago (2013-09-18 21:41:31 UTC) #15
mmenke
LGTM, either way. https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc File net/http/http_cache_transaction.cc (left): https://codereview.chromium.org/23710059/diff/11001/net/http/http_cache_transaction.cc#oldcode1346 net/http/http_cache_transaction.cc:1346: DoneWritingToEntry(true); On 2013/09/18 21:41:32, rvargas wrote: ...
7 years, 3 months ago (2013-09-18 22:14:31 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidben@chromium.org/23710059/45001
7 years, 3 months ago (2013-09-19 15:41:30 UTC) #17
commit-bot: I haz the power
7 years, 3 months ago (2013-09-19 17:58:41 UTC) #18
Message was sent while issue was closed.
Change committed as 224161

Powered by Google App Engine
This is Rietveld 408576698