Http cache: Implement a timeout for the cache lock.

Http cache: Implement a timeout for the cache lock.

The cache has a single writer / multiple reader lock to avoid downloading the
same resource n times. However, it is possible to block many tabs on the same
resource, for instance behind an auth dialog.

This CL implements a 20 seconds timeout so that the scenario described in the
bug results in multiple authentication dialogs (one per blocked tab) so the
user can know what to do. It will also help with other cases when the single
writer blocks for a long time.

The timeout is somewhat arbitrary but it should allow medium size resources
to be downloaded before starting another request for the same item. The general
solution of detecting progress and allow readers to start before the writer
finishes should be implemented on another CL.

BUG=6697, 46104
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279326

[rvargas (doing something else), 2014-06-20 01:02:18]

PTAL

[davidben, 2014-06-20 19:28:36]

Any timeout here is going to be arbitrary, but 20 seconds seems really on the
high end, depending on what cases this is intending to help with. If it's any
operation the user's waiting on, they'll probably have given up by then.

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

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:1213: TimeDelta::FromSeconds(20));
Nit: Pull the timeout up to a constant?

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2378: if (entry_lock_waiting_since_ !=
start_time)
I can't imagine it's possible for this to cause a problem, but instead of using
TimeTicks what about either a generation number or having a second
WeakPtrFactory, pulling the WeakPtr from there and, in DoAddToEntryComplete,
call timeout_weak_factory_.InvalidateWeakPtrs()? Just so the code isn't
sensitive to base::TimeTicks resolution.

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2380: 
Nit: DCHECK_EQ(STATE_ADD_TO_ENTRY_COMPLETE, next_state_)?

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2381: if (!cache_)
Nit: NOTREACHED() here?

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2385: OnIOComplete(ERR_CACHE_LOCK_TIMEOUT);
Should we add an entry to the net log when this happens? Hopefully we'll
recognize it, but it's probably worth marking it explicitly.

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

Thanks.

The main use case is the one of a lot of tabs and some dialog waiting on one of
them. The risk is that there's lots of pages that take more than 20 seconds to
load, and I don't want to start skipping the lock just because something takes
time to download (which is probably the case where the lock has more value).

So this is not so much about providing a smooth experience as to break infinite
waits. (for example, noticing the lock actually allowed us to detect and fix
that prerender bug). I want to go from "no matter how much I wait this doesn't
work" to "something is not right but it eventually works"

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

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2378: if (entry_lock_waiting_since_ !=
start_time)
On 2014/06/20 19:28:35, David Benjamin wrote:
> I can't imagine it's possible for this to cause a problem, but instead of
using
> TimeTicks what about either a generation number or having a second
> WeakPtrFactory, pulling the WeakPtr from there and, in DoAddToEntryComplete,
> call timeout_weak_factory_.InvalidateWeakPtrs()? Just so the code isn't
> sensitive to base::TimeTicks resolution.

In theory we could just check that entry_lock_waiting_since_ is not NULL... I
just wanted to be a little bit more conservative here (there could be a race and
the value may be different but if the timeout triggers and we are still waiting
we should probably just abort the wait).

So even if TimeTicks has a resolution of seconds we should still be OK relying
on that. I don't think it is worth the effort to come up with anything more
complicated than this, do you?

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2380: 
On 2014/06/20 19:28:35, David Benjamin wrote:
> Nit: DCHECK_EQ(STATE_ADD_TO_ENTRY_COMPLETE, next_state_)?

Done.

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2381: if (!cache_)
On 2014/06/20 19:28:35, David Benjamin wrote:
> Nit: NOTREACHED() here?

The cache may be gone leaving transactions behind. We don't notify transactions
waiting on the cache when that happens.

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2385: OnIOComplete(ERR_CACHE_LOCK_TIMEOUT);
On 2014/06/20 19:28:35, David Benjamin wrote:
> Should we add an entry to the net log when this happens? Hopefully we'll
> recognize it, but it's probably worth marking it explicitly.

DoAddToEntryComplete logs the result

[davidben, 2014-06-20 23:08:18]

LGTM.

Well, 20 seconds for an "eventually works" is probably long enough that it
doesn't really help, but *shrug*. I'm not going to bikeshed over the timeout.
:-)

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

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2378: if (entry_lock_waiting_since_ !=
start_time)
On 2014/06/20 22:40:06, rvargas wrote:
> On 2014/06/20 19:28:35, David Benjamin wrote:
> > I can't imagine it's possible for this to cause a problem, but instead of
> using
> > TimeTicks what about either a generation number or having a second
> > WeakPtrFactory, pulling the WeakPtr from there and, in DoAddToEntryComplete,
> > call timeout_weak_factory_.InvalidateWeakPtrs()? Just so the code isn't
> > sensitive to base::TimeTicks resolution.
> 
> In theory we could just check that entry_lock_waiting_since_ is not NULL... I
> just wanted to be a little bit more conservative here (there could be a race
and
> the value may be different but if the timeout triggers and we are still
waiting
> we should probably just abort the wait).
> 
> So even if TimeTicks has a resolution of seconds we should still be OK relying
> on that. I don't think it is worth the effort to come up with anything more
> complicated than this, do you?

Hrm. DoAddToEntry -> DoAddToEntryComplete can trigger multiple times because of
ERR_CACHE_RACE -> STATE_INIT_ENTRY. I suppose it's not actually important that
you get hit by the timer you originally scheduled, so just checking NULL is
probably fine, sure.

https://codereview.chromium.org/345643003/diff/20001/net/http/http_cache_tran...
net/http/http_cache_transaction.cc:2381: if (!cache_)
On 2014/06/20 22:40:06, rvargas wrote:
> On 2014/06/20 19:28:35, David Benjamin wrote:
> > Nit: NOTREACHED() here?
> 
> The cache may be gone leaving transactions behind. We don't notify
transactions
> waiting on the cache when that happens.

Oh, didn't notice this was a WeakPtr.

[rvargas (doing something else), 2014-06-24 02:11:30]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-06-24 02:12:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rvargas@chromium.org/345643003/40001

[commit-bot: I haz the power, 2014-06-24 05:33:51]

Change committed as 279326