Closed
Bug 291033
Opened 19 years ago
Closed 19 years ago
Enable support for profile temp directory on local filesystem
Categories
(Toolkit :: Startup and Profile System, defect, P1)
Toolkit
Startup and Profile System
Tracking
()
RESOLVED
FIXED
mozilla1.8beta2
People
(Reporter: darin.moz, Assigned: darin.moz)
References
(Depends on 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
78.77 KB,
patch
|
benjamin
:
first-review+
chofmann
:
approval-aviary1.1a1+
chofmann
:
approval1.8b2+
|
Details | Diff | Splinter Review |
22.36 KB,
patch
|
benjamin
:
first-review+
darin.moz
:
second-review+
asa
:
approval1.8b2+
|
Details | Diff | Splinter Review |
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.)
Assignee | ||
Updated•19 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Assignee | ||
Comment 1•19 years ago
|
||
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.
Attachment #181246 -
Flags: first-review?(benjamin)
Assignee | ||
Comment 2•19 years ago
|
||
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 3•19 years ago
|
||
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 4•19 years ago
|
||
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...
Comment 5•19 years ago
|
||
I also agree that we should refer to "local" instead of "temp".
Comment 6•19 years ago
|
||
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...
Assignee | ||
Comment 7•19 years ago
|
||
> 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?
Assignee | ||
Comment 8•19 years ago
|
||
> 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.
Assignee | ||
Comment 9•19 years ago
|
||
with "local" instead of "temp" :)
Attachment #181246 -
Attachment is obsolete: true
Attachment #181386 -
Flags: first-review?(benjamin)
Comment 10•19 years ago
|
||
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...
Assignee | ||
Comment 11•19 years ago
|
||
> 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.
Updated•19 years ago
|
Attachment #181386 -
Flags: first-review?(benjamin) → first-review+
Comment 12•19 years ago
|
||
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.
Attachment #181633 -
Flags: second-review?(darin)
Attachment #181633 -
Flags: first-review?(benjamin)
Assignee | ||
Comment 13•19 years ago
|
||
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.
Attachment #181633 -
Flags: second-review?(darin) → second-review+
Comment 14•19 years ago
|
||
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?
Attachment #181633 -
Flags: first-review?(benjamin) → first-review+
Assignee | ||
Updated•19 years ago
|
Attachment #181386 -
Flags: approval1.8b2?
Attachment #181386 -
Flags: approval-aviary1.1a?
Comment 15•19 years ago
|
||
Comment on attachment 181386 [details] [diff] [review] v1.1 patch a=chofmann
Assignee | ||
Comment 16•19 years ago
|
||
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.
Updated•19 years ago
|
Attachment #181386 -
Flags: approval1.8b2?
Attachment #181386 -
Flags: approval1.8b2+
Attachment #181386 -
Flags: approval-aviary1.1a?
Attachment #181386 -
Flags: approval-aviary1.1a+
Assignee | ||
Comment 17•19 years ago
|
||
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.
Assignee | ||
Comment 18•19 years ago
|
||
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 19•19 years ago
|
||
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)
Attachment #181633 -
Flags: approval1.8b2?
Comment 20•19 years ago
|
||
(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.
Assignee | ||
Comment 21•19 years ago
|
||
> 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 22•19 years ago
|
||
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?
Comment 23•19 years ago
|
||
we care nowhere else either...
Comment 24•19 years ago
|
||
Comment on attachment 181633 [details] [diff] [review] xpfe part a=asa
Attachment #181633 -
Flags: approval1.8b2? → approval1.8b2+
Comment 25•19 years ago
|
||
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.
Comment 26•19 years ago
|
||
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Attachment #181246 -
Flags: first-review?(benjamin)
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
Comment 27•12 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•