Last Comment Bug 783755 - new value of browser.cache.disk.smart_size.use_old_max doesn't stick
: new value of browser.cache.disk.smart_size.use_old_max doesn't stick
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cache (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla18
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
Mentors:
Depends on: 786086
Blocks: 709297
  Show dependency treegraph
 
Reported: 2012-08-18 02:21 PDT by Jo Hermans
Modified: 2012-10-29 15:52 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
patch (1.61 KB, patch)
2012-08-21 11:32 PDT, Nicholas Hurley [:nwgh][:hurley]
no flags Details | Diff | Review
patch (4.90 KB, patch)
2012-08-22 15:22 PDT, Nicholas Hurley [:nwgh][:hurley]
no flags Details | Diff | Review
Functionality fix (13.15 KB, patch)
2012-08-23 13:04 PDT, Nicholas Hurley [:nwgh][:hurley]
michal.novotny: review+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Jo Hermans 2012-08-18 02:21:07 PDT
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.
Comment 1 Nicholas Hurley [:nwgh][:hurley] 2012-08-21 09:47:22 PDT
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 :)
Comment 2 Nicholas Hurley [:nwgh][:hurley] 2012-08-21 09:52:04 PDT
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.
Comment 3 Nicholas Hurley [:nwgh][:hurley] 2012-08-21 11:32:57 PDT
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
Comment 4 Nicholas Hurley [:nwgh][:hurley] 2012-08-21 14:14:42 PDT
Comment on attachment 653867 [details] [diff] [review]
patch

Ignore this patch, it's causing intermittent deadlocks on try.
Comment 5 Nicholas Hurley [:nwgh][:hurley] 2012-08-22 15:22:57 PDT
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?
Comment 6 Nicholas Hurley [:nwgh][:hurley] 2012-08-22 15:24:27 PDT
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.
Comment 7 Nicholas Hurley [:nwgh][:hurley] 2012-08-23 13:04:49 PDT
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.
Comment 8 Nicholas Hurley [:nwgh][:hurley] 2012-08-27 09:23:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/28c1e4960596
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-27 19:19:48 PDT
https://hg.mozilla.org/mozilla-central/rev/28c1e4960596
Comment 10 Josh Aas 2012-09-08 13:32:55 PDT
Comment on attachment 654385 [details] [diff] [review]
patch

Can Michal review this instead of Jason?
Comment 11 Michal Novotny (:michal) 2012-10-01 17:53:05 PDT
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.
Comment 12 Nicholas Hurley [:nwgh][:hurley] 2012-10-29 14:33:37 PDT
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.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-29 15:27:51 PDT
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.
Comment 14 Nicholas Hurley [:nwgh][:hurley] 2012-10-29 15:49:03 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/02f8117afe40

Note You need to log in before you can comment on or make changes to this bug.