Do not allow to enable old disk cache backend

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
2 years ago
10 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)

84.48 KB, patch
michal
: review+
francois
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years 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

2 years 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

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

Comment 7

a year 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

a year 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

a year 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: a year 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

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