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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jo Hermans]

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

[u408661]

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

[u408661]

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

[u408661]

Attachment: 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

[u408661]

Comment on attachment 653867 [details] [diff] [review]
patch

Ignore this patch, it's causing intermittent deadlocks on try.

Comment 5

[u408661]

Attachment: 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

[u408661]

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

[u408661]

Attachment: 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

[u408661]

https://hg.mozilla.org/integration/mozilla-inbound/rev/28c1e4960596

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28c1e4960596

Comment 10

[Josh Aas]

Comment on attachment 654385 [details] [diff] [review]
patch

Can Michal review this instead of Jason?

Comment 11

[Michal Novotny [:michal]]

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

[u408661]

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]

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

[u408661]

https://hg.mozilla.org/releases/mozilla-beta/rev/02f8117afe40