Do not DCHECK on SimpleCache deletion-while-closing.

Do not DCHECK on SimpleCache deletion-while-closing.

The cache should be safe to delete immediately after calling the
destructor. This means that failures from files going missing while
the index is being serialized are acceptable; if we are going to
actually restart this cache, they can be dealt with at startup anyway.

R=jkarlin@chromium.org
BUG=416940
Committed: https://crrev.com/8e33c7610574ebe3466177f227b1c17e47f4f731
Cr-Commit-Position: refs/heads/master@{#302272}

[gavinp, 2014-10-30 18:59:20]

jkarlin: PTAL

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_index_file.cc:74: File::FLAG_CREATE |
File::FLAG_WRITE | File::FLAG_SHARE_DELETE);
This change is good to do while we are here: if multiple serializations of the
same index are overlapping, this causes the second one to abort. This may
actually enhance the entry not found problem you describe in the underlying
issue; esp on Linux, so expect some changes here.

[jkarlin, 2014-10-30 19:19:24]

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
net/disk_cache/backend_unittest.cc:3487: // NOTE: IF THIS TEST IS FLAKY THEN IT
IS FAILING.
Does this flake at least say 20% of the time?  It needs to flake fairly  often
to be noticed.  How hard would it be to make this test work 100% of the time?

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
net/disk_cache/backend_unittest.cc:3487: // NOTE: IF THIS TEST IS FLAKY THEN IT
IS FAILING.
Append the bug that this CL fixes to the test comment.

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_index_file.cc:227: 
Would be better to replace DCHECK(result) with DCHECK(result || shutting_down)
so that other writes are still checked.

[gavinp, 2014-10-31 13:55:54]

jkarlin: remediated. Went with a TODO on your good DCHECK suggestion.

What do you think?

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
net/disk_cache/backend_unittest.cc:3487: // NOTE: IF THIS TEST IS FLAKY THEN IT
IS FAILING.
On 2014/10/30 19:19:24, jkarlin wrote:
> Append the bug that this CL fixes to the test comment.

Done.

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/backend_u...
net/disk_cache/backend_unittest.cc:3487: // NOTE: IF THIS TEST IS FLAKY THEN IT
IS FAILING.
On 2014/10/30 19:19:24, jkarlin wrote:
> Does this flake at least say 20% of the time?  It needs to flake fairly  often
> to be noticed.  How hard would it be to make this test work 100% of the time?

I get a 100% failure rate on 100 iterations on my Mac, and that runs in 380ms.
I'm calling that acceptable for now. I'm a bit concerned about windows, so I'll
watch try bots.

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
File net/disk_cache/simple/simple_index_file.cc (right):

https://codereview.chromium.org/691803003/diff/20001/net/disk_cache/simple/si...
net/disk_cache/simple/simple_index_file.cc:227: 
On 2014/10/30 19:19:24, jkarlin wrote:
> Would be better to replace DCHECK(result) with DCHECK(result || shutting_down)
> so that other writes are still checked.

That data isn't easily available here; it would require some plumbing through.
I've added a TODO and a discussion of the reasoning.

[jkarlin, 2014-10-31 13:57:57]

lgtm

[gavinp, 2014-10-31 14:54:49]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-10-31 14:55:25]

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

[commit-bot: I haz the power, 2014-10-31 15:42:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 15:42:56]

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