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
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla17
Assigned To: Jason Duell [:jduell] (needinfo? me)
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

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

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:

  http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheDevice.cpp?rev=4c632ce71344#1007

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 Nicholas Hurley [:nwgh][:hurley] 2012-08-06 16:11:12 PDT
Comment on attachment 649438 [details] [diff] [review]
v1

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 Jason Duell [:jduell] (needinfo? me) 2012-08-06 16:27:20 PDT
That would be great--thanks.
Comment 3 Nicholas Hurley [:nwgh][:hurley] 2012-08-07 15:21:11 PDT
Try looks good for this + bug 709297 https://tbpl.mozilla.org/?tree=Try&rev=988be862d179
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2012-08-07 15:50:00 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7231d7d68e8
Comment 5 Ed Morley [:emorley] 2012-08-08 09:32:04 PDT
https://hg.mozilla.org/mozilla-central/rev/c7231d7d68e8

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