1206407 - Incorrect use of IsForcedValidEntry in CacheStorageService::RemoveEntry

Status: RESOLVED INVALID

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

Seems like we are not correctly preventing entries forced to be valid.  According bug 1020416 comment 63 we must _prevent removal_ of entries that are EITHER OF:
- memory only, or
- marked as forced valid

But the condition at [1] seem to do something different, _prevent removal_ when:
- entry is memory only, AND
- is marked as forced valid

which goes completely against the idea.  We should replace && with || on that line, if I'm correct.  How could I miss this..?


[1] http://hg.mozilla.org/mozilla-central/annotate/6227054b7811/netwerk/cache2/CacheStorageService.cpp#l990

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

See comment 0 on rational.

I will do a try push after my series are not polluted with pinning/origin attributes.

Comment 2

[Michal Novotny [:michal]]

Comment on attachment 8663306 [details] [diff] [review]
v1

Review of attachment 8663306 [details] [diff] [review]:
-----------------------------------------------------------------

I think the current code is correct. Why we must not remove forced valid entry from CacheStorageService's arrays when it's on the disk? The entry isn't doomed so we can still read it from the disk once we need it and the information that it's a forced valid entry is kept in mForcedValidEntries until it expires. So I don't see any problem.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #2)
> Comment on attachment 8663306 [details] [diff] [review]
> v1
> 
> Review of attachment 8663306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think the current code is correct. Why we must not remove forced valid
> entry from CacheStorageService's arrays when it's on the disk? The entry
> isn't doomed so we can still read it from the disk once we need it and the
> information that it's a forced valid entry is kept in mForcedValidEntries
> until it expires. So I don't see any problem.

The patch wants to prevent purge of any MEMORY-ONLY entry (there is an '!').  But you are right.  I thing I'm just tired of the pinning patch so that I'm starting to see ghosts :D

This is actually the only place we evict stuff from the memory cache (which is implemented by the pool.)  Hence preventing any mem-only entry from purging will actually ruin the memory cache logic.

Thanks.

INVALID.