239254 - [Linux] Support disk cache on a local path

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement, P5)

Description

[benc]

from bug 74085: a LINUX only bug.

(This bug applies to all forms of UNIX, including Mac OS X. However, a Mac OS X
solution should be discussed in yet another bug, because of legacy support for
HFS and AppleShare volumes.)

PROBLEM: by default, we use a cache directory that is a sub-directory of the
profile. If the profile is NFS mounted, the cache is NFS mounted.

Minimally, this has causes a two-fold increase in most network requests, because
data streams from the origin server to the client and then is written to the NFS
server.

I would have expected more complaints, but after thinking further, I realized
that a cache-based read would pull from the NFS server, which means that this
bug doesn't really result in a two-fold increase in network utilization,
although it does force virtually all non-mem cache hits to generate NFS traffic. 

At any rate, the fix for Windows will differ substantially from UNIX, because
newer Windows environments support the existence of local-only user space when
using roaming profiles.

SOLUTION:
I know that we want to let users select this, ideally upon install or profile
creation. The problem is that you can't set the path in all.js -or- prefs.js.

Using prefs.js, the profile would store the data, so this would assume that the
user-entered path would be valid on all systems. NFS mounts across different
UNIX implementations would possibly break. (You can actually do this now, by
hand, but nobody has said they use this as a workaround, so I'm assuming the
idea is bankrupt)

Using all.js would create a single path for all users. We would need to add
support for some "~" of username expansion. This also assumes that mozilla is
installed locally. If mozilla were run from NFS, then this solution would also
incur the problems described above for prefs.js.

We can't blindly use temp space, because on some systems it is small, on other
systems, it is mapped to memory. We also have a couple "disk cache grows in
crazy ways" bugs, so this would be completely out of the question.

What we might need is a new, system-level file that administrators could hack
that we read for the local-path. If so, we would probably need to add another
pref that favors a the sys admin's choice over the profile's choice.

At any rate, getting this to work appears to be non-trivial, there are a variety
of design and system issues that need to be tackled at once. Contributors that
fix this for one platform should look for a solution that potentially helps
other platforms as well.

Comment 1

[Brad Garcia]

> Using prefs.js... You can actually do this now, by hand...

I would be happy to use this as a temporary solution for now.
Could you explain what needs to be set within prefs.js?

Comment 2

[Darin Fisher]

So, my patch for bug 291033 enables support for this on Windows and OSX using
platform defaults for the local path.  There is no good default for the local
path on UNIX, so we can't really do something by default there.  That said, we
should probably support an environment variable or something that would set the
local path.

I see this as pretty low-priority, so if someone wants to make this happen,
please write a patch.  It should be pretty easy.  You just need to modify
nsXREDirProvider::GetUserDataDirectory.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

*** Bug 319799 has been marked as a duplicate of this bug. ***

Comment 4

[Bryce Mozilla Nesbitt]

Noet: There may be other types of cache files that sysadmins may want to keep locally. XUL fastload files, and (for Mozilla Suite) anything IMAP.  What files must be in the profile, and which can be recreated on demand?

Moving the "can be recreated on demand" files local to the machine is good.  But it creates an issue also: every time a user roams from a machine, they'll leave tracks.  This is a minor security issue, and also a temp space management issue.

See also: http://kb.mozillazine.org/Browser.cache.disk.parent_directory

Comment 5

[xofer]

> See also: http://kb.mozillazine.org/Browser.cache.disk.parent_directory

The problem with browser.cache.disk.parent_directory is that this setting is stored in the profile. In the case of a remote profile (NFS, WebDAV, etc.), the local file system may be different on each access.

Why not add an option to profiles.ini e.g. "Cache=..." like so:

[Profile0]
Name=remote
IsRelative=0
Path=/mnt/remote/firefox
Cache=~/.mozilla/Cache

Comment 6

[Chris Tracy]

I'm trying to solve this problem for a university *nix lab environment.

In reading this bug and #291033 and the code for Firefox 3, I seems that it *is* possible to use the XRE_PROFILE_LOCAL_PATH environment variable to set the "Profile Local" directory on *nix.  However, the code that uses XRE_PROFILE_LOCAL_PATH is only run if you've also set XRE_PROFILE_PATH.

Setting XRE_PROFILE_LOCAL_PATH is exactly what I want, as it allows me to redirect the Cache as well as urlclassifier3.sqlite and everything else deemed not necessarily persistent about a profile.

However, needing to set XRE_PROFILE_PATH for it to work is a problem, since I don't want to redirect the persistent bits of the profile, just the non-persistent bits.  (I'd just set the var to the current location of the profile, but for the <random>.default/ directory it all lives in under ~/.mozilla/firefox/)

Comment 7

[Chris Tracy]

Near as I can tell, a logic change in toolkit/xre/nsAppRunner.cpp would work, to allow XRE_PROFILE_LOCAL_PATH to be used even when XRE_PROFILE_PATH isn't set.  [Inside the function SelectProfile()]

I'm trying to come up with a patch, but have run into issues. Mostly, I don't understand the firefox build process well enough to even figure out how to build xulrunner (which is where nsAppRunner seems to end up) to then test with firefox.  (I'm just a humble sysadmin, my forays into writing patches are few and far between)

There's also the issue of overriding the setting of the profile local path for a profile that already exists.  nsToolkitProfile doesn't seem to provide any means to change mLocalDir after a profile object is created, which would be needed for the common case.  (ie, no args, single profile already exists which we want to use)

Oh, and for those wondering, the reason I like an environment variable so much is that it can be set differently on whatever machine the user is on, allowing different settings for different configs.  It can also easily be set to different values for different users, circumventing the issues you run into trying to use browser.cache.disk.parent_directory to solve this issue.  (ie, I can set b.c.d.p to /tmp, but then all users on the system end up trying to use /tmp/Cache which will fail for all but the user who wins the race to create the directory first)

Comment 8

[Ilia K.]

Attachment: proposed fix

patch to use $XDG_CACHE_HOME when set, otherwise ~/.cache directory as per freedesktop specification at http://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
Note that Google Chrome/Chromium uses the same spec.
For example, directory for disk cache in firefox becomes
~/.cache/mozilla/firefox/profile.name/Cache

Comment 9

[mike]

(In reply to comment #6)
> However, needing to set XRE_PROFILE_PATH for it to work is a problem, since
> I don't want to redirect the persistent bits of the profile, just the
> non-persistent bits.  (I'd just set the var to the current location of the
> profile, but for the <random>.default/ directory it all lives in under
> ~/.mozilla/firefox/)

I'm not seeing what the problem is there. Apart from it being utterly obsqure and cumbersome.

In my test this works as desired as far as I can tell

$ export XRE_PROFILE_PATH=${HOME}/.mozilla/firefox/whatever.default
$ export XRE_PROFILE_LOCAL_PATH=$(mktemp -d)
$ firefox

Firefox then puts the Cache directory in the location specified by XRE_PROFILE_LOCAL_PATH along with urlclassifier3.sqlite and the XUL.mfasl and XPC.mfasl. I've tried this with Firefox 3.6 and Firefox 5.

The trick in a lab environment would be getting XRE_PROFILE_PATH always set to the the location of the user's existing profile, but that should be easy enough to script.

Support for XDG directory specification (see https://bugzilla.mozilla.org/show_bug.cgi?id=259356) would render such faffing unnecessary though, so long as it was done in such a way that everything that gets put in XRE_PROFILE_LOCAL_PATH gets put in a sub directory of XDG_CACHE_HOME.

Comment 10

[Ilia K.]

@bsmedberg: a kind reminder to review the patch in comment #8.

More than two weeks has passed and I still haven't got any feedback. If you feel you're too busy, please suggest another reviewer. If you only have "initial thoughts" I'd be glad to hear & discuss, no need to wait for a "full" review.

Comment 11

[Benjamin Smedberg]

Comment on attachment 541970 [details] [diff] [review]
proposed fix

Sorry for the delay, I've been on vacation.

What is the behavior of this patch for existing profiles?
If ~/.cache doesn't exist (as it doesn't on my machine), what happens for existing and new profiles?

Comment 12

[Ilia K.]

(In reply to comment #11)
> What is the behavior of this patch for existing profiles?
> If ~/.cache doesn't exist (as it doesn't on my machine), what happens for
> existing and new profiles?

The behavior for existing profile will be just like someone has specified a path using
XRE_PROFILE_LOCAL_PATH. For example, if you have a profile directory 
~/.mozilla/firefox/salt.default the following should be equivalent:

1. Running Firefox *without* the patch:
  $ export XRE_PROFILE_PATH=$HOME/.mozilla/firefox/salt.default
  $ export XRE_PROFILE_LOCAL_PATH=$HOME/.cache/mozilla/firefox/salt.default
  $ firefox
2. Running patched Firefox with default profile:
  $ firefox -P salt.default
2.1. Of course, if firefox is configured to use default profile automatically,
     then it's enough to run just
  $ firefox

Now you can look at any firefox behavior when run the 1st way and expect exactly the 
same behavior when run the 2nd way. For example:
- local profile directory is created upon startup, e.g.
  XRE_main() -> nsXREDirProvider::SetProfile() ->
    nsXREDirProvider::EnsureDirectoryExists()
- old Cache directory is removed if exists
  nsCacheProfilePrefObserver::ReadPrefs() -> DeleteDir()

Overall, the patch should not add any behavior which wasn't observed before, either on 
platforms which already support local profile dir (Win, Mac) or under linux with other 
mechanisms for choosing local profile dir (e.g. $XRE_PROFILE_LOCAL_PATH).

An interesting example I've observed even before writing a patch: 
if new profile is created during firefox startup (using profile manager or 
-createprofile command line argument), the first session will assume ProfLD==ProfD, 
i.e. local data is put with the rest of the profile data until firefox is restarted. 
I don't know whether this is a feature or a bug but this has nothing to do with my 
patch (you can even observe this with the unpatched firefox under windows).

Comment 13

[mike]

It might be worth noting that in my tests with setting XRE_PROFILE_PATH and XRE_PROFILE_LOCAL_PATH for existing profiles, the stuff which is put in to XRE_PROFILE_LOCAL_PATH is not removed from XRE_PROFILE_PATH. So you end up with an urlclassifier3.sqlite file and Cache directory in XRE_PROFILE_PATH which aren't being used and new ones in XRE_PROFILE_LOCAL_PATH which are being used. Which is a bit messy and potentially confusing. It's also unhelpful if the reason you want to be using XRE_PROFILE_LOCAL_PATH is to get that stuff out of a home directory that has a quota applied. Ideally anything which is relocated in to XRE_PROFILE_LOCAL_PATH should be automatically purged from XRE_PROFILE_PATH.

I don't know if Ilia K's patch addresses that at all, building patched versions of Firefox is beyond me right now.

Comment 14

[Ilia K.]

For brevity sake, I'll write ProfD for profile data directory and ProfLD for profile local data 
directory hereinafter.

(In reply to comment #13)

AFAIK there is no single place in code with the purpose of getting rid of old stuff in ProfD when 
ProfLD points to a different place. Each subsystem which stores data in ProfLD is responsible to 
check that ProfLD != ProfD and respond accordingly, e.g. by removing ProfD/my_stuff.

My patch is very localized and does not change anything except what is stated: tell the rest of 
mozilla environment that ProfLD is ~/.cache/mozilla/firefox/salt.profile. So for better or worse, 
if you observe some behavior when using $XRE_PROFILE_LOCAL_PATH it's not likely to change as a 
result of this patch.

Comment 15

[Benjamin Smedberg]

Comment on attachment 541970 [details] [diff] [review]
proposed fix

Mike, can you take this review? I think it's correct, although everyone will lose:

1) URL classifier data
2) the startup cache
3) the offline cache
4) the necko disk cache

We may need code to migrate that data if we think it's important...

Comment 16

[mike]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> Comment on attachment 541970 [details] [diff] [review]
> proposed fix
> 
> Mike, can you take this review?

There's no one else called Mike in the thread so I guess you're addressing me. I fear however that you have mistaken me for someone who is qualified to do such a thing. I have experience of building bits and pieces from source but I wouldn't know where to start building a version of Firefox incorporating the patch. I certainly couldn't comment on the technical quality or whatever of the patch. I'm just a guy who'd like to see XDG_CACHE_HOME support in Firefox so that I can stop using the wrapper bash script hack I wrote that sets XRE_PROFILE_PATH and XRE_PROFILE_LOCAL_PATH to suitable values. Sorry.

Comment 17

[Benjamin Smedberg]

You will see that the review request was directed to Mike Hommey, a longtime Mozilla contributor who has maintained the Debian version of Firefox, not yourself ;-).

Comment 18

[Mike Hommey [:glandium]]

And I'll look into it tomorrow.

Comment 19

[mike]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> You will see that the review request was directed to Mike Hommey, a longtime
> Mozilla contributor who has maintained the Debian version of Firefox, not
> yourself ;-).

Phew! :)

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 541970 [details] [diff] [review]
proposed fix

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

> although everyone will lose:
> 1) URL classifier data
> 2) the startup cache
> 3) the offline cache
> 4) the necko disk cache
>
> We may need code to migrate that data if we think it's important...

I think it's not worth caring to migrate the data. However, cleaning up the old data is IMHO important, especially when considering the size of the data.

When the corresponding local cache support was landed for other platforms, code was added to handle the "migration" of the necko disk cache (which simply removes the old cache). http://hg.mozilla.org/mozilla-central/file/3d615a56ad46/netwerk/cache/nsCacheService.cpp#l705

URL classifier doesn't do it. http://hg.mozilla.org/mozilla-central/file/3d615a56ad46/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#l1332
Startup cache doesn't either. http://hg.mozilla.org/mozilla-central/file/3d615a56ad46/startupcache/StartupCache.cpp#l161
Offline cache doesn't either. http://hg.mozilla.org/mozilla-central/file/3d615a56ad46/netwerk/cache/nsCacheService.cpp#l801

It looks like these are the only places that use the local data directory.

::: toolkit/xre/nsXREDirProvider.cpp
@@ +1068,5 @@
> +      dir = cacheHome;
> +    } else {
> +      if (dir.Last()!='/')
> +        dir += '/';
> +      dir += ".cache";

I'd initialize localDir first, and use Append or AppendNative instead.
https://developer.mozilla.org/en/nsIFile#append%28%29

@@ +1315,5 @@
> +  nsCAutoString folder;
> +  // Make it hidden (by starting with "."), except when local
> +  // (when local, don't hide under local UserDataDirectoryHome)
> +  if (!aLocal)
> +    folder.Append('.');

Assign instead of Append. = would work, too.

@@ +1325,5 @@
>        profileStart++;
>  
>      // On the off chance that someone wanted their folder to be hidden don't
>      // let it become ".."
> +    if (*profileStart == '.' && folder.First() == '.')

&& !aLocal instead would seem more appropriate.

Comment 21

[Jason Duell]

Just want to make sure this on Michal/Bjarne's radar.

Comment 22

[Gian-Carlo Pascutto [:gcp]]

This bug is marked [Linux], but would settings those defines work on Windows too? Despite the Local/Roaming config on Windows, there's quite some users requesting the ability to redirect where the Roaming part of the profile is stored (see e.g. bug 359145), so they might benefit from this as well.

Comment 23

[David Benjamin]

Attachment: New version of patch to address review and cleanup old data

I went ahead reworked the patch to address the comments in comment 20. I also added the cleanup code in URL classifier, startup cache, and offline cache.

They're all #ifdef XP_UNIX since that should be the only platform to need a migration, but I can remove the ifdef if that's preferred. I wasn't sure about the "ProfDS" bit, but I noticed StartupCache uses "ProfLDS" there, so I gather they're startup variants? I also couldn't include DeleteDir outside netwerk/cache/, so I just used Remove(true) in the StartupCache code. Not sure if it'd be preferable to do the delete on another thread like DeleteDir does.

Would this sort of thing need tests? Where should I put them?

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 576857 [details] [diff] [review]
New version of patch to address review and cleanup old data

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

r=me with these fixed.

::: netwerk/cache/nsCacheService.cpp
@@ +775,4 @@
>                                     getter_AddRefs(directory));
>              if (!directory)
>                  directory = profDir;
> +#ifdef XP_UNIX

You may want to exclude OSX (same applies on other files).

@@ +784,5 @@
> +                    if (NS_SUCCEEDED(profDir->AppendNative(
> +                          NS_LITERAL_CSTRING("OfflineCache")))) {
> +                        bool exists;
> +                        if (NS_SUCCEEDED(profDir->Exists(&exists)) && exists)
> +                            DeleteDir(profDir, false, false);

nsDeleteDir::DeleteDir(profDir, false);

::: startupcache/StartupCache.cpp
@@ +197,5 @@
> +        if (NS_SUCCEEDED(
> +              profDir->AppendNative(NS_LITERAL_CSTRING("startupCache")))) {
> +          bool exists;
> +          if (NS_SUCCEEDED(profDir->Exists(&exists)) && exists)
> +              profDir->Remove(true);

Checking for existence is unnecessary. profDir->Remove(true); does it already. (same applies on other similar code)

::: toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ +1371,5 @@
> +      rv = oldDBFile->Clone(getter_AddRefs(oldPSFile));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      if (NS_SUCCEEDED(oldDBFile->Equals(mDBFile, &same)) && !same) {
> +        // We no longer store the database in the main profile
> +        // directory, so we should cleanup the old one.

You should initialize oldPSFile here.

Comment 25

[Mike Hommey [:glandium]]

(In reply to David Benjamin from comment #23)
> They're all #ifdef XP_UNIX since that should be the only platform to need a
> migration, but I can remove the ifdef if that's preferred. I wasn't sure
> about the "ProfDS" bit, but I noticed StartupCache uses "ProfLDS" there, so
> I gather they're startup variants?

#define NS_APP_PROFILE_DIR_STARTUP "ProfDS"
#define NS_APP_PROFILE_LOCAL_DIR_STARTUP "ProfLDS"

> I also couldn't include DeleteDir outside
> netwerk/cache/, so I just used Remove(true) in the StartupCache code. Not
> sure if it'd be preferable to do the delete on another thread like DeleteDir
> does.

StartupCache is only one file, basically, so it shouldn't be a big overhead. OfflineCache is another story.

> Would this sort of thing need tests?

Benjamin, do you think we need tests for this?

As a side note, I'm not completely sure we shouldn't try to migrate some of this data, most notably offline cache and necko disk cache. Some people may rely on them being filled (more so for the offline cache). Who could make the call, here?

Comment 26

[David Benjamin]

Attachment: revised patch

Excluded XP_MACOSX, removed existence checks for nsIFile::Remove, moved oldPSFile declaration, and switched to nsDeleteDir::DeleteDir (I guess that changed after the revision I based my patch on).

Comment 27

[Wayne Mery (:wsmwk)]

what about Bug 217146 - browser.cache.disk.parent_directory should be POSIX, not base64

Comment 28

[Matthias Versen [:Matti]]

Comment on attachment 580615 [details] [diff] [review]
revised patch

I assume that you want to request a new review

Comment 29

[Josh Triplett]

Any updates on this?  The patch from comment 26 seems ready for review and inclusion, and seems to incorporate all the outstanding issues raised in this bug.

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 580615 [details] [diff] [review]
revised patch

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

Sorry for having let this slip for so long. This looks good to me, but I'd like a network peer to look at the cache "migration" part.

Comment 31

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 580615 [details] [diff] [review]
revised patch

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

The offline cache is used for web apps so I am not sure it should be considered a cache in the same way that the HTTP cache is considered a cache. In Windows terms, I would expect that the offline cache should be stored in the Roaming part of the profile vs. the Local part. But, maybe it is OK to blow away the offline cache data for an installed web app, because we'll just re-cache it the next time we access it? Honza? To test this, we'd have to install an app, load it, then quit Firefox, change $XDG_CACHE_HOME to point to a different path, and then start Firefox and verify that the installed app can still be loaded.

Comment 32

[Mike Hommey [:glandium]]

(In reply to Brian Smith (:bsmith) from comment #31)
> The offline cache is used for web apps so I am not sure it should be
> considered a cache in the same way that the HTTP cache is considered a
> cache. In Windows terms, I would expect that the offline cache should be
> stored in the Roaming part of the profile vs. the Local part.

The code in nsCacheProfilePrefObserver::ReadPrefs defaults to putting it in the local part, even on Windows. What the patch does is add a separate local part on linux, which mac and windows already have, and add some cleanup for the migration. The question is whether throwing away offline cache works, or if it should be moved instead. Or left alone, for that matter. (That is, we could just decide that if there is an offlinecache in the roaming part, we leave it there)

Comment 33

[Honza Bambas (:mayhemer)]

David, please resubmit the patch with

[defaults]
diff=-U 8 -p
qdiff=-U 8 

[diff]
git=true
showfunc=true
unified=8

in you hgrc.  Thanks.

(In reply to Mike Hommey [:glandium] from comment #32)
> (In reply to Brian Smith (:bsmith) from comment #31)
> > The offline cache is used for web apps so I am not sure it should be
> > considered a cache in the same way that the HTTP cache is considered a
> > cache. In Windows terms, I would expect that the offline cache should be
> > stored in the Roaming part of the profile vs. the Local part.

This is questionable.  If user has access to internet, then a web app will automatically download and cache it self on next access.  W/o any prompts, since the offline permission is carried in the roaming part of the profile.

Only thing we break is allowing a web app to work also offline.  Since it may work with local user data in localStorage or IndexedDB, we still have the data but no code to access it.

Based on the last line, I might start thinking that moving web app offline caches to the roaming part could have some benefits.

> 
> The code in nsCacheProfilePrefObserver::ReadPrefs defaults to putting it in
> the local part, even on Windows. What the patch does is add a separate local
> part on linux, which mac and windows already have, and add some cleanup for
> the migration. The question is whether throwing away offline cache works, or
> if it should be moved instead. Or left alone, for that matter. (That is, we
> could just decide that if there is an offlinecache in the roaming part, we
> leave it there)

We can have separate offline device for "normal" offline cache that stores to local part (if there are actually any consumers of this feature...) and "web apps" offline cache that stores to the roaming part.  We already move a way to have more then one offline cache in the process anyway.

Comment 34

[David Benjamin]

Attachment: better diff format, rebased, migrate page thumbnails

Here's the patch with the new diff options. I've also rebased it to tip and resolved some conflicts. I also made do the same sort of "migration" for the page thumbnails since those have since been added. (I just bumped the version number and had it trigger removeThumbnailsFromRoamingProfile again.)

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 649099 [details] [diff] [review]
better diff format, rebased, migrate page thumbnails

Michal, I think you are more suitable to review this patch them I'm.

Comment 36

[David Benjamin]

Wow, the hot potato has been passed around quite a bit. Michal, have you had a chance to look at the patch? I'd like to avoid having to keep rebasing it over and over... I'm going to have to add new code every time someone adds a feature in ProfLD.

Comment 37

[Michal Novotny [:michal]]

Comment on attachment 649099 [details] [diff] [review]
better diff format, rebased, migrate page thumbnails

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

::: netwerk/cache/nsCacheService.cpp
@@ +749,5 @@
>              NS_GetSpecialDirectory(NS_APP_USER_PROFILE_LOCAL_50_DIR,
>                                     getter_AddRefs(directory));
>              if (!directory)
>                  directory = profDir;
> +#if defined(XP_UNIX) && !defined(XP_MACOSX)

This isn't necessary since the offline cache is already stored on the local path on other systems.

@@ +759,5 @@
> +                    if (NS_SUCCEEDED(profDir->AppendNative(
> +                          NS_LITERAL_CSTRING("OfflineCache")))) {
> +                        bool exists;
> +                        if (NS_SUCCEEDED(profDir->Exists(&exists)) && exists)
> +                            nsDeleteDir::DeleteDir(profDir, false);

The directory doesn't need to be deleted immediately, so some delay should be specified (e.g. 60 seconds like in nsDiskCacheDevice::OpenDiskCache()). The same should be done when deleting "Cache" directory.

Comment 38

[David Benjamin]

Attachment: Rebased, deleted delete, removed ifdefs

Rebased the patch. I removed the nsUrlClassifierDBService.cpp bit since that code seemed to have been rewritten, and if this lands in the same release there won't be any need to migrate anything.

> This isn't necessary since the offline cache is already stored on the local
> path on other systems.

Okay, I've removed them.

> The directory doesn't need to be deleted immediately, so some delay should be
> specified (e.g. 60 seconds like in nsDiskCacheDevice::OpenDiskCache()). The
> same should be done when deleting "Cache" directory.

Done.

Comment 39

[Gian-Carlo Pascutto [:gcp]]

>I removed the nsUrlClassifierDBService.cpp bit since that code seemed to have been 
>rewritten, and if this lands in the same release there won't be any need to 
>migrate anything.

Probably just need to remember this bug when fixing bug 723153.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 653240 [details] [diff] [review]
Rebased, deleted delete, removed ifdefs

Factoring out the copy/pasted code in nsCacheProfilePrefObserver::ReadPrefs( might be nice, but otherwise seems fine.

Comment 41

[Martin Stránský [:stransky] (ni? me)]

David, what's a plan for this bug? Are you going to ask for check-in?

Comment 42

[David Benjamin]

Oh yeah, this guy... how does asking for a check-in work? I'm not familiar with Mozilla's processes.

Comment 43

[Boris Zbarsky [:bzbarsky]]

You add the "checkin-needed" keyword to the bug.

Before doing that it's best to upload a patch with a commit message and user info already set, in general.

Comment 44

[Benjamin Smedberg]

I tried to do a try run, but the patch doesn't apply cleanly:

$ hg qimport -n bug239254 https://bugzilla.mozilla.org/attachment.cgi?id=653240
adding bug239254 to series file
[bsmedberg@stravinsky (hg:default) ~/builds/mozilla-central/src]
$ hg qpush
applying bug239254
patching file browser/components/thumbnails/PageThumbs.jsm
Hunk #1 FAILED at 6
Hunk #2 FAILED at 338
2 out of 2 hunks FAILED -- saving rejects to file browser/components/thumbnails/PageThumbs.jsm.rej
patching file toolkit/xre/nsXREDirProvider.cpp
Hunk #2 FAILED at 1220
Hunk #3 succeeded at 1359 with fuzz 1 (offset 16 lines).
Hunk #4 FAILED at 1404
2 out of 4 hunks FAILED -- saving rejects to file toolkit/xre/nsXREDirProvider.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh bug239254

I tried to apply the rejects manually but there appears to be a nontrivial merge required in PageThumbs.jsm: LATEST_STORAGE_VERSION is now 2, and there is some removal/migration code between versions which has changed.

Comment 45

[David F.]

From what I tried, the changes to browser/components/thumbnails/PageThumbs.jsm are obsolete and can be removed from the change set since v16.0.

Comment 46

[Michal Novotny [:michal]]

Some time ago biesi suggested on IRC that we should try to move/rename the old cache instead of deleting it. It should be fast if the new destination is on the same FS and it should fail in case it isn't. David, could you please add this change to the patch? This is what biesi wrote on IRC:

(07:29:22 PM) biesi: rename() fails if it's not on the same FS
(07:29:33 PM) biesi: it's possible that our abstractions don't let you do that, but we can fix that!
(07:30:15 PM) biesi  (Linux  permits  a file system to be mounted at multiple points,
(07:30:15 PM) biesi: but rename() does not work across different mount  points,  even
(07:30:16 PM) biesi: if the same file system is mounted on both.)
(07:31:46 PM) biesi: ideally I'd want nsILocalFile to offer a method that's fast-or-fail
(07:31:53 PM) biesi: but using rename directly WFM too

Comment 47

[David Benjamin]

Okay. If it's not on the same filesystem, should it be deleted or just kept around? Which caches should this be for? Just the offline one or everything?

Comment 48

[Michal Novotny [:michal]]

Just delete it if rename() fails. Try to rename both, the disk cache and the offline cache.

Comment 50

[Martin Stránský [:stransky] (ni? me)]

Attachment: v3, latest trunk

Updated patch to latest trunk, added rename. The patch works but there's one thing - thumbnails and thumbnails-old directories are left in the old cache location (i.e. profile dir). Shall we remove the old thumbnails or try to move them?

Comment 51

[Michal Novotny [:michal]]

Comment on attachment 694348 [details] [diff] [review]
v3, latest trunk

> -  // Make it hidden (i.e. using the ".")
> -  nsAutoCString folder(".");
> +  // Make it hidden (i.e. using the ".")  

The comment should be removed.


> +    nsresult rv = aOldCacheSubdir->AppendNative(nsDependentCString(aCacheSubdir));

80 chars max


> +    aNewCacheSubdir->GetNativePath(newPath);

Check the return value (the same in case of oldPath).


> +    if (NS_FAILED(aNewCacheSubdir->Exists(&exists)) || !exists) {
> +        // New cache directory does not exist, try to move the old one here
> +        // rename needs an empty target directory

I would be here probably more strict. The fact that Exists() fails doesn't ensure that the directory doesn't exist. So I would prefer this:

if (NS_SUCCEEDED(aNewCacheSubdir->Exists(&exists)) && !exists)


> +            if(rename(oldPath.get(), newPath.get()) == 0)
> +              return;

Does this compile on Windows? And please fix the indentation.


+    // Delay delete by 1 minute to avoid IO thrash on
+    // startup.

The comment would fit on one line now.


> The patch works but there's one thing - thumbnails and thumbnails-old
> directories are left in the old cache location (i.e. profile dir). Shall we
> remove the old thumbnails or try to move them?

It isn't true that the change in PageThumbs.jsm is no longer needed. See bugs #754671 and #784868. I would guess (but I'm not expert about this code) that we can safely delete old "thumbnails" directory because the bug #754671 reduced it to some sane size. And the fix in bug #784868 should try to search for "thumbnails-old" directory in both locations.

Comment 53

[Martin Stránský [:stransky] (ni? me)]

Attachment: v4

Should address the comments. rename() is supported on windows (http://msdn.microsoft.com/en-us/library/zw5t957f%28v=VS.71%29.aspx) but I'll give it a shot on try.

Comment 54

[Martin Stránský [:stransky] (ni? me)]

Try for v4 - https://tbpl.mozilla.org/?tree=Try&rev=0327abb6a953

Comment 55

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 697043 [details] [diff] [review]
v4

Try looks fine.

Comment 56

[Michal Novotny [:michal]]

Comment on attachment 697043 [details] [diff] [review]
v4

Looks good. The change in PageThumbs.jsm needs a review from browser peer.

Comment 57

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 697043 [details] [diff] [review]
v4

Felipe, I see you've already reviewed the the PageThumbs.jsm, can you please check the changes here or direct the review to the right person? Thanks!

Comment 58

[Tim Taubert [:ttaubert] (inactive)]

Why do we need to increase the thumbnail storage's version number? We don't store thumbnails in the file cache anymore.

Comment 59

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #58)
> Why do we need to increase the thumbnail storage's version number? We don't
> store thumbnails in the file cache anymore.

In which version? I still see the thumbnails in ~/.cache/mozilla/firefox/profile_dir/thumbnails. Have this change landed recently?

Comment 60

[Tim Taubert [:ttaubert] (inactive)]

Talked to Martin and we found out that we not only need to increase the PageThumbs storage's version number but also move the thumbnails directory to its new target.

Comment 61

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 697043 [details] [diff] [review]
v4

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

This looks good but I think we should do the same as the cache service is doing. Try to move existing thumbnails to their new directory first and if that fails we then should just remove the old one. Otherwise we'd just throw away all thumbnails and present an empty new tab page.

Comment 62

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #61)
> This looks good but I think we should do the same as the cache service is
> doing. Try to move existing thumbnails to their new directory first and if
> that fails we then should just remove the old one. Otherwise we'd just throw
> away all thumbnails and present an empty new tab page.

Tim, after some investigation I think we should not move the existing thumbnails from remote profile dir to a local one. It's because nsIFile interface is missing an equivalent to rename() function we have in C/POSIX. The nsIFile.moveTo() does not fail when we try to move files across volumes/disks but it tries to copy the data. So I propose we should just remove the old thumbnails as it's in the recent patch.

Comment 63

[Tim Taubert [:ttaubert] (inactive)]

I don't think we can just throw away existing thumbnails for everyone out there. Isn't there a way to check if those two directories are on the same file system and only then start copying? If not we can throw thumbnails which should then only affect a very tiny minority of our users.

Comment 64

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #63)
> file system and only then start copying?

We want to move files, of course. Not copy.

Comment 65

[Martin Stránský [:stransky] (ni? me)]

(In reply to Tim Taubert [:ttaubert] from comment #63)
> I don't think we can just throw away existing thumbnails for everyone out
> there. Isn't there a way to check if those two directories are on the same
> file system and only then start copying? If not we can throw thumbnails
> which should then only affect a very tiny minority of our users.

Tim, I'm afraid that the nsIFile abstraction does not offer the rename equivalent or any way how to find the volume/mount topology, see comment 46. We can't say when the moveTo()  performs move or copy.

Comment 66

[Benjamin Smedberg]

There is no particular reason why we have to rely on the nsIFile abstraction. Just write some *nix-specific c++ code to do what we need to do.

Comment 67

[Tim Taubert [:ttaubert] (inactive)]

I'm currently preparing a patch for a proper thumbnail migration to the new location that uses OS.File (async file I/O) and that supports moving without copying.

Comment 68

[Tim Taubert [:ttaubert] (inactive)]

Attachment: try to move (and don't copy) existing thumbnails to their new home

Ok, here's a patch that uses OS.File to move existing thumbnails to their new local profile directory on Linux. If we can't move them they're just removed. This all happens on a background thread (that's what OS.File is about).

I seized the chance to clean up our thumbnail storage migration code a little and wrote a test for the migration to version 3. Worked also when trying manually with my trunk profile.

David: sorry for bitrotting your patch from bug 753768 again :/

Comment 69

[David Teller [:Yoric] - still alive but not very active]

(In reply to Tim Taubert [:ttaubert] from comment #68)
> David: sorry for bitrotting your patch from bug 753768 again :/

It's ok. Just don't be surprised if you find Windows 8 installed on your computer during the next work week. Just saying.

Comment 70

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 711987 [details] [diff] [review]
try to move (and don't copy) existing thumbnails to their new home

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

Looks good, with minor changes.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +394,1 @@
>      let roaming = FileUtils.getDir("ProfD", [THUMBNAIL_DIRECTORY]);

Might as well use |OS.Constants.Path.profileDir| and |OS.Constants.Path.localProfileDir| - provided you don't forget to create the directory if necessary.

::: browser/components/thumbnails/PageThumbsWorker.js
@@ +74,5 @@
> +
> +  moveOrDeleteAllThumbnails:
> +  function Worker_moveOrDeleteAllThumbnails(msg) {
> +    if (!OS.File.exists(msg.from))
> +      return true;

Future note: I should make sure that we can test existence of a directory from within the constructor of DirectoryIterator. This will save one I/O operation.

@@ +78,5 @@
> +      return true;
> +
> +    let iter = new OS.File.DirectoryIterator(msg.from);
> +    for (let entry in iter) {
> +      if (!entry.isDir && !entry.isSymLink) {

Nit: I would go for
  if (entry.isDir || entry.isSymLink) {
    continue;
  }

@@ +83,5 @@
> +        let from = OS.Path.join(msg.from, entry.name);
> +        let to = OS.Path.join(msg.to, entry.name);
> +
> +        try {
> +          OS.File.move(from, to, {noOverwrite: true, noCopy: true});

Ok, so this won't move files from a device to another one, is that intended?

@@ +89,5 @@
> +          OS.File.remove(from);
> +        }
> +      }
> +    }
> +

Please don't forget to close the iterator. Not strictly necessary in this context but a good idea nevertheless.

::: browser/components/thumbnails/test/browser_thumbnails_storage_migrate3.js
@@ +47,5 @@
> +  PageThumbsStorageMigrator.migrateToVersion3();
> +  ok(true, "migration finished");
> +
> +  // Wait until the first thumbnail was moved to its new location.
> +  yield whenFileExists(URL);

In real usage scenarios, this will generally not happen because ProfD and ProfLD are on distinct devices. Here, you place them on the same device. That's probably ok, but don't forget to document this somewhere.

@@ +55,5 @@
> +  yield whenFileExists(URL2);
> +  ok(true, "second thumbnail moved");
> +
> +  yield whenFileRemoved(roaming);
> +  ok(true, "roaming thumbnail directory removed");

You haven't checked that files are never overwritten. That's probably not very important in this case, though.

::: browser/components/thumbnails/test/head.js
@@ +228,5 @@
> +    callback = function () whenFileExists(aURL, aCallback);
> +  }
> +
> +  executeSoon(callback || next);
> +}

Let me guess: at some point, you will want support for inotifyd et al.

Comment 71

[Martin Stránský [:stransky] (ni? me)]

Attachment: v4 for check-in

Wow, thanks for the OS.File patch! There's the v4 patch for check-in, without the PageThumbs.jsm part.

Comment 72

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #70)
> > +        let from = OS.Path.join(msg.from, entry.name);
> > +        let to = OS.Path.join(msg.to, entry.name);
> > +
> > +        try {
> > +          OS.File.move(from, to, {noOverwrite: true, noCopy: true});
> 
> Ok, so this won't move files from a device to another one, is that intended?

Yes, we want to move files if we can. The costs for copying are too high and that should really only affect a minority.

> > +  PageThumbsStorageMigrator.migrateToVersion3();
> > +  ok(true, "migration finished");
> > +
> > +  // Wait until the first thumbnail was moved to its new location.
> > +  yield whenFileExists(URL);
> 
> In real usage scenarios, this will generally not happen because ProfD and
> ProfLD are on distinct devices. Here, you place them on the same device.
> That's probably ok, but don't forget to document this somewhere.

I think that most of the people have ~/.cache/ on the same file system as ~/.mozilla/ so the general case is that the thumbnails are moved. Emulating the case where both of those are on different file systems is a little too hard with mochitests and we don't care anyway because they're just going to be removed.

> > +  yield whenFileExists(URL2);
> > +  ok(true, "second thumbnail moved");
> > +
> > +  yield whenFileRemoved(roaming);
> > +  ok(true, "roaming thumbnail directory removed");
> 
> You haven't checked that files are never overwritten. That's probably not
> very important in this case, though.

Not too important but I think I can add a check for this, too.

> > +    callback = function () whenFileExists(aURL, aCallback);
> > +  }
> > +
> > +  executeSoon(callback || next);
> > +}
> 
> Let me guess: at some point, you will want support for inotifyd et al.

*smirk*

Comment 73

[Tim Taubert [:ttaubert] (inactive)]

Pushed both patches to try:

https://tbpl.mozilla.org/?tree=Try&rev=fcba62ef6b1a

Comment 74

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/86b8a6ff664e
https://hg.mozilla.org/integration/fx-team/rev/13c6407debdb

Comment 75

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/86b8a6ff664e
https://hg.mozilla.org/mozilla-central/rev/13c6407debdb

Comment 76

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 712379 [details] [diff] [review]
v4 for check-in

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

::: toolkit/xre/nsXREDirProvider.h
@@ +114,5 @@
>    static nsresult AppendProfilePath(nsIFile* aFile,
>                                      const nsACString* aProfileName,
>                                      const nsACString* aAppName,
> +                                    const nsACString* aVendorName,
> +                                    PRBool aLocal);

So, uh, I know that this is an old bug, but PRBools have been gone for quite a while...

Comment 77

[Martin Stránský [:stransky] (ni? me)]

Attachment: PRBool remove

PRBool -> bool replacement. Asking Michal for r+ because he did the original patch review but anyone feels free to give the r+ :)

Comment 78

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/299b1aa354da

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/299b1aa354da

Comment 80

[Jonathan Kew [:jfkthame]]

This has apparently broken the feature (for Firefox on Android) of being able to use fonts installed in a /fonts/ directory in the user's profile; see bug 842262.

Comment 81

[Jonathan Kew [:jfkthame]]

The reason for that breakage appears to be confusion between the "profile" (ProfD) and "profile local" (ProfLD) directories, so it should be fixable by updating the addon concerned.

However, I just wanted to confirm whether the change in behavior on Android was intended/expected. By adding logging in a local build, I confirmed that prior to this bug, ProfD and ProfLD resolved to the same directory:

I/Gecko   (17221): profile dir:       [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
I/Gecko   (17221): profile-local dir: [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]

but after this landing, ProfLD now resolves to a directory under files/.cache:

I/Gecko   (17329): profile dir:       [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
I/Gecko   (17329): profile-local dir: [/data/data/org.mozilla.fennec_jkew/files/.cache/mozilla/syhz4rjf.default]

Was this the intended result here?

Comment 82

[Mike Hommey [:glandium]]

(In reply to Jonathan Kew (:jfkthame) from comment #81)
> The reason for that breakage appears to be confusion between the "profile"
> (ProfD) and "profile local" (ProfLD) directories, so it should be fixable by
> updating the addon concerned.
> 
> However, I just wanted to confirm whether the change in behavior on Android
> was intended/expected. By adding logging in a local build, I confirmed that
> prior to this bug, ProfD and ProfLD resolved to the same directory:
> 
> I/Gecko   (17221): profile dir:      
> [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
> I/Gecko   (17221): profile-local dir:
> [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
> 
> but after this landing, ProfLD now resolves to a directory under
> files/.cache:
> 
> I/Gecko   (17329): profile dir:      
> [/data/data/org.mozilla.fennec_jkew/files/mozilla/syhz4rjf.default]
> I/Gecko   (17329): profile-local dir:
> [/data/data/org.mozilla.fennec_jkew/files/.cache/mozilla/syhz4rjf.default]
> 
> Was this the intended result here?

Certainly not.

Comment 83

[Gian-Carlo Pascutto [:gcp]]

This is going to make everything that was storing stuff in ProfLD to lose it's data, isn't it? So it will make existing SafeBrowsing databases invisible to the app, for example. Most of the other stuff is caches, so not the end of the world, but unless we move things it's going to end up taking storage on people's devices.

IIRC one of the cache dirs allows Android to drop data if storage is getting full, though I'm not sure that was ".cache" or not?

Comment 84

[Brad Lassey [:blassey] (use needinfo?)]

The directory that Android will purge if needed is retrieved by getCacheDir() [http://developer.android.com/reference/android/content/Context.html#getCacheDir%28%29]. We store that directory in the CACHE_DIRECTORY env var [http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/mozglue/GeckoLoader.java.in#125]