Firefox59 does not properly honor cache size set in autoconfig files

RESOLVED FIXED in Firefox 67

Status

()

defect
P3
normal
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: jmp242, Assigned: michal)

Tracking

52 Branch
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20180319000641

Steps to reproduce:

Create an  autoconfig file with 
"browser.cache.disk.capacity": {
      "value": 24576
    },
    "browser.cache.disk.smart_size.enabled": {
      "value": false
    }, 


Actual results:

Cache size shown in UI is 1024 MB


Expected results:

Cache size shown in UI and used is 24MB
So it appears the issue is when you make it a default pref, it gets set to a different calue.

If you make it a user pref or a locked pref, it shoudld work properly.

Updated

a year ago
Component: Untriaged → File Handling

Comment 2

a year ago
Please correct as needed if this is the incorrect component. Thanks.
Taking for now because I want to debug before it's passed along.
Assignee: nobody → mozilla

Updated

11 months ago
Priority: -- → P3

Comment 4

11 months ago
https://searchfox.org/mozilla-central/source/netwerk/cache/nsCacheService.cpp#672

In order to switch users over to smart caching, the code checks to see if the user ever had set a value and if not, assume we can switch them.

That's why it works when you set a user value.

Since the default smart size value is true, this code needs to be modified so that if smartsize is set to false, the firstRun code isn't followed at all.
Mike, do we still care about that?
Flags: needinfo?(mozilla)
Summary: FirefoxESR52 and RR59 do not properly honor cache size set in autoconfig files → Firefox59 does not properly honor cache size set in autoconfig files
I think so. It is a bug.
Flags: needinfo?(mozilla)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 8

6 months ago
Honza, shouldn't we deny all prefs manipulation in the old cache code when cache2 is used?
Flags: needinfo?(honzab.moz)
(In reply to Michal Novotny [:michal] from comment #8)
> Honza, shouldn't we deny all prefs manipulation in the old cache code when
> cache2 is used?

Oh, definitely.  I don't think there is anymore a way to even use cache1, except appcache, which has its own set of preferences and doesn't touch this particular bug at all.

OTOH, we still have the smart cache size concept even in the new cache, right?  Is there a different pref to manipulate it that is expected to work but it isn't?  Should we fix something in the new cache code (CacheObserver, I assume) then?
Flags: needinfo?(honzab.moz) → needinfo?(michal.novotny)
(Assignee)

Comment 10

6 months ago
Bug 1382688 removed possibility to use the old disk cache. New cache uses the same preference to enable/disable smart cache size and the old cache shouldn't touch it. I'll fix it.
Assignee: mozilla → michal.novotny
Component: File Handling → Networking: Cache
Flags: needinfo?(michal.novotny)
Product: Firefox → Core
(In reply to Michal Novotny [:michal] from comment #10)
> Bug 1382688 removed possibility to use the old disk cache. New cache uses
> the same preference to enable/disable smart cache size and the old cache
> shouldn't touch it. I'll fix it.

OK, if the prefs are the same, then the autoconfig file should work.  But it seems like it doesn't.  Hence, it looks like we need to fix something, but not in the cache1 code, rather in the cache2 code?  That was the point of my questions.
(Assignee)

Comment 12

6 months ago
Did you read the comments in phabricator? The problem is that prefs are read and set in the old cache, so this has to be removed.
(In reply to Michal Novotny [:michal] from comment #12)
> Did you read the comments in phabricator? The problem is that prefs are read
> and set in the old cache, so this has to be removed.

Why are people writing comments to phab and not here?  

OK, I finally get it!  If the patch prevents the Set*Pref calls that offend the autoconf file, let's take it.

Thanks for explanation.

Updated

6 months ago
Whiteboard: [necko-triaged]
(Assignee)

Comment 14

4 months ago
Posted patch fixSplinter Review

Removes all preferences manipulation from old cache code that isn't used by offline cache. It removes also some related code (e.g. everything smart size related, unused defines etc.), but the goal wasn't to remove all unused code from the old cache.

Attachment #9022722 - Attachment is obsolete: true
Attachment #9036321 - Flags: review?(honzab.moz)
Comment on attachment 9036321 [details] [diff] [review]
fix

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

thanks and sorry for the delay.
Attachment #9036321 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 17

4 months ago

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/780eec2d27c3
Firefox59 does not properly honor cache size set in autoconfig files, r=mayhemer

Keywords: checkin-needed

Comment 18

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Attachment #9022722 - Attachment is obsolete: false
Attachment #9022722 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.