Enable support for profile temp directory on local filesystem On Windows, we want to be able to store the network cache and XUL fastload cache on the local filesystem. This means storing data under the user's Local Settings directory. The same goes for Mac OSX (~/Library/Caches). My solution: o) Create a temp directory corresponding to each profile directory that can be used to store various caches and other volatile data files. o) Only create a lock in the main profile directory. o) Make the network cache and XUL fastload service cleanup any files leftover in the old location automatically. Patch coming up... (I'm filing this bug here because I want to avoid the large CC lists on the other bugs. I'll mark them up appropriately once this one is resolved.)

This is still somewhat of a rough cut. I've tested it with Firefox and XULRunner on Windows with a mix of existing and non-existing profiles, and it seems to work properly. I'm posting it here for early feedback.

A few small bug fixes included in this patch: 1) It makes -profile work even if the specified directory does not exist yet. 2) The disk cache deletion code now deletes Cache.Trash as well as the contents of that folder. biesi: if you have time to look over this patch and give feedback, it'd be appreciated. thanks!

Comment on attachment 181246 [details] [diff] [review] v1 patch Can we change "temp" to "local" throughout? Temp implies that the files get deleted, which is obviously incorrect. At nsIProfileLock.unlock, the comment is misleading: since we don't lock the "tempDirectory", we're not really unlocking it either. Maybe I missed it, but how does the profile point to the profilelocaldir? toolkitprofileservice::createprofile doesn't seem to have the code I expected for that. I don't think you want separate nsXREDirProvider.SetProfileDir/SetProfileTempDir, just use one func and pass null if there is no local dir. I might want to think about using a localdir from the profile manager UI as well, but that can be a separate patch.

Comment on attachment 181246 [details] [diff] [review] v1 patch why not move the cache directory to the temp directory, instead of just deleting it? that would avoid losing the cache...

I also agree that we should refer to "local" instead of "temp".

Comment on attachment 181246 [details] [diff] [review] v1 patch netwerk/cache/src/nsCacheService.cpp + if (NS_SUCCEEDED(profDir->Equals(directory, &same)) && !same) { + // We're migrating from storing the cache directory in the + // profile main directory to storing it in the profile temp + // directory. We should doom the old one. So... At the location of this comment we are not actually sure that we are migrating a directory, are we? The check whether the directory exists in the profile dir is only some lines later...

> Can we change "temp" to "local" throughout? Temp implies that the files get > deleted, which is obviously incorrect. I used the term "temp" because this directory could get deleted, and it should have non-fatal consequences. Backup schemes should probably exclude this directory. Developers should think of this directory in that way. But, perhaps "temp" is too strong a term for this directory. That does imply some frequency to the data loss. Perhaps that is not what I mean. I considered "local" but decided on "temp" for the reasons I just enumerated. I'm not tied to "temp" though so I will consider "local" as an alternative. > At nsIProfileLock.unlock, the comment is misleading: since we don't lock the > "tempDirectory", we're not really unlocking it either. Ok, that makes sense. > Maybe I missed it, but how does the profile point to the profilelocaldir? > toolkitprofileservice::createprofile doesn't seem to have the code I expected > for that. Well, the profilelocaldir is only applicable when the entry in profiles.ini has isRelative set to true. Then, I simply use the given profile path relative to the Local Settings base path to locate the profilelocaldir. > I don't think you want separate > nsXREDirProvider.SetProfileDir/SetProfileTempDir, just use one func and pass > null if there is no local dir. There is always a local dir. It is either a separate dir, or it is the same as the profile dir. I like your idea of combining the functions. > I might want to think about using a localdir from the profile manager UI as > well, but that can be a separate patch. What did you have in mind exactly? You want to allow the user to select a local dir at profile creation time? I think that's a pretty advanced use case. Is it really worth it?

> why not move the cache directory to the temp directory, instead of just > deleting it? that would avoid losing the cache... I can only do that if the two locations are on the same filesystem. If they are not, then copying the files would be prohibitively costly. If I have time, I would like to implement "is same filesystem" checking, and then just move the directory as you suggest. > netwerk/cache/src/nsCacheService.cpp > + if (NS_SUCCEEDED(profDir->Equals(directory, &same)) && !same) > { > + // We're migrating from storing the cache directory in the > + // profile main directory to storing it in the profile > temp > + // directory. We should doom the old one. > > So... At the location of this comment we are not actually sure that we are > migrating a directory, are we? The check whether the directory exists in the > profile dir is only some lines later... Sure, the comment is somewhat incorrect. This code is trying to delete the cache if it is found in the old location. The current code does this even if the new cache already exists. The use case where that might matter is if someone were switching back-n-forth between Firefox 1.0 and 1.1. I'm not sure that we care about that use case so much. What I think is important is that we do not leave around a 50M unused database on the user's system.

with "local" instead of "temp" :)

Comment on attachment 181386 [details] [diff] [review] v1.1 patch OK, I was confused about a few things. I thought that the "local path" was contained in a pref or a file in the profile, not linked directly from profiles.ini. Now I'm trying to decide if "what I thought" is better, or what you coded. But you coded it this way, and unless I can come up with a good reason to disagree, let's go with your way... The only real question I have left is, what is the moveToTrash arg for? I don't see it being used anywhere...

> OK, I was confused about a few things. I thought that the "local path" was > contained in a pref or a file in the profile, not linked directly from > profiles.ini. Now I'm trying to decide if "what I thought" is better, or what > you coded. > > But you coded it this way, and unless I can come up with a good reason to > disagree, let's go with your way... I didn't really want to make the system (nsXREDirProvider and nsToolkitProfileService) have to know anything about the pref system. > The only real question I have left is, what is the moveToTrash arg for? I don't > see it being used anywhere... Look more carefully :-) In some cases I pass true and in others false. In some cases we want to move the directory to the trash folder before we delete it because we are going to reuse the existing location, but in the case of deleting the Firefox 1.0 cache, we can just delete it in place.

possible issues: - does not salt the local dir - is that a problem? - does not store the local dir in the registry, always uses the one under local settings. not sure if this is a problem.

Comment on attachment 181633 [details] [diff] [review] xpfe part What if the user configures his profile directory to be somewhere specific? The toolkit patch reverts to storing these files in the user specified directory in that case. I think the profile salting is probably important but less so for the cache.

Comment on attachment 181633 [details] [diff] [review] xpfe part This isn't going to hurt us if we decide to salt the localdir in the future, is it?

Comment on attachment 181386 [details] [diff] [review] v1.1 patch a=chofmann

I think the xpfe changes are just intermediate changes anyways. If the seamonkey people are planning on moving seamonkey to the new toolkit, then there is going to be profile migration issues anyways. I personally don't think it is worth the effort to port this patch to xpfe, but biesi has already done it.

v1.1 patch landed on trunk for firefox 1.1a. biesi: i did not land your patch. not sure if we should leave this open for that patch or what.

It turns out that VC6 does not define CSIDL_LOCAL_APPDATA. That's interesting, as it may indicate that on some systems that location may not be defined. In that case, we probably need to add some failover logic to nsXREDirProvider. I filed bug 291882 for this possible problem.

Comment on attachment 181633 [details] [diff] [review] xpfe part this patch only affects seamonkey and allows storing the cache in a local directory. Since this is only intermediate, I think the issues from comment 13 / comment 14 aren't so important for now - they'll become non-issues once seamonkey moves to toolkit (although that probably won't happen for the first release)

(In reply to comment #18) > It turns out that VC6 does not define CSIDL_LOCAL_APPDATA. That's interesting, > as it may indicate that on some systems that location may not be defined. In > that case, we probably need to add some failover logic to nsXREDirProvider. I > filed bug 291882 for this possible problem. Local AppData is defined since Windows 2000. It also may be available on older systems (such as Windows NT 4/IE 6 on which I'm writing this) if IE has been upgraded. See http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/enums/csidl.asp Note that CSIDL_INTERNET_CACHE exists and may be the best place to put a sub-directory for an Internet cache.

> Local AppData is defined since Windows 2000. It also may be available on older > systems (such as Windows NT 4/IE 6 on which I'm writing this) if IE has been > upgraded. What about Win98? http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/reference/enums/csidl.asp Yeah, this page seriously lacks documentation on which versions of the OS support which flags. > Note that CSIDL_INTERNET_CACHE exists and may be the best place to put a > sub-directory for an Internet cache. If we were talking about just the internet cache, then I would agree. But, we need a place to store other caches as well. bug 291882 is about implementing a fallback strategy when CSIDL_LOCAL_APPDATA is not defined on the OS.

Comment on attachment 181633 [details] [diff] [review] xpfe part >Index: xpcom/io/nsAppFileLocationProvider.cpp >=================================================================== >RCS file: /cvsroot/mozilla/xpcom/io/nsAppFileLocationProvider.cpp,v >@@ -327,15 +331,15 @@ NS_METHOD nsAppFileLocationProvider::Clo >-NS_METHOD nsAppFileLocationProvider::GetProductDirectory(nsILocalFile **aLocalFile) >+NS_METHOD nsAppFileLocationProvider::GetProductDirectory(nsILocalFile **aLocalFile, PRBool aLocal) Won't this warn on some OS's with certain compilers about aLocal not being used?

we care nowhere else either...

Comment on attachment 181633 [details] [diff] [review] xpfe part a=asa

Comment on attachment 181633 [details] [diff] [review] xpfe part biesi, don't forgot darin's nsXREDirProvider.cpp bustage fix. Salting the directory would be nice as a precaution.

xpfe part checked in, with the bustage fix While I'd like to implement salting for xpfe too, I'm not sure that I'll have time for that... marking bug fixed. filed Bug 292075 for salting.

I verified on my machine that the fix works with Firefox-3.6 but it looks like an embedded xulrunner (from the very same Firefox) does not have the fix. Why is that ? Please comment on Eclipse https://bugs.eclipse.org/bugs/show_bug.cgi?id=367216 with any suggestions, many thanks!