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.

> 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?

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.

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

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

> 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

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/)

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)

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

(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.

@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 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?

(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).

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.

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 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...

(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.

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 ;-).

And I'll look into it tomorrow.

(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 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.

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

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.

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 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.

(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?

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).

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

Comment on attachment 580615 [details] [diff] [review] revised patch I assume that you want to request a new review

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 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 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.

(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)

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.

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 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.

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 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.

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.

>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 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.

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

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

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.

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.

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.

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

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?

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

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 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.

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.

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

Comment on attachment 697043 [details] [diff] [review] v4 Try looks fine.

Comment on attachment 697043 [details] [diff] [review] v4 Looks good. The change in PageThumbs.jsm needs a review from browser peer.

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!

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

(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?

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 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.

(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.

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.

(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.

(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.

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.

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.

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

(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 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.

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

(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*

Pushed both patches to try: https://tbpl.mozilla.org/?tree=Try&rev=fcba62ef6b1a

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

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

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...

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

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.

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?

(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.

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?

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]