Do not allow to enable old disk cache backend

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: michal, Assigned: michal)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

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
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+
Posted patch patch v2Splinter Review
Attachment #8898304 - Attachment is obsolete: true
Attachment #8899593 - Flags: review+
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)
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+
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
Closed: 2 years ago
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
Flags: needinfo?(michal.novotny)
You need to log in before you can comment on or make changes to this bug.