1038357 - CacheStorageService mForcedValidEntries HashTable should be cleaned up

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jeremy Poulin [:jaypoulz]]

Something similiar to CacheStorageService::TelemetryPrune:

https://bugzilla.mozilla.org/attachment.cgi?id=8452048&action=diff#a/netwerk/cache2/CacheStorageService.cpp_sec4

Comment 1

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1038357-fix.patch

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 8455833 [details] [diff] [review]
bug-1038357-fix.patch

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

f+ but r-, please fix the comments and if you feel fit, ask for r next time.

::: netwerk/cache2/CacheStorageService.cpp
@@ +1057,5 @@
>  void CacheStorageService::ForceEntryValidFor(nsACString &aCacheEntryKey,
>                                               uint32_t aSecondsToTheFuture)
>  {
> +  TimeStamp now = TimeStamp::NowLoRes();
> +  ForcedValidEntriesPrune(now);

ehm.. this needs to be called under the lock.

@@ +1063,4 @@
>    mozilla::MutexAutoLock lock(mLock);
>  
>    // This will be the timeout
>    TimeStamp validUntilPtr = TimeStamp::NowLoRes() +

reuse |now| here (will be obtained under the lock)

@@ +1063,5 @@
>    mozilla::MutexAutoLock lock(mLock);
>  
>    // This will be the timeout
>    TimeStamp validUntilPtr = TimeStamp::NowLoRes() +
> +  TimeDuration::FromSeconds(aSecondsToTheFuture);

why are you changing the indention?

::: netwerk/cache2/CacheStorageService.h
@@ +163,5 @@
>  
> +  /**
> +   * Cleans out the entries in mForcedValidEntries that have timed out.
> +   */
> +  void ForcedValidEntriesPrune(TimeStamp &now);

put this please under |nsresult AddStorageEntry(nsCSubstring const& aContextKey...| declaration.  No need for a comment I think.. The one in the .cpp is enough.

Comment 3

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1038357-fix.patch

https://tbpl.mozilla.org/?tree=Try&rev=644d9163fd89

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 8456245 [details] [diff] [review]
bug-1038357-fix.patch

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

r=honzab _with the comments fixed_

::: netwerk/cache2/CacheStorageService.cpp
@@ +1067,1 @@
>      TimeDuration::FromSeconds(aSecondsToTheFuture);

this is no longer "Ptr" (I repeat myself here...)
nit: maybe keep this on one line

Comment 5

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1038357-fix.patch

Comment 6

[Jeremy Poulin [:jaypoulz]]

https://tbpl.mozilla.org/?tree=Try&rev=02d58cae5728

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98fa8afd9169

Comment 8

[Carsten Book [:Tomcat]]

sorry had to backout this change since bug 1020416 was backedout for causing startup crashes and this resulted a merge conflict from mozilla-inbound to mozilla-central with this patch

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6ee18b1e9d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f6ee18b1e9d

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

The .cpp file change was backed out as part of https://hg.mozilla.org/mozilla-central/rev/8da875b402fe though the .h change was not.

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

For cleanliness, I backed out the rest in:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28b762c2b3e2
so that the patch isn't half-landed.

Comment 13

[Jeremy Poulin [:jaypoulz]]

Thanks! :) I appreciate this! :)

Comment 14

[Jeremy Poulin [:jaypoulz]]

Attachment: bug-1038357-fix.patch

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92c571f0daf9

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/92c571f0daf9