New issue
Advanced search Search tips

Issue 768023 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 636400



Sign in to add a comment

Make sure sparse operations don't get out-of-sync when handling error

Project Member Reported by morlovich@chromium.org, Sep 22 2017

Issue description

Right now in SimpleCache, if SimpleSynchronousEntry detects an error, it returns the error code to SimpleEntryImpl, which calls MarkAsDoomedx, but doesn't do anything to remove the underlying files. That causes two problems:
1) MarkAsDoomed remove the entry from the index, so we have some files on disk while we think we don't, which can cause CreateEntry to fail next time we do something with the key.
2) MarkAsDoomed removes the entry from an active list, so we have an undoomed Entry object floating around which potentially shares its key with some other entry which we completely lost track off. This will be extra-problematic once we start renaming files and releasing descriptors, since this thing still has the *old* name.

 
Blocking: 636400
Note: this is for sparse ops only.

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 25 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/466e19d7a64c93550c80a903b4062a412df4c7be

commit 466e19d7a64c93550c80a903b4062a412df4c7be
Author: Maks Orlovich <morlovich@chromium.org>
Date: Mon Sep 25 15:07:53 2017

SimpleCache: Fixes to error handling in sparse code.

1) Doom entry in SSE on sparse op error.
SimpleEntryImpl expects this to happen (and non-sparse ops comply), and
accordingly calls MarkAsDoomed in EntryOperationComplete, which makes us
set up the world as if there are no longer files for the given hash.

2) Make the sparse ops called in failed state fail clearly. Current state
would DCHECK-fail in debug mode and try to do stuff in release mode.
(It could, for example, try writing to sparse file which failed 
 InitializeSparseFile() --- the reason I noticed this in the first place)

Bug:  768023 
Change-Id: I803c9d64627736185b09b74d16447d203761ee2b
Reviewed-on: https://chromium-review.googlesource.com/665067
Reviewed-by: Josh Karlin <jkarlin@chromium.org>
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504051}
[modify] https://crrev.com/466e19d7a64c93550c80a903b4062a412df4c7be/net/disk_cache/entry_unittest.cc
[modify] https://crrev.com/466e19d7a64c93550c80a903b4062a412df4c7be/net/disk_cache/simple/simple_entry_impl.cc
[modify] https://crrev.com/466e19d7a64c93550c80a903b4062a412df4c7be/net/disk_cache/simple/simple_synchronous_entry.cc
[modify] https://crrev.com/466e19d7a64c93550c80a903b4062a412df4c7be/net/disk_cache/simple/simple_util.h

Status: Fixed (was: Started)

Sign in to add a comment