Http cache: Fix DCHECK with POSTs and a disabled cache.

Http cache: Fix DCHECK with POSTs and a disabled cache.

Make sure that we don't update the cache after a POST if the cache is
disabled.

BUG=426059
TEST=net_unittests
R=derekjchow@chromium.org, davidben@chromium.org

Committed: https://crrev.com/8e4b4b6ae451720d637556433099a68dab46d4ee
Cr-Commit-Position: refs/heads/master@{#300950}

[derekjchow1, 2014-10-22 23:16:25]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:16:27]

The CQ bit was unchecked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:16:32]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:16:37]

The CQ bit was unchecked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:22:45]

The CQ bit was checked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:22:47]

The CQ bit was unchecked by derekjchow@chromium.org

[derekjchow1, 2014-10-22 23:23:50]

On 2014/10/22 22:00:55, rvargas wrote:

LGTM

[rvargas (doing something else), 2014-10-23 00:04:54]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-23 00:06:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/672773002/1

[commit-bot: I haz the power, 2014-10-23 00:06:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-23 00:06:24]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rvargas (doing something else), 2014-10-23 01:16:18]

rvargas@chromium.org changed reviewers:
+ davidben@google.com

[rvargas (doing something else), 2014-10-23 01:16:41]

+David

[davidben, 2014-10-23 19:14:42]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2014-10-23 19:14:43]

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1222: if (!(effective_load_flags_ &
LOAD_DISABLE_CACHE) &&
Should this be mode_ != NONE or mode_ & WRITE or something of the sort? It looks
like LOAD_DISABLE_CACHE figures into ShouldPassThrough() which then controls
mode_.

[rvargas (doing something else), 2014-10-23 19:36:10]

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1222: if (!(effective_load_flags_ &
LOAD_DISABLE_CACHE) &&
On 2014/10/23 19:14:43, David Benjamin wrote:
> Should this be mode_ != NONE or mode_ & WRITE or something of the sort? It
looks
> like LOAD_DISABLE_CACHE figures into ShouldPassThrough() which then controls
> mode_.

There are multiple causes for mode_ to change between ShouldPassThrough() and
this point. What this line is saying is "unless the conditions when request was
made indicated that this should not affect the cache at all, we should
invalidate any cached entry at this point"

[davidben, 2014-10-23 19:45:42]

lgtm

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1222: if (!(effective_load_flags_ &
LOAD_DISABLE_CACHE) &&
On 2014/10/23 19:36:09, rvargas wrote:
> On 2014/10/23 19:14:43, David Benjamin wrote:
> > Should this be mode_ != NONE or mode_ & WRITE or something of the sort? It
> looks
> > like LOAD_DISABLE_CACHE figures into ShouldPassThrough() which then controls
> > mode_.
> 
> There are multiple causes for mode_ to change between ShouldPassThrough() and
> this point. What this line is saying is "unless the conditions when request
was
> made indicated that this should not affect the cache at all, we should
> invalidate any cached entry at this point"

Ohh, I see. This is about invalidating GETs after a successful POST, not
something about a cache entry attached to the current request or so.

Perhaps it's worth a comment above 1222 so it's clearer what this block is
doing?

[rvargas (doing something else), 2014-10-23 20:06:16]

thanks

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/672773002/diff/1/net/http/http_cache_transact...
net/http/http_cache_transaction.cc:1222: if (!(effective_load_flags_ &
LOAD_DISABLE_CACHE) &&
On 2014/10/23 19:45:42, David Benjamin wrote:
> On 2014/10/23 19:36:09, rvargas wrote:
> > On 2014/10/23 19:14:43, David Benjamin wrote:
> > > Should this be mode_ != NONE or mode_ & WRITE or something of the sort? It
> > looks
> > > like LOAD_DISABLE_CACHE figures into ShouldPassThrough() which then
controls
> > > mode_.
> > 
> > There are multiple causes for mode_ to change between ShouldPassThrough()
and
> > this point. What this line is saying is "unless the conditions when request
> was
> > made indicated that this should not affect the cache at all, we should
> > invalidate any cached entry at this point"
> 
> Ohh, I see. This is about invalidating GETs after a successful POST, not
> something about a cache entry attached to the current request or so.
> 
> Perhaps it's worth a comment above 1222 so it's clearer what this block is
> doing?

Done.

[rvargas (doing something else), 2014-10-23 20:06:22]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-10-23 20:07:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/672773002/20001

[commit-bot: I haz the power, 2014-10-23 21:25:04]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-10-23 21:26:13]

Patchset 2 (id:??) landed as
https://crrev.com/8e4b4b6ae451720d637556433099a68dab46d4ee
Cr-Commit-Position: refs/heads/master@{#300950}