1050613 - CacheStorageService::mForcedValidEntries should not persist beyond the entries themselves

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jeremy Poulin [:jaypoulz]]

There are 2 weird cases that occur in the current configuration:

1. Entries that are forced valid remain forced valid even if they've been individually forced out of the cache.

2. CacheStorageService::Clear() doesn't empty mForcedValidEntries (leading to the same behavior of case 1).

Since I'm no expert in this realm, I might be forgetting something, but I think these are the primary issues.

Comment 1

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1050613-fix.patch

I don't know if this covers all the loose ends, but it does make my tests pass for 1016628. :)

https://tbpl.mozilla.org/?tree=Try&rev=2bf8532e14b2

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8469758 [details] [diff] [review]
bug-1050613-fix.patch

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

Don't refer a bug number.

Please update before landing.  r+ with these nits addressed.

::: netwerk/cache2/CacheStorageService.cpp
@@ +938,5 @@
>  
>    LOG(("RemoveExactEntry [entry=%p removed]", aEntry));
>    aEntries->Remove(aKey);
> +
> +  // A entry that is removed cannot be forced valid (bug 1050613)

An entry

@@ +939,5 @@
>    LOG(("RemoveExactEntry [entry=%p removed]", aEntry));
>    aEntries->Remove(aKey);
> +
> +  // A entry that is removed cannot be forced valid (bug 1050613)
> +  CacheStorageService::Self()->mForcedValidEntries.Remove(aKey);

r- for this

aKey is not the key you want.  You want HashingKeyWithStorage() and here it's just HashingKey().  And also this is called unnecessarily twice - simply a bad place.

Better would be if a new entry in its ctor deleted itself from the cache service force-valid hash table, every time.  Have a method like "ResetEntryValidFor" on the cache service accessible to entries to do so.

Comment 3

[Jeremy Poulin [:jaypoulz]]

> aKey is not the key you want.  You want HashingKeyWithStorage() and here
> it's just HashingKey().  And also this is called unnecessarily twice -
> simply a bad place.
> 
> Better would be if a new entry in its ctor deleted itself from the cache
> service force-valid hash table, every time.  Have a method like
> "ResetEntryValidFor" on the cache service accessible to entries to do so.

This new architecture isn't sufficient to prevent the persistence problem. For example, if we load in an image cat.jpg, force it valid, and purge it from memory, a call to IsForcedValid will still return true even though the asset in question does not exist. I think what I need to do is extend IsForcedValidInternal to verify that a cache entry exists before returning true.

Is there a way to find out if a cache entry exists when our only link to the entry is a HashingKeyWithStorage() key?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to Jeremy Poulin [:jaypoulz] from comment #3)
> > aKey is not the key you want.  You want HashingKeyWithStorage() and here
> > it's just HashingKey().  And also this is called unnecessarily twice -
> > simply a bad place.
> > 
> > Better would be if a new entry in its ctor deleted itself from the cache
> > service force-valid hash table, every time.  Have a method like
> > "ResetEntryValidFor" on the cache service accessible to entries to do so.
> 
> This new architecture isn't sufficient to prevent the persistence problem.
> For example, if we load in an image cat.jpg, force it valid, and purge it
> from memory, 

And that is the whole point of the hash table in the cache service.  The information must remain!

> a call to IsForcedValid will still return true even though the
> asset in question does not exist. 

That is not a bug - that is an expected behavior.

> I think what I need to do is extend
> IsForcedValidInternal to verify that a cache entry exists before returning
> true.

Absolutely not.


What you need is to remove the record from the hash table when the entry is doomed, put that code into CacheEntry::DoomAlreadyRemoved().  You only need to fix CacheStorageService::CacheFileDoomed - call to entry->DoomAlreadyRemoved() doesn't need to happen under the service lock.

And also let ctor of new entries remove it as I've suggested.

Comment 5

[Jeremy Poulin [:jaypoulz]]

> You only need to fix CacheStorageService::CacheFileDoomed - call to
> entry->DoomAlreadyRemoved() doesn't need to happen under the service lock.

I think that if I add code to remove the forced valid entry in the service's hash table, then the call to entry->DoomAlreadyRemoved will now have to be under the service lock. I think the goal here is to ensure mutual exclusion on writes to the hash table.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Jeremy Poulin [:jaypoulz] from comment #5)
> > You only need to fix CacheStorageService::CacheFileDoomed - call to
> > entry->DoomAlreadyRemoved() doesn't need to happen under the service lock.
> 
> I think that if I add code to remove the forced valid entry in the service's
> hash table, then the call to entry->DoomAlreadyRemoved will now have to be
> under the service lock. I think the goal here is to ensure mutual exclusion
> on writes to the hash table.

I'm not sure I follow, can you provide a quick WIP patch to see what you intend to do?  It will be better for me to understand.

Comment 7

[Jeremy Poulin [:jaypoulz]]

Attachment: WIP -- bug-1050613-fix.patch

This is my work in progress patch. This causes some wierd behavior with the tests I have written for prefetch. It should clarify what I'm doing though.

Comment 8

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1050613-fix.patch

This is the latest iteration of this patch. It clears away the forced validation of entries *except* when an entry goes from disk to memory only.

I wasn't sure how to address that, so I just special cased it. Please let me know if there is a better way, otherwise, I think this is pretty much done.

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 8530461 [details] [diff] [review]
bug-1050613-fix.patch

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

please, first create the patch with function names and context of 8 lines please.

::: netwerk/cache2/CacheStorageService.cpp
@@ +740,4 @@
>  
>  NS_IMETHODIMP CacheStorageService::Clear()
>  {
> +

no space

Comment 10

[u408661]

Attachment: patch

Here's Jeremy's patch cleaned up and rebased.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8700086 [details] [diff] [review]
patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +229,5 @@
>  
>    CacheStorageService::Self()->RecordMemoryOnlyEntry(
>      this, !aUseDisk, true /* overwrite */);
> +
> +  if (!aResetForcedValid) {

here I would prefer if (aResetForcedValid) { do..; }

@@ +1645,5 @@
> +  }
> +
> +  // An entry that is removed cannot be forced valid
> +  if (clearForcedValid) {
> +    CacheStorageService::Self()->mForcedValidEntries.Remove(key);

how can you do this?  first, it must be done under the proper lock, second, there can easily be a race condition when a newer entry has filled this already with a different value.  this is unfortunately by design, but OTOH, why do we really need to remove the key from this hashtable at this exact place anyway?  you are removing it in an entry's ctor, that is enough.

::: netwerk/cache2/CacheStorageService.cpp
@@ +764,5 @@
>  NS_IMETHODIMP CacheStorageService::Clear()
>  {
> +
> +  // Make sure all force valid timeouts are cleared
> +  mForcedValidEntries.Clear();

this must be done under BOTH the mLock and mForcedValidEntriesLock (taken in that order).

@@ +1448,5 @@
>        LOG(("  new storage entries table for context '%s'", aContextKey.BeginReading()));
>      }
>  
>      bool entryExists = entries->Get(entryKey, getter_AddRefs(entry));
> +    bool aResetForcedValid = true;

remove the "a" prefix

@@ +1467,5 @@
>        LOG(("  dooming entry %p for %s because of OPEN_TRUNCATE", entry.get(), entryKey.get()));
>        // On purpose called under the lock to prevent races of doom and open on I/O thread
>        // No need to remove from both memory-only and all-entries tables.  The new entry
>        // will overwrite the shadow entry in its ctor.
> +      entry->DoomAlreadyRemoved(aResetForcedValid);

I don't think you need to do it here since the ctor for a new entry will be called few lines below when you are already here (aCreateIfNotExist arg is always true, actually, should be removed one day).

@@ +1476,5 @@
>  
>      // Ensure entry for the particular URL, if not read/only
>      if (!entryExists && (aCreateIfNotExist || aReplace)) {
>        // Entry is not in the hashtable or has just been truncated...
> +      entry = new CacheEntry(aContextKey, aURI, aIdExtension, aWriteToDisk, aSkipSizeCheck, aPin, aResetForcedValid);

persoanlly, I would manipulate the mForcedValidEntries hashtable directly here and don't bother with passing the arg down the ctor.  don't forget to take the lock.

you may then not need the ResetForcedValidEntry method at all.

::: netwerk/cache2/CacheStorageService.h
@@ +163,5 @@
> +  /**
> +   * Clears a cache entry's forced valid status by removing from the
> +   * mForcedValidEntries hash table
> +   */
> +  void ResetForcedValidEntry(nsACString &aCacheEntryKey);

when you are here, please make it |nsACString const &|, also in the ForceEntryValidFor variant.

Comment 12

[u408661]

Attachment: patch

Updated as requested - everything still seems OK in terms of functionality (the old one was Jeremy's that somehow applied cleanly after a year, so I didn't understand it much), so that's good :)

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 8711782 [details] [diff] [review]
patch

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

::: netwerk/cache2/CacheEntry.cpp
@@ +1626,5 @@
> +  nsAutoCString key;
> +  nsresult rv = HashingKeyWithStorage(key);
> +  if (NS_FAILED(rv)) {
> +    return;
> +  }

leftover, please remove it.

::: netwerk/cache2/CacheStorageService.cpp
@@ +767,5 @@
> +  {
> +    mozilla::MutexAutoLock lock(mLock);
> +    mozilla::MutexAutoLock forcedValidEntriesLock(mForcedValidEntriesLock);
> +    // Make sure all force valid timeouts are cleared
> +    mForcedValidEntries.Clear();

no.  the goal here is to don't have a lock-open window between mForcedValidEntries.Clear(); and clearing the internal hashtables.  so it must be done inside the lock in the CacheObserver::UseNewCache() branch below.

don't bother with fact the pref can change at runtime.  possibility that we have forced valid entries and someone switches to the old cache and then clears leaving forced entries in be a known issue...  we will soon remove the old code and the branching.

@@ +1450,5 @@
>      if (entryExists && !aReplace) {
>        // check whether we want to turn this entry to a memory-only.
>        if (MOZ_UNLIKELY(!aWriteToDisk) && MOZ_LIKELY(entry->IsUsingDisk())) {
>          LOG(("  entry is persistnet but we want mem-only, replacing it"));
> +        resetForcedValid = false;

more thinking about this, i believe you want to remove the info for this case too.  you are creating a whole new entry.  I don't think we want to keep the info about forcing validation for it.
 
if you have a reason to, please add a good comment why.  otherwise remove this code.

@@ +1476,5 @@
>        entry = new CacheEntry(aContextKey, aURI, aIdExtension, aWriteToDisk, aSkipSizeCheck, aPin);
> +      if (resetForcedValid)
> +      {
> +        nsAutoCString key;
> +        nsresult rv = entry->HashingKeyWithStorage(key);

since we are removing resetForcedValid I think you should return to the previous way - implement CacheStorageService::ResetForcedValidEntry(nsACString &aCacheEntryKey) and let it be called from the entry's ctor.

sorry for these jumps, I realized we probably don't need this be optional just now.

::: netwerk/cache2/CacheStorageService.h
@@ +161,4 @@
>                            uint32_t aSecondsToTheFuture);
>  
> +  /**
> +   * Clears a cache entry's forced valid status by removing from the

// removing it from

Comment 14

[u408661]

Attachment: patch

OK, I think this follows what you're asking for - my brain went a little wonky while trying to undo most everything I did previously, as well as addressing your new review comments :)

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 8717094 [details] [diff] [review]
patch

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

I realized one thing: forced valid disk entries can be removed from memory.  we then construct new CacheEntry objects when such entries are re-requested.  that will effectively remove the force-valid information.

::: netwerk/cache2/CacheStorageService.cpp
@@ +1143,5 @@
> +// Clears a cache entry's forced valid status.
> +// Called upon creation of a new entry.
> +void CacheStorageService::ResetForcedValidEntry(nsACString const &aCacheEntryKey)
> +{
> +  mozilla::MutexAutoLock lock(mLock);

apparently you didn't even bother to test this patch.  this code is called when this lock is already held.  it would quickly throw on you:

>	nss3.dll!PR_Assert(0x5693954c, 0x56939500, 214) Line 553	C
 	nss3.dll!PR_Lock(0x0d5ebc50) Line 214	C
 	xul.dll!mozilla::OffTheBooksMutex::Lock() Line 382	C++
 	xul.dll!mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>({...}, {...}) Line 165	C++
 	xul.dll!mozilla::net::CacheStorageService::ResetForcedValidEntry({...}) Line 1147	C++
 	xul.dll!mozilla::net::CacheEntry::CacheEntry({...}, 0x0e3d8200, {...}, true, false, false) Line 239	C++
 	xul.dll!mozilla::net::CacheStorageService::AddStorageEntry({...}, 0x0e3d8200, {...}, true, false, false, true, false, 0x004fe8dc) Line 1480	C++
 	xul.dll!mozilla::net::CacheStorageService::AddStorageEntry(0x19ca2e60, 0x0e3d8200, {...}, true, false, 0x004fe8dc) Line 1412	C++
 	xul.dll!mozilla::net::CacheStorage::AsyncOpenURI(0x0e3d8080, {...}, 16, 0x1a84339c) Line 109	C++
 	xul.dll!mozilla::net::nsHttpChannel::OpenCacheEntry(false) Line 3038	C++

Comment 16

[Honza Bambas (:mayhemer)]

Taking this bug now.  I need to think about the right solution first.

Comment 17

[u408661]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Taking this bug now.  I need to think about the right solution first.

Honza, I don't mind you taking this bug - if you think you can get the solution faster than going around in circles with me, that's fine. However, keep in mind, this is blocking prefetch from landing, which has been waiting to land for a *very* long time. As such, this is pretty high-priority.

Comment 18

[Honza Bambas (:mayhemer)]

OK, will give it a priority.

Comment 19

[Honza Bambas (:mayhemer)]

Attachment: wip1

Needs testing first, backing up only.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v6

few nits first:
- API is now split to take context key and entry key separately (we usually have it in hands at the call time)
- that BTW fixes a bug around this code in CacheStorageService::RemoveEntry
- removed aCreateIfNotExist argument to AddStorageEntry, was always true

force-valid info removed, for entries in memory:
- on first call of DoomAlreadyRemoved on an entry, this happens when:
  - we are replacing an existing entry from AddStorageEntry ; Recreate or OPEN_TRUNCATE
  - IO manager evicted an entry and that entry is the one actually being evicted
- on first call of AsyncDoom public API on an entry

force-valid info removed, for entries NOT in memory:
- when we Recreate() or OPEN_TRUNCATE an entry
- doom by URL
- IO manager evicted an entry

generally:
- when we clear the cache
- when we clear only a single context

It can never happen we have a newer entry given to consumers that could be forced valid before the existing one is doomed.  Hence it can never happen we would falsely delete force-valid info.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a3408c03f8

Comment 21

[Carsten Book [:Tomcat]]

does not apply cleanly 

applying 1050613-force-valid-remove.patch
patching file netwerk/cache2/CacheStorage.cpp
Hunk #2 FAILED at 124
1 out of 2 hunks FAILED -- saving rejects to file netwerk/cache2/CacheStorage.cpp.rej

Comment 22

[Carsten Book [:Tomcat]]

never mind my bad :)

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de67e29ea715

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/de67e29ea715