Closed Bug 596476 Opened 11 years ago Closed 11 years ago

Fixup of smart_size cache code


(Core :: Networking: Cache, defect)

Not set



Tracking Status
blocking2.0 --- beta7+


(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)


(Blocks 1 open bug)



(4 files, 1 obsolete file)

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.
Attachment #475395 - Flags: review?(jst)
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.
Attachment #475396 - Flags: review?(jst)
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.
Attachment #475398 - Flags: review?(jst)
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.
Attachment #475399 - Flags: review?(jst)
I'd like to land all these with the fix for bug 595413.
blocking2.0: --- → ?
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.

Attachment #475395 - Flags: review?(jst) → review+
Attachment #475396 - Flags: review?(jst) → review+
Attachment #475398 - Attachment is patch: true
Attachment #475398 - Attachment mime type: application/octet-stream → text/plain
Attachment #475398 - Flags: review?(jst) → review+
Blocking, though this is probably not super critical.
blocking2.0: ? → betaN+
Closed: 11 years ago
Resolution: --- → FIXED
Damn, wrong bug.  HG changeset in comment 7 is bogus
Resolution: FIXED → ---
Blocks: http_cache
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)
Attachment #475398 - Attachment is obsolete: true
Attachment #476170 - Flags: review+
Depends on: 559942
Blocks: 598007
(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.
blocking2.0: betaN+ → ?
No longer blocks: 598007
Very bad, and this probably caused bug 598007.
Blocks: 598007
Duplicate of this bug: 597658
blocking2.0: ? → beta7+
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))

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.
Attachment #475399 - Flags: review?(jst) → review+
(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?
> Name use GECKO_2_0 here instead of FF4, as this is not specific to Firefox.

Oops--thanks, nice catch.  Fixed.
Assignee: nobody → jduell.mcbugs
You need to log in before you can comment on or make changes to this bug.