Bypass the cache for the second or later HttpCache::Transaction...

Removing reviewers not to distract them by this suspended review. I have no time to finish this CL now.
Original reviewer: darin, rvargas, wtc

Let pending transactions on an HttpCache's ActiveEntry timeout after certain
time passed since queued. On timeout, we check if the writer transcation is
suspended due to some error such as SSL certification problem. If so, bypass
the cache. We put a task with 5s delay into MessageLoop for each pending
request to invoke timeout.

Motivation: By this change, we can avoid waiting the user with unuseful status
message "waiting for cache" instead of showing the SSL interstitial page when
the user open two windows for the same SSL-enabled URL with a bad certificate.

BUG=6697

[tyoshino-do-not-use, 2009-05-07 23:33:41]

Hi wtc,

I'm not sure if this approach is good or not. I'd like you to get this reviewed.

This is not a perfect approach because this bypassing does not occur when the
next or later transaction arrives during the first transaction is dealing with
TCP handshake or SSL handshake. But in most case, we can avoid confusing users
by this.

Thanks,

[wtc, 2009-05-07 23:55:14]

Darin, Ricardo,

Could you look at Takeshi's proposed solution to http://crbug.com/6697?

I'm not sure if it is fine to bypass the cache under that condition (the user
ponders over an SSL interstial page).

Assuming it's fine to bypass the cache:

I think using STATE_NONE to detect that condition
is unreliable, because any HTTP network transaction is
in STATE_NONE after Start() succeeds and before Read() is
called.  So we may bypass the cache on transactions without
any SSL certificate errors if the timing is wrong.

[darin (slow to review), 2009-05-08 00:00:43]

I would rather see us use a time-based heuristic.  If the transaction has been
stuck in the queue for too long, then we should give up waiting for exclusive
access to the cache entry.

[rvargas (doing something else), 2009-05-08 00:05:52]

On 2009/05/08 00:00:43, darin wrote:
> I would rather see us use a time-based heuristic.  If the transaction has been
> stuck in the queue for too long, then we should give up waiting for exclusive
> access to the cache entry.

Stuck as in not making progress, I guess. The first writer could be downloading
a relatively large file over a slow connection.

[darin (slow to review), 2009-05-08 05:46:51]

Yeah, good point.  I think it is reasonable to restrict this to the case where
the writer has not started writing after a suitable timeout.

[tyoshino-do-not-use, 2009-05-14 00:51:12]

Thanks for comments. Ready for another look. If this approach looks good for
you, I proceed to work on modifying Mock to pass unittests.

[rvargas (doing something else), 2009-05-14 23:35:12]

On 2009/05/14 00:51:12, tyoshino wrote:
> Thanks for comments. Ready for another look. If this approach looks good for
> you, I proceed to work on modifying Mock to pass unittests.

I don't think it is a good idea to store a pointer to the transaction and five
seconds later see if that pointer lives on a pending list (even after verifying
that the url is the same). Other than that, the approach seems reasonable.

[darin (slow to review), 2009-05-18 17:30:19]

i have more comments... i'm not so sure about this new state.  i need to think
about it more.

http://codereview.chromium.org/111005/diff/3025/3030
File net/base/load_states.h (right):

http://codereview.chromium.org/111005/diff/3025/3030#newcode15
Line 15: LOAD_STATE_WAITING_FOR_USER_ACTION,
nit: please append to the existing enumeration.

http://codereview.chromium.org/111005/diff/3025/3027
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/111005/diff/3025/3027#newcode522
Line 522: && next_state_ != STATE_NONE
nit: chrome style is to put the && on the preceding line.

[tyoshino-do-not-use, 2009-06-02 09:38:07]

Thank you for comments and sorry for not responding for a week.

rvargas: I think it's ugly to store a pointer, too. This week I've tried to move
the task to HttpCache::Transaction and use ScopedRunnableMethodFactory. The
closure created by this factory has nice ref counting (Revocable* stuff).

Also addressed darin's comments.

Could you take a look again?

If adding new state is not good, instead I'll add some method say HasStarted()
on HttpNetworkTransaction to check if it has already started downloading.

[rvargas (doing something else), 2009-06-03 18:52:12]

The problem with this approach is that you are taking a pointer to the current
active entry and sending it to five seconds in the future. At that time, you
access the entry, but there is no guarantee that this entry is alive.

If what we want is to restrict the cache bypass to a particular instance when an
action is taking too long (waiting for user action), maybe we can start the
timer when we start that action, cancel it when it finishes, and if it triggers,
send back a notification to the cache so that all pending transactions are
notified that we've been waiting a long time on something.

[tyoshino-do-not-use, 2009-06-24 16:55:49]

Thank you.

On 2009/06/03 18:52:12, rvargas wrote:
> The problem with this approach is that you are taking a pointer to the current
> active entry and sending it to five seconds in the future. At that time, you
> access the entry, but there is no guarantee that this entry is alive.
> 

OK. I'll leave that task to cache_ and try to get rid of holding pointers.

> If what we want is to restrict the cache bypass to a particular instance when
an
> action is taking too long (waiting for user action), maybe we can start the
> timer when we start that action, cancel it when it finishes, and if it
triggers,
> send back a notification to the cache so that all pending transactions are
> notified that we've been waiting a long time on something.

So, it'll be like this:
- Trans A becomes writer
- Trans B, C, D come and pend
- After passing t sec, trans A tells cache to let incoming trans to bypass the
cache
- Trans B, C, D bypass the cache
- Trans E, F, G comes after the notification from A and A is still suspended.
The cache immediately tell them to bypass the cache
- Trans E, F, G bypass the cache

Am I understanding correctly?

Sorry for taking too long time for this...

[rvargas (doing something else), 2009-06-24 18:18:00]

On 2009/06/24 16:55:49, tyoshino wrote:
> Thank you.
> 
> On 2009/06/03 18:52:12, rvargas wrote:
> > The problem with this approach is that you are taking a pointer to the
current
> > active entry and sending it to five seconds in the future. At that time, you
> > access the entry, but there is no guarantee that this entry is alive.
> > 
> 
> OK. I'll leave that task to cache_ and try to get rid of holding pointers.
> 
> > If what we want is to restrict the cache bypass to a particular instance
when
> an
> > action is taking too long (waiting for user action), maybe we can start the
> > timer when we start that action, cancel it when it finishes, and if it
> triggers,
> > send back a notification to the cache so that all pending transactions are
> > notified that we've been waiting a long time on something.
> 
> So, it'll be like this:
> - Trans A becomes writer
> - Trans B, C, D come and pend
> - After passing t sec, trans A tells cache to let incoming trans to bypass the
> cache
> - Trans B, C, D bypass the cache
> - Trans E, F, G comes after the notification from A and A is still suspended.
> The cache immediately tell them to bypass the cache
> - Trans E, F, G bypass the cache
> 
> Am I understanding correctly?

Yes, I think that's the correct approach. And just to keep going:

- The user allows transaction A to continue
- Transactions H and I come to the cache, and because A is not waiting for the
user anymore, H and I are added to the pending queue, waiting for A to finish.

> 
> Sorry for taking too long time for this...

no problem.