Disk cache: Keep a map of all open entries....

Disk cache: Keep a map of all open entries.

We still have a few crashes when for some reason we believe
an entry is not dirty and we follow the pointer stored by
its rankings node only to crash while accessing the memory.
I have no explanation to why the dirty id matches the current
one (a page boundary issue maybe?), but having a map with all
open entries solves the issue of having to follow pointers
from disk.

BUG=15596
, b/1120346
TEST=unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=20067

[rvargas (doing something else), 2009-07-07 00:21:27]

The first patch set is the core of the change. The second one is some additional
cleanup and the third one is the unit test.

[Nicolas Sylvain, 2009-07-07 17:47:14]

LGTM

http://codereview.chromium.org/149218/diff/1009/1011
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/149218/diff/1009/1011#newcode727
Line 727: EntriesMap::iterator it = open_entries_.find(address.value());
why would it not be in the map? dirty maybe? should we dcheck when it's not?

http://codereview.chromium.org/149218/diff/1009/1011#newcode1163
Line 1163: cache_entry = NULL;
do you really need this here? since you do it below too. Well, i guess it makes
it clearer.

[rvargas (doing something else), 2009-07-07 18:13:24]

Thanks.

http://codereview.chromium.org/149218/diff/1009/1011
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/149218/diff/1009/1011#newcode727
Line 727: EntriesMap::iterator it = open_entries_.find(address.value());
On 2009/07/07 17:47:14, Nicolas Sylvain wrote:
> why would it not be in the map? dirty maybe? should we dcheck when it's not?

Yes and no. We don't store dirty entries in the map, but it should be a frequent
thing to open dirty entries so we cannot dcheck here (the dirty flag is cleared
before the destruction of the entry)

http://codereview.chromium.org/149218/diff/1009/1011#newcode1163
Line 1163: cache_entry = NULL;
On 2009/07/07 17:47:14, Nicolas Sylvain wrote:
> do you really need this here? since you do it below too. Well, i guess it
makes
> it clearer.

if I don't set it to null, we'll copy an invalid pointer into parent_entry. I
guess that now we have more calls to Release() than assignments to plain
pointers so I could see how this function looks after replacing cache_entry and
parent_entry with scoped pointers, but I'll leave that for another cl.

[Nicolas Sylvain, 2009-07-07 18:38:22]

On Tue, Jul 7, 2009 at 11:13 AM, <rvargas@chromium.org> wrote:

> Thanks.
>
>
> http://codereview.chromium.org/149218/diff/1009/1011
> File net/disk_cache/backend_impl.cc (right):
>
> http://codereview.chromium.org/149218/diff/1009/1011#newcode727
> Line 727: EntriesMap::iterator it = open_entries_.find(address.value());
> On 2009/07/07 17:47:14, Nicolas Sylvain wrote:
>
>> why would it not be in the map? dirty maybe? should we dcheck when
>>
> it's not?
>
> Yes and no. We don't store dirty entries in the map, but it should be a
> frequent thing to open dirty entries so we cannot dcheck here (the dirty
> flag is cleared before the destruction of the entry)
>
> http://codereview.chromium.org/149218/diff/1009/1011#newcode1163
> Line 1163: cache_entry = NULL;
> On 2009/07/07 17:47:14, Nicolas Sylvain wrote:
>
>> do you really need this here? since you do it below too. Well, i guess
>>
> it makes
>
>> it clearer.
>>
>
> if I don't set it to null, we'll copy an invalid pointer into
> parent_entry.


ah, got it, lgtm


> I guess that now we have more calls to Release() than
> assignments to plain pointers so I could see how this function looks
> after replacing cache_entry and parent_entry with scoped pointers, but
> I'll leave that for another cl.
>
>
> http://codereview.chromium.org/149218
>