1382688 - Do not allow to enable old disk cache backend

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[Michal Novotny [:michal]]

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.

Comment 2

[Michal Novotny [:michal]]

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.

Comment 3

[Honza Bambas (:mayhemer)]

(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.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d3413464133824acbb6c4eb50e3443b45579805&selectedJob=123866566

Comment 5

[Honza Bambas (:mayhemer)]

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)

Comment 6

[Michal Novotny [:michal]]

Attachment: patch v2

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 8899593 [details] [diff] [review]
patch v2

I'm not sure if this requires data review. The patch removes old cache probes.

Comment 8

[François Marier [: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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b37f9d5c83
Do not allow to enable old disk cache backend, r=honzab

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/79b37f9d5c83

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?)

Comment 12

[Honza Bambas (:mayhemer)]

(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