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)

defect

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)

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.)
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Attached patch v1 patch (obsolete) — Splinter Review
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)
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.
Attached patch v1.1 patchSplinter Review
with "local" instead of "temp" :)
Attachment #181246 - Attachment is obsolete: true
Attachment #181386 - Flags: first-review?(benjamin)
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.
Attachment #181386 - Flags: first-review?(benjamin) → first-review+
Attached patch xpfe partSplinter Review
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)
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 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+
Attachment #181386 - Flags: approval1.8b2?
Attachment #181386 - Flags: approval-aviary1.1a?
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.
Attachment #181386 - Flags: approval1.8b2?
Attachment #181386 - Flags: approval1.8b2+
Attachment #181386 - Flags: approval-aviary1.1a?
Attachment #181386 - Flags: approval-aviary1.1a+
Blocks: 290399
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.
Blocks: 74085
Blocks: 216204
Blocks: 239254
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)
Attachment #181633 - Flags: approval1.8b2?
(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
Attachment #181633 - Flags: approval1.8b2? → approval1.8b2+
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Attachment #181246 - Flags: first-review?(benjamin)
Blocks: 299542
Depends on: 373512
Component: XRE Startup → Startup and Profile System
QA Contact: nobody → startup
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.