Closed Bug 961024 Opened 6 years ago Closed 6 years ago

Decide where to store HTTP cache v2 data on Android

Categories

(Core :: Networking: Cache, defect)

Other Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mayhemer, Assigned: mayhemer)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Based on advice from bsmith we currently store all cache data (300MB+) in the Android's context cache directory, see [1].

According https://bugzilla.mozilla.org/show_bug.cgi?id=921142#c2 however it seems the reasonable limit is only 1MB for this cache dir content and there is no auto-eviction from OS at all.

So, also as we already have a (close to land) code for intelligent eviction of our own, should we rather move the HTTP cache storage to local profile, as the current cache (v1) does?


[1] http://hg.mozilla.org/mozilla-central/annotate/b53589696cf8/netwerk/cache2/CacheFileIOManager.cpp#l948
Flags: needinfo?(mark.finkle)
I think we should consider using the Android context cache as a start, although I would like a refresher on the prs/cons of doing that. I thought at one time that Android did not count that space against your "Size on Disk" footprint.

I am concerned that Android would flip out if we start putting 250MB worth of data in there, but we still might want to try it and see.

If we see weird issues, we can always move it back to the profile. Wait... profiles!

We have a Guest Browsing mode that uses a separate profile under the covers. The Android context cache does not have a concept of profiles. Therefore, I think we need to use the existing profile based system.

NI'ing Brian to confirm my suspicion.
Flags: needinfo?(mark.finkle) → needinfo?(bnicholson)
It would be nice to use the default cache location for a couple of reasons:
1) The "Clear cache" button in the app settings does the right thing and clears the cache
2) The cache is automatically cleared for us if we're running out of space

I'm not sure whether the cache counts toward the reported app size or not; if not, that's another bonus.

(In reply to Honza Bambas (:mayhemer) from comment #0)
> According https://bugzilla.mozilla.org/show_bug.cgi?id=921142#c2 however it
> seems the reasonable limit is only 1MB for this cache dir content and there
> is no auto-eviction from OS at all.

I think the 1MB given in the Android docs is just an arbitrary limit they picked as an example; reading the docs, the impression I get is that the cache size limit is up for the app developer to decide.

I agree with the profile concerns Mark mentioned. Even though Fennec isn't multi-profile now (excluding guest mode), it would be good to plan for it since we're building this from the ground up anyway. Could we add dirs in the context cache directories that match the Fennec profile names? That is, if Fennec has profiles X, Y, and Z, can we create X, Y, and Z subdirs in the context cache directory to avoid conflicts?
Flags: needinfo?(bnicholson)
(In reply to Brian Nicholson (:bnicholson) from comment #2)
> I agree with the profile concerns Mark mentioned. Even though Fennec isn't
> multi-profile now (excluding guest mode), it would be good to plan for it
> since we're building this from the ground up anyway. Could we add dirs in
> the context cache directories that match the Fennec profile names? That is,
> if Fennec has profiles X, Y, and Z, can we create X, Y, and Z subdirs in the
> context cache directory to avoid conflicts?

Definitely can.  I just need the info which if the profiles (X, Y or Z) is being used.

Question is if android also deletes files in subdirs, (the cache is even now in subdisrs, so no blocker for profile distinction).  According what the cache size info shows when viewing Firefox app status in settings, it seems like android is at least aware of files even in subdirs.
Mark, how can I recognize the profile dir used?
Flags: needinfo?(mark.finkle)
(In reply to Honza Bambas (:mayhemer) from comment #4)
> Mark, how can I recognize the profile dir used?

You could could getDir("ProfD") and pulloff the leaf folder name. That's probably easier that using nsIToolkitProfileService.selectedProfile and getting the folder (as an nsIFile). Either way, I think you need to grab the leaf folder from an nsIFile.
Flags: needinfo?(mark.finkle)
I received some comments related to a blog post about the (old) cache on Firefox for Android: http://gbrownmozilla.wordpress.com/2013/06/19/firefox-for-androids-disk-cache/#comments. Some people are frustrated that the cache (and profile) is on /data, rather than /sdcard -- something to keep in mind.
(In reply to Geoff Brown [:gbrown] from comment #6)
> I received some comments related to a blog post about the (old) cache on
> Firefox for Android:
> http://gbrownmozilla.wordpress.com/2013/06/19/firefox-for-androids-disk-
> cache/#comments. Some people are frustrated that the cache (and profile) is
> on /data, rather than /sdcard -- something to keep in mind.

The http cache storage location can be altered with browser.cache.disk.parent_directory pref.  This can be exposed somewhere to UI with drop down options like /data, /cache, /sdcard or |custom|.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
- on android (widget) we add to the CACHE_DIRECTORY env (when set) name of the profile (ProfD leaf native name)
Attachment #8428719 - Flags: review?(mark.finkle)
Comment on attachment 8428719 [details] [diff] [review]
v1

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

If I read this correctly, this will start a new cache directory in the new location, but leave the old (~200M dir) where it currently is, further bloating the profile so FatFennec can now use 400M of storage?
Comment on attachment 8428719 [details] [diff] [review]
v1

This looks like it should create folders in the AppCache based on the current profile.

I have the same question as Gian-Carlo: What do we need to do to cleanup the existing cache2 folder in the AppCache? Do we think that Android will just cleanup all the files eventually (since it's the AppCache)?
>What do we need to do to cleanup the existing cache2 folder in the AppCache?

It's not in the AppCache right now, is it?
By "AppCache" you mean the html5 offline application cache?  What that should have in common with HTTP cache location?

According cleanup, that is tricky, but we do some cleanup magic in bug 1014394 around it, so it could be used for any old mobile cache data wipe out as well.
Depends on: 1014394
>By "AppCache" you mean the html5 offline application cache?  What that should have in common with HTTP cache location?

The Android temporary storage you get when you call getCacheDir(): http://developer.android.com/guide/topics/data/data-storage.html#filesInternal and which is shown as "Cache" when the user looks at the application stats.

It has the interesting property that files there will magically disappear when Android is low on space.

As far as I know, this is only used by nsDeleteDir so far, not cache2 itself.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> >By "AppCache" you mean the html5 offline application cache?  What that should have in common with HTTP cache location?
> 
> The Android temporary storage you get when you call getCacheDir()

Ah, this cache.  It has so many names :)

> 
> As far as I know, this is only used by nsDeleteDir so far, not cache2 itself.

No, cache2 is using it too: http://hg.mozilla.org/mozilla-central/annotate/e017c15325ae/netwerk/cache2/CacheFileIOManager.cpp#l1343

The code in nsDeleteDir should probably be removed so that upper levels take care to set the dir explicitly.  I have to check on that code once more.
Depends on: 1017546
Attached patch v2Splinter Review
- apply on top of bug 1017546
- as in v1, we inject profile dir leaf name between the patch and "cache2"
- this patch also deletes cache2 + cache2.Trash* located in the root of the android cache dir (so called "profileless" directory)
- I'm keeping the profileless dir at a new member IOMan::mCacheProfilelessDirectory, I personally don't want to parse mCacheDirecory, seems to me potentially unreliable
Attachment #8428719 - Attachment is obsolete: true
Attachment #8428719 - Flags: review?(mark.finkle)
Attachment #8431162 - Flags: review?(michal.novotny)
Attachment #8431162 - Flags: feedback?(mark.finkle)
Comment on attachment 8431162 [details] [diff] [review]
v2

This looks OK to me. I wish we didn't need to put the extra code in for the "profileless" directory, but that will clean up the current cache2 folder.

Can we remove the extra "profileless" code in Fx33? As far as I know the "profileless" cache2 folder will only exist on Fx32 Nightly. It should not exist on Fx31 (Aurora) right? I mean, once this patch lands :)
Attachment #8431162 - Flags: feedback?(mark.finkle) → feedback+
Blocks: 1018192
(In reply to Mark Finkle (:mfinkle) from comment #16)
> Comment on attachment 8431162 [details] [diff] [review]
> v2
> 
> This looks OK to me. I wish we didn't need to put the extra code in for the
> "profileless" directory, but that will clean up the current cache2 folder.
> 
> Can we remove the extra "profileless" code in Fx33? As far as I know the
> "profileless" cache2 folder will only exist on Fx32 Nightly. It should not
> exist on Fx31 (Aurora) right? I mean, once this patch lands :)

Filed bug 1018192 on it.
Comment on attachment 8431162 [details] [diff] [review]
v2

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

::: netwerk/cache2/CacheStorageService.cpp
@@ +593,5 @@
>      nsDeleteDir::RemoveOldTrashes(mCache2Dir);
>    }
> +#if defined(MOZ_WIDGET_ANDROID)
> +  if (mCache2Profileless) {
> +    nsDeleteDir::RemoveOldTrashes(mCache2Profileless);

this RemoveOldTrashes call can be removed since with MOZ_WIDGET_ANDROID the trash dirs are always looked for in the root of the android cache directory.  So, it's actually redundant with nsDeleteDir::RemoveOldTrashes call in the if (mCache2Dir) branch.
Attachment #8431162 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/757c578c3574
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1024495
See Also: → 1388883
You need to log in before you can comment on or make changes to this bug.