Closed Bug 1455723 Opened 6 years ago Closed 5 years ago

Firefox59 does not properly honor cache size set in autoconfig files

Categories

(Core :: Networking: Cache, defect, P3)

52 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: jmp242, Assigned: michal)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

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.
Component: Untriaged → File Handling
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
Priority: -- → P3
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
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)
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.
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.
Whiteboard: [necko-triaged]
Attached 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+
Keywords: checkin-needed

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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Attachment #9022722 - Attachment is obsolete: false
Attachment #9022722 - Attachment is obsolete: true
Regressions: 1554466
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: