Last Comment Bug 780750 - HTTP disk cache never gets used/fixed if wrong permissions on CACHE_MAP file
: HTTP disk cache never gets used/fixed if wrong permissions on CACHE_MAP file
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: mozilla17
Assigned To: Jason Duell [:jduell] (needinfo me)
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2012-08-06 15:37 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2012-08-08 09:32 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (1.94 KB, patch)
2012-08-06 15:37 PDT, Jason Duell [:jduell] (needinfo me)
hurley: review+
Details | Diff | Splinter Review

Description User image Jason Duell [:jduell] (needinfo me) 2012-08-06 15:37:58 PDT
Created attachment 649438 [details] [diff] [review]

Just found this while reviewing an unrelated patch--no evidence it's happening in the wild.

Steps to repro:   
  1) chmod 000 FIREFOX_PROFILE/Cache/_CACHE_001_ 
  2) run firefox: debugger shows we give up on opening disk cache, but we also don't delete cache.  So the disk cache will never be used again until/unless something else than firefox changes things :(

Culprit: the 'just return error' logic if we see any error code other than NS_ERROR_CORRUPT:

nsDiskCacheMap::Open does a valiant job of trying to convert errors to CORRUPT when appropriate, but it missed this case at least.   Instead of trying to fix that whole call tree (likely to break when someone changes the code), I think it makes more sense to whitelist the errors.  AFAICT the only error we can hit where we'd NOT want to delete/recreate the cache is NS_ERROR_ALREADY_INITIALIZED (I'm not really clear on whether hitting that is a logical error; for now I'm just returning, same as the code has done previously).

My old comment for the 'else' block was totally off--for new profiles we'll fail the mCacheDirectory->Exists test above, so will never get to that block.

I've verified the fix works for the repro case.
Comment 1 User image Nicholas Hurley [:nwgh][:hurley] (also 2012-08-06 16:11:12 PDT
Comment on attachment 649438 [details] [diff] [review]

Review of attachment 649438 [details] [diff] [review]:

Looks good (and seems like it even fixes a subtle bug in the collection of corrupt details telemetry!) r=me. If you like, I can rebase this on top of my patch for bug 709297 and push them both at the same time (will probably run the combo through try just to make sure there aren't any bad interactions we're missing)
Comment 2 User image Jason Duell [:jduell] (needinfo me) 2012-08-06 16:27:20 PDT
That would be great--thanks.
Comment 3 User image Nicholas Hurley [:nwgh][:hurley] (also 2012-08-07 15:21:11 PDT
Try looks good for this + bug 709297
Comment 4 User image Nicholas Hurley [:nwgh][:hurley] (also 2012-08-07 15:50:00 PDT
Comment 5 User image Ed Morley [:emorley] 2012-08-08 09:32:04 PDT

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