Closed Bug 1050613 Opened 6 years ago Closed 4 years ago

CacheStorageService::mForcedValidEntries should not persist beyond the entries themselves

Categories

(Core :: Networking: Cache, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: jaypoulz, Assigned: mayhemer)

References

Details

Attachments

(1 file, 7 obsolete files)

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.
Summary: CacheStorageService::mForcedValidEntries should be remove at CacheStorageService::Clear() or at entry removal → CacheStorageService::mForcedValidEntries should not persist beyond the entries themselves
Attached patch bug-1050613-fix.patch (obsolete) — Splinter Review
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
Attachment #8469758 - Flags: review?(michal.novotny)
Attachment #8469758 - Flags: review?(michal.novotny) → review?(honzab.moz)
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.
Attachment #8469758 - Flags: review?(honzab.moz) → review-
> 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?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
> 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.
(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.
Attached patch WIP -- bug-1050613-fix.patch (obsolete) — Splinter Review
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.
Attachment #8523374 - Flags: feedback?(honzab.moz)
Attached patch bug-1050613-fix.patch (obsolete) — Splinter Review
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.
Attachment #8469758 - Attachment is obsolete: true
Attachment #8523374 - Attachment is obsolete: true
Attachment #8523374 - Flags: feedback?(honzab.moz)
Attachment #8530461 - Flags: review?(honzab.moz)
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
Attachment #8530461 - Flags: review?(honzab.moz)
Attached patch patch (obsolete) — Splinter Review
Here's Jeremy's patch cleaned up and rebased.
Assignee: jaypoulz → hurley
Status: NEW → ASSIGNED
Attachment #8700086 - Flags: review?(honzab.moz)
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.
Attachment #8700086 - Flags: review?(honzab.moz) → review-
Attached patch patch (obsolete) — Splinter Review
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 :)
Attachment #8711782 - Flags: review?(honzab.moz)
Attachment #8700086 - Attachment is obsolete: true
Attachment #8530461 - Attachment is obsolete: true
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
Attachment #8711782 - Flags: review?(honzab.moz) → review-
Attached patch patch (obsolete) — Splinter Review
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 :)
Attachment #8717094 - Flags: review?(honzab.moz)
Attachment #8711782 - Attachment is obsolete: true
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++
Attachment #8717094 - Flags: review?(honzab.moz)
Taking this bug now.  I need to think about the right solution first.
Assignee: hurley → honzab.moz
Status: ASSIGNED → NEW
Attachment #8717094 - Attachment is obsolete: true
(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.
OK, will give it a priority.
Attached patch wip1 (obsolete) — Splinter Review
Needs testing first, backing up only.
Status: NEW → ASSIGNED
Attached patch v6Splinter Review
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
Attachment #8720892 - Attachment is obsolete: true
Attachment #8721411 - Flags: review?(michal.novotny)
Attachment #8721411 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
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
Flags: needinfo?(honzab.moz)
Keywords: checkin-needed
never mind my bad :)
Flags: needinfo?(honzab.moz)
https://hg.mozilla.org/mozilla-central/rev/de67e29ea715
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.