The default bug view has changed. See this FAQ.

Fix potential problem in nsDiskCacheMap timer handling

RESOLVED FIXED in mozilla17

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

Trunk
mozilla17
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Michal Novotny (:michal) wrote:
> I would much rather see a proper solution to not process
> RevalidateTimerCallback() when the timer was canceled.
> 
> Btw. I've found another potential problem in
> nsDiskCacheMap::RevalidateTimerCallback(). The disk cache map passed as arg
> to the timer can be already deleted in case the disk device was shutdown in
> nsCacheService::Shutdown() instead of nsCacheService::OnProfileShutdown().
> We should first check if the disk device
> nsCacheService::gService->mDiskDevice still exists, then we could access the
> cache map as nsCacheService::gService->mDiskDevice.mCacheMap. And since we
> shouldn't use aClosure argument to pass the cache map to the timer, we can
> use it for the proper canceling.
> 
> We could have a counter in the cache map, that we increment every time we
> cancel the timer. We would pass the actual value of the counter as aClosure
> argument and in nsDiskCacheMap::RevalidateTimerCallback() we would compare
> it with the counter in the cache map. If it differs the timer was canceled.
(Assignee)

Comment 1

5 years ago
Created attachment 654198 [details] [diff] [review]
Patch v1.

- No longer pass a 'this' argument for the timer callback
- All diskCacheMap access is now protected by the lock inside the timer callback
- I now check at the top of the timer callback to make sure nsCacheService::gService->mDiskDevice exists and nsCacheService::gService->mDiskDevice is initialized
- I moved the _CACHE_CLEAN_ file outside of the cache directory, inside it's parent, since stats would be skewed because that directory gets moved out of the way
Attachment #654198 - Flags: review?(michal.novotny)
Comment on attachment 654198 [details] [diff] [review]
Patch v1.

> +
> +    // If we have less than kLastInvalidateTime since the last timer was

There should be kRevalidateCacheTimeout instead of kLastInvalidateTime.


> We could have a counter in the cache map, that we increment every time we
> cancel the timer. We would pass the actual value of the counter as aClosure
> argument and in nsDiskCacheMap::RevalidateTimerCallback() we would compare
> it with the counter in the cache map. If it differs the timer was canceled.

The patch looks good. I'm just curious why you don't want to replace kRevalidateCacheTimeout mechanism that doesn't work in all cases with the one described above that would work in all cases?
(Assignee)

Comment 3

5 years ago
(In reply to Michal Novotny (:michal) from comment #2)
> The patch looks good. I'm just curious why you don't want to replace
> kRevalidateCacheTimeout mechanism that doesn't work in all cases with the
> one described above that would work in all cases?

I'm not sure if all of the invalidate calls are protected under the cache lock.  If they aren't then we would need another lock to protect this counter. Further I'm a bit concerned it could at some point get out of sync and then we'd never revalidate.  For now that case would just effect telemetry data, but later it would have a greater effect of clearing the cache.  

The biggest reason though is just that it's not harmful to revalidate, so if the kRevalidateCacheTimeout mechanism ever does fail (and that's a really really really rare case), it's not harmful. 

So basically just because it's already coded the one way and I'm working on a bunch of other stuff, so I don't think it's necessary to change.
(Assignee)

Comment 4

5 years ago
Created attachment 654239 [details] [diff] [review]
Patchv 2.

Just a small fix for the mentioned wrong comment
Attachment #654198 - Attachment is obsolete: true
Attachment #654198 - Flags: review?(michal.novotny)
Attachment #654239 - Flags: review?(michal.novotny)
Attachment #654239 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 5

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d83016f1562
https://hg.mozilla.org/mozilla-central/rev/7d83016f1562
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.