new value of browser.cache.disk.smart_size.use_old_max doesn't stick

RESOLVED FIXED in Firefox 17

Status

()

Core
Networking: Cache
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Jo Hermans, Assigned: nwgh)

Tracking

Trunk
mozilla18
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Bug 709297 added the browser.cache.disk.smart_size.use_old_max config value to determine if the old (1GB) or the new (350MB) cache size is supposed to be used. It defaults to true, but will be set to false when a new cache folder is being created (see MarkStartingFresh).

When I reset the cache (by deleting the cache folder, not by clearing it), I can see that browser.cache.disk.smart_size.use_old_max is now set to false, and I have a 350MB cache instead of a 1GB one. However, I noticed that it was displayed in a regular font, in about:config, not bold, meaning that it was still supposed to be the default (which is true), and nothing was written out to disk.

When I restarted later, I noticed that the the setting was indeed reverted back to its default, which means that the cache is back to 1GB.
(Reporter)

Updated

5 years ago
Assignee: nobody → hurley
Jo, what is the value of browser.cache.disk.smart_size_cached_value in the profile where you've seen this behavior? Also, what does about:cache show for the maximum storage size of the disk cache device? This might just be an inconsistency between values that are displayed and values that actually matter for operation. I'm seeing similar behavior here, but the values that matter (the ones I asked about) are all what they should be.

This of course raises the larger question of why we display values we aren't using, but that's an issue for another bug :)
Jo, nevermind. I realized that eventually the actual size *will* get set to the 1GB value, although for a few minutes after the fresh start, the size will be the 350MB. For the record, I still believe there's an issue with the value we display, as well. I'll attempt to fix both issues with one patch.
Created attachment 653867 [details] [diff] [review]
patch

This fixes both the actual issue (non-stickyness of the pref) as well as the display issue in the advanced preferences UI (which would show the old max, even if it wasn't in effect). Running through try now https://tbpl.mozilla.org/?tree=Try&rev=79baa582baa8
Attachment #653867 - Flags: review?(jduell.mcbugs)
Comment on attachment 653867 [details] [diff] [review]
patch

Ignore this patch, it's causing intermittent deadlocks on try.
Attachment #653867 - Flags: review?(jduell.mcbugs)
Created attachment 654385 [details] [diff] [review]
patch

Here's a patch that avoids the deadlocks... apparently setting a pref causes pref listeners to be called immediately. When I'm already holding the lock that the pref listener ends up wanting to hold, that's a Bad Thing.

I'm not necessarily a fan of the way I mitigated the locking issue (by having the pref listener not do the sub-portion of work that requires the lock), hence the f? instead of r? I could also do something where I set that pref in an event that happens later on the main thread (the pref setting causing the issue is purely cosmetic - the advanced prefs UI shows the wrong value until the pref gets changed). Thoughts?
Attachment #653867 - Attachment is obsolete: true
Attachment #654385 - Flags: feedback?(jduell.mcbugs)
Forgot to say, this is all good on try https://tbpl.mozilla.org/?tree=Try&rev=cf1afef49603 so we know the fixing of deadlocks works.

The only thing the patch still needs is the minor tweaks Christian suggested in bug  709297, but those are purely cosmetic.
Created attachment 654754 [details] [diff] [review]
Functionality fix

Let's do it this way, so we can get the functionality fix in before the merge on monday. Here's a patch to make the change actually stick (as well as two minor non-functional changes requested by Christian in the initial bug). We can fix the display of the value in a followup that may (or may not) be uplifted to aurora, depending on how release drivers feel about the change.
Attachment #654754 - Flags: review?(michal.novotny)

Updated

5 years ago
Attachment #654754 - Attachment is patch: true
Attachment #654754 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c1e4960596
Status: NEW → ASSIGNED
Depends on: 786086
https://hg.mozilla.org/mozilla-central/rev/28c1e4960596
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 10

5 years ago
Comment on attachment 654385 [details] [diff] [review]
patch

Can Michal review this instead of Jason?
Attachment #654385 - Flags: feedback?(jduell.mcbugs) → feedback?(michal.novotny)
Comment on attachment 654385 [details] [diff] [review]
patch

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

::: netwerk/cache/nsCacheService.cpp
@@ +1631,5 @@
>          }
>  
> +        if (!async) {
> +            nsCacheService::gService->mObserver->QuiesceCapacityPref(true);
> +        }

I would prefer to always post an event in nsCacheService::MarkStartingFresh(), so we could always simply set the pref value without dealing with the lock re-entrancy.
Attachment #654385 - Flags: feedback?(michal.novotny)
Comment on attachment 654754 [details] [diff] [review]
Functionality fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 709297
User impact if declined: Cache will get resized twice after each unclean restart, once to the intended new max size, and afterwards, once to the original size.
Testing completed (on m-c, etc.): on m-c and m-a for a while
Risk to taking this patch (and alternatives if risky): Relatively low. If we opt not to take this patch (or its companion in bug 786086), we should back out bug 709297 AND add a patch to delete the pref created by the patch in that bug, to avoid the situation the patch was designed to avoid in the first place.
String or UUID changes made by this patch: None

Note as mentioned above, both this and the patch in bug 786086 should be either approved or denied together.
Attachment #654754 - Flags: approval-mozilla-beta?
Comment on attachment 654754 [details] [diff] [review]
Functionality fix

It's been on m-c/m-a for a while so we'll take this in our next beta (Beta 4) going to build tomorrow.  Will also approve bug 786086 as well, please land to branches asap.
Attachment #654754 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/02f8117afe40
status-firefox17: --- → fixed
You need to log in before you can comment on or make changes to this bug.