HTTP disk cache never gets used/fixed if wrong permissions on CACHE_MAP file

RESOLVED FIXED in mozilla17

Status

()

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

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
Attachment #649438 - Flags: review?(hurley)
(Assignee)

Updated

5 years ago
Summary: HTTP 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
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)
Attachment #649438 - Flags: review?(hurley) → review+
(Assignee)

Comment 2

5 years ago
That would be great--thanks.
Try looks good for this + bug 709297 https://tbpl.mozilla.org/?tree=Try&rev=988be862d179
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7231d7d68e8

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c7231d7d68e8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.