Last Comment Bug 784657 - Fix potential problem in nsDiskCacheMap timer handling
: Fix potential problem in nsDiskCacheMap timer handling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86_64 All
: -- critical (vote)
: mozilla17
Assigned To: Brian R. Bondy [:bbondy]
:
:
Mentors:
Depends on:
Blocks: 777328
  Show dependency treegraph
 
Reported: 2012-08-22 05:15 PDT by Brian R. Bondy [:bbondy]
Modified: 2012-08-24 20:04 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1. (7.11 KB, patch)
2012-08-22 07:16 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patchv 2. (7.11 KB, patch)
2012-08-22 09:29 PDT, Brian R. Bondy [:bbondy]
michal.novotny: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-08-22 05:15:23 PDT
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.
Comment 1 Brian R. Bondy [:bbondy] 2012-08-22 07:16:11 PDT
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
Comment 2 Michal Novotny (:michal) 2012-08-22 09:08:41 PDT
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?
Comment 3 Brian R. Bondy [:bbondy] 2012-08-22 09:17:57 PDT
(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.
Comment 4 Brian R. Bondy [:bbondy] 2012-08-22 09:29:23 PDT
Created attachment 654239 [details] [diff] [review]
Patchv 2.

Just a small fix for the mentioned wrong comment
Comment 5 Brian R. Bondy [:bbondy] 2012-08-23 19:25:15 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d83016f1562
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-08-24 20:04:09 PDT
https://hg.mozilla.org/mozilla-central/rev/7d83016f1562

Note You need to log in before you can comment on or make changes to this bug.