bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Do not allow to enable old disk cache backend

RESOLVED FIXED in Firefox 57

Status

()

Core
Networking: Cache
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: michal, Assigned: michal)

Tracking

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
Old disk cache isn't used for a long time now, but it's still possible to enable it with preference. Crash in the old cache started to appear in nightly 56 (bug 1379080). We want to remove the pref to make sure all those crashes are from profiles where users enabled the old cache. We no longer maintain the old cache code so there is no reason to allow anybody to enable it.
Duplicate of this bug: 1379080
(Assignee)

Comment 2

11 months ago
What should we do with browser.cache.auto_delete_cache_version preference? When removing the ability to use the old cache, shouldn't we assume this pref is set to 0, so we get rid of any old cache dir? Then in some next version we could remove the code around this pref completely.
Flags: needinfo?(honzab.moz)
(In reply to Michal Novotny (:michal) from comment #2)
> What should we do with browser.cache.auto_delete_cache_version preference?
> When removing the ability to use the old cache, shouldn't we assume this
> pref is set to 0, so we get rid of any old cache dir? Then in some next
> version we could remove the code around this pref completely.

Not necessarily in this bug, but I would agree to it.
Flags: needinfo?(honzab.moz)
Comment on attachment 8898304 [details] [diff] [review]
patch v1

Review of attachment 8898304 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

::: netwerk/cache2/CacheStorageService.cpp
@@ +596,2 @@
>  {
>    // CleaupCacheDirectories is called regardless what cache version is set up to use.

this may use an update too

@@ +605,5 @@
>    service->GetCacheIOTarget(getter_AddRefs(thread));
>    if (!thread)
>      return false;
>  
> +  RefPtr<CleaupCacheDirectoriesRunnable> r = 

nit: tws

@@ +690,5 @@
>    NS_ENSURE_ARG(aLoadContextInfo);
>    NS_ENSURE_ARG(_retval);
>  
>    nsCOMPtr<nsICacheStorage> storage;
> +  storage = new CacheStorage(aLoadContextInfo, false, false, false, false);

merge on one line?  (applies to all of them)
Attachment #8898304 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 6

11 months ago
Created attachment 8899593 [details] [diff] [review]
patch v2
Attachment #8898304 - Attachment is obsolete: true
Attachment #8899593 - Flags: review+
(Assignee)

Comment 7

11 months ago
Comment on attachment 8899593 [details] [diff] [review]
patch v2

I'm not sure if this requires data review. The patch removes old cache probes.
Attachment #8899593 - Flags: review?(rweiss)
(Assignee)

Updated

11 months ago
Attachment #8899593 - Flags: review?(francois)
Comment on attachment 8899593 [details] [diff] [review]
patch v2

Review of attachment 8899593 [details] [diff] [review]:
-----------------------------------------------------------------

The diff is confusing, but from what I can see you're only removing probes.

I don't think that requires a datareview, but here's one anyways.
Attachment #8899593 - Flags: review?(rweiss)
Attachment #8899593 - Flags: review?(francois)
Attachment #8899593 - Flags: review+

Comment 9

11 months ago
Pushed by mnovotny@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b37f9d5c83
Do not allow to enable old disk cache backend, r=honzab
https://hg.mozilla.org/mozilla-central/rev/79b37f9d5c83
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Was a bug ever filed to actually *remove* the old cache backend?  (Are we shipping a substantial amount of code here to our users that never actually gets used?)
Flags: needinfo?(michal.novotny)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #11)
> Was a bug ever filed to actually *remove* the old cache backend?  

913828

> (Are we
> shipping a substantial amount of code here to our users that never actually
> gets used?)

it's used for appcache we still ship
(Assignee)

Updated

4 months ago
Flags: needinfo?(michal.novotny)
You need to log in before you can comment on or make changes to this bug.