Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

BUG=647177
Review-Url: https://codereview.chromium.org/2671793002
Cr-Commit-Position: refs/heads/master@{#449342}
Committed: https://chromium.googlesource.com/chromium/src/+/3701cde07f15ce076d9005bac70cb6fe65f465bb

[shivanisha, 2017-02-02 20:04:39]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 20:05:03]

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

[shivanisha, 2017-02-02 20:05:24]

shivanisha@chromium.org changed reviewers:
+ jkarlin@chromium.org

[shivanisha, 2017-02-02 20:42:21]

PTAL, Thanks!
The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

[commit-bot: I haz the power, 2017-02-02 21:16:57]

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

[commit-bot: I haz the power, 2017-02-02 21:16:57]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-02-03 13:16:30]

Nice catch. lgtm!

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:1776: cache.BypassCacheLock();
nit: Can you add a comment that it's because of this line that the test will
immediately simulate timeout?

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:1792: 
Remove this line

[shivanisha, 2017-02-03 14:57:04]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 14:57:19]

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

[shivanisha, 2017-02-03 14:58:09]

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:1776: cache.BypassCacheLock();
done.

https://codereview.chromium.org/2671793002/diff/1/net/http/http_cache_unittes...
net/http/http_cache_unittest.cc:1792: 
done.

[shivanisha, 2017-02-03 15:12:22]

Description was changed from

==========
Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

BUG=647177
==========

to

==========
Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

BUG=647177
==========

[shivanisha, 2017-02-03 15:12:44]

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

[shivanisha, 2017-02-03 15:13:18]

asanka@, PTAL for owner approval, thanks!

The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

[commit-bot: I haz the power, 2017-02-03 16:35:09]

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

[commit-bot: I haz the power, 2017-02-03 16:35:10]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[asanka, 2017-02-07 22:56:03]

LGTM % test considerations

Sorry about the delay.

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

https://codereview.chromium.org/2671793002/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:1778: cache.BypassCacheLock();
This method is poorly named :-(

https://codereview.chromium.org/2671793002/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:1786: // Using load flags similar to
MetadataWriter.
There are two things to test:

1) That HttpCache::WriteMetadata() will fail if the cache lock times out.
2) That HttpCache::Transaction fails if LOAD_ONLY_FROM_CACHE is set and the
cache lock times out.

Due to the additional flags set by WriteMetadata, it's not robust to assume that
#1 implies #2. But the inverse is defensible. So let's change this test to only
specify LOAD_ONLY_FROM_CACHE. The other flags should be moot.

To close the loop I'd suggest adding a test case to the WriteMetadata_* group
above which simulates the cache lock timeout while invoking
HttpCache::WriteMetadata().

[shivanisha, 2017-02-08 19:14:13]

Thanks for the review. 
Addressed the feedback. Also created a separate but related issue:
crbug.com/690099

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

https://codereview.chromium.org/2671793002/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:1778: cache.BypassCacheLock();
Updated to SimulateCacheLockTimeout for clarity and removed the comment over it.

https://codereview.chromium.org/2671793002/diff/20001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:1786: // Using load flags similar to
MetadataWriter.
Done.

The new test added for WriteMetadata_* group cannot test 2 simultaneous metadata
writers because of an issue in the way MetadataWriter uses the load flags. I
have added a todo note and created a bug for the same. In the test I have added
one normal (non read only) transaction and one MetadataWriter to achieve cache
lock timeout.

[shivanisha, 2017-02-08 20:25:49]

The CQ bit was checked by shivanisha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-08 20:26:38]

Dry run: 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, 2017-02-08 23:16:25]

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

[commit-bot: I haz the power, 2017-02-08 23:16:26]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-02-09 02:39:53]

LGTM

I think the nits are more symptoms of this file needing some cleanup. Perhaps we
should do that separately.

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

https://codereview.chromium.org/2671793002/diff/40001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:6832: NULL, 0);
this doesn't do anything?

https://codereview.chromium.org/2671793002/diff/40001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:6860: ASSERT_TRUE(response.metadata.get() ==
NULL);
nullptr or just ASSERT_FALSE(response.metadata.get())

here and above.

[shivanisha, 2017-02-09 16:46:41]

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

https://codereview.chromium.org/2671793002/diff/40001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:6832: NULL, 0);
Removed.

https://codereview.chromium.org/2671793002/diff/40001/net/http/http_cache_uni...
net/http/http_cache_unittest.cc:6860: ASSERT_TRUE(response.metadata.get() ==
NULL);
Changed to ASSERT_FALSE.

[shivanisha, 2017-02-09 16:46:44]

The CQ bit was checked by shivanisha@chromium.org

[shivanisha, 2017-02-09 16:46:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
jkarlin@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/2671793002/#ps60001
(title: "Feedback addressed.")

[commit-bot: I haz the power, 2017-02-09 16:47:29]

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, 2017-02-09 17:43:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486658804844940,
"parent_rev": "9c8fc091e86393dbff8a23f60de82a7132990cc1", "commit_rev":
"3701cde07f15ce076d9005bac70cb6fe65f465bb"}

[commit-bot: I haz the power, 2017-02-09 17:43:51]

Description was changed from

==========
Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

BUG=647177
==========

to

==========
Correct handling of ERR_CACHE_LOCK_TIMEOUT for read only transactions.

The investigation of the root cause is detailed in
https://bugs.chromium.org/p/chromium/issues/detail?id=647177#c11

BUG=647177

Review-Url: https://codereview.chromium.org/2671793002
Cr-Commit-Position: refs/heads/master@{#449342}
Committed:
https://chromium.googlesource.com/chromium/src/+/3701cde07f15ce076d9005bac70c...
==========

[commit-bot: I haz the power, 2017-02-09 17:43:52]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/3701cde07f15ce076d9005bac70c...