596476 - Fixup of smart_size cache code

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

Attachment: Corrects previous shipped cache size: is 50 MB, not 250

This is a bunch of patches for fixes I made while debugging the smart_size cache code crash (bug 595413)

The first patch fixes our algorithm which uses smart_size unless the user manually set a size lower than our old default (50 MB).  We had 250 MB instead.

Comment 1

[Jason Duell]

Attachment: Covert all cache sizes to KB; fix a few errors

We were mixing bytes and KB, and in several places were accidentally assigning byte fields to mDiskCacheCapacity, which takes kb.

Converted everything to KB to make life less error-prone.

Comment 2

[Jason Duell]

Attachment: Use old cache size while waiting for smart size to be updated

Use previous cache size while we wait for smart_size update, instead of MAX_SIZE (1 GB).  If something goes wrong with determinig smart size, we don't want to have set the user's cache by default to 1 GB, and even using it temporarily seems weird.

Comment 3

[Jason Duell]

Attachment: Ensure correct cache directory is used for smart_size check

We get the cache dir by checking a number of prefs.  The previous code only checked one pref.  Now we send directory path to smart_size event to ensure it has correct dir.

Comment 4

[Jason Duell]

I'd like to land all these with the fix for bug 595413.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 475395 [details] [diff] [review]
Corrects previous shipped cache size: is 50 MB, not 250

+// Default cache size was 50 MB for many years until FF 4:
+const PRInt32 PRE_FF4_DEFAULT_CACHE_SIZE = 50 * 1024 * 1024;

Name use GECKO_2_0 here instead of FF4, as this is not specific to Firefox.

r=jst

Comment 6

[Johnny Stenback (:jst)]

Blocking, though this is probably not super critical.

Comment 7

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/e4843996dd06

Comment 8

[Jason Duell]

Damn, wrong bug.  HG changeset in comment 7 is bogus

Comment 9

[Jason Duell]

Attachment: Use old cache size while waiting for smart size to be updated AND set default manual sz to 1 GB

Minor fix: first time new cache code run, if user didn't set sz < 50 mb, set manual setting to 1 GB as starting point for mods (see bug 559942 comment 65)

Comment 10

[chris hofmann]

(In reply to comment #6)
> Blocking, though this is probably not super critical.

sounds like it might help with #1-2 top crash bug 598007.  we should try to move up from betaN+ if that is the case.

Comment 11

[Benjamin Smedberg]

Very bad, and this probably caused bug 598007.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 475399 [details] [diff] [review]
Ensure correct cache directory is used for smart_size check

- In nsCacheProfilePrefObserver::GetSmartCacheSize(const nsAString& cachePath):

+  nsCOMPtr<nsILocalFile> 
+      cacheDirectory (do_CreateInstance(NS_LOCAL_FILE_CONTRACTID));
+  rv = cacheDirectory->InitWithPath(cachePath);
+  if (NS_FAILED(rv))
     return DEFAULT_CACHE_SIZE;

Given what we're hoping this might fix, I'd feel a bit more comfortable if we null checked cacheDirectory after creating it above.

r=jst with that.

Comment 14

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/f26eacbf7a3b
http://hg.mozilla.org/mozilla-central/rev/e705df4f708c
http://hg.mozilla.org/mozilla-central/rev/ecac1d09d40c
http://hg.mozilla.org/mozilla-central/rev/9c44ec22e1f4

Comment 15

[[not reading bugmail]]

(In reply to comment #5)
> Name use GECKO_2_0 here instead of FF4, as this is not specific to Firefox.

Jason, I don't see this comment addressed, is that in a followup?

Comment 16

[Jason Duell]

> Name use GECKO_2_0 here instead of FF4, as this is not specific to Firefox.

Oops--thanks, nice catch.  Fixed.

http://hg.mozilla.org/mozilla-central/rev/942b5b6e74e4