Last Comment Bug 291033 - Enable support for profile temp directory on local filesystem
: Enable support for profile temp directory on local filesystem
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Startup and Profile System (show other bugs)
: unspecified
: All All
: P1 normal with 2 votes (vote)
: mozilla1.8beta2
Assigned To: Darin Fisher
:
: Benjamin Smedberg [:bsmedberg]
Mentors:
Depends on: 373512
Blocks: 74085 216204 239254 290399 299542
  Show dependency treegraph
 
Reported: 2005-04-19 14:54 PDT by Darin Fisher
Modified: 2011-12-20 08:37 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 patch (78.17 KB, patch)
2005-04-19 23:21 PDT, Darin Fisher
no flags Details | Diff | Splinter Review
v1.1 patch (78.77 KB, patch)
2005-04-20 22:38 PDT, Darin Fisher
benjamin: first‑review+
chofmann: approval‑aviary1.1a1+
chofmann: approval1.8b2+
Details | Diff | Splinter Review
xpfe part (22.36 KB, patch)
2005-04-23 10:08 PDT, Christian :Biesinger (don't email me, ping me on IRC)
benjamin: first‑review+
darin.moz: second‑review+
asa: approval1.8b2+
Details | Diff | Splinter Review

Description Darin Fisher 2005-04-19 14:54:41 PDT
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.)
Comment 1 Darin Fisher 2005-04-19 23:21:44 PDT
Created attachment 181246 [details] [diff] [review]
v1 patch

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.
Comment 2 Darin Fisher 2005-04-19 23:25:44 PDT
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 Benjamin Smedberg [:bsmedberg] 2005-04-20 05:13:56 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-20 06:17:05 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-20 06:18:52 PDT
I also agree that we should refer to "local" instead of "temp".
Comment 6 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-20 11:01:01 PDT
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...
Comment 7 Darin Fisher 2005-04-20 19:16:10 PDT
> 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?
Comment 8 Darin Fisher 2005-04-20 19:19:37 PDT
> 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.
Comment 9 Darin Fisher 2005-04-20 22:38:58 PDT
Created attachment 181386 [details] [diff] [review]
v1.1 patch

with "local" instead of "temp" :)
Comment 10 Benjamin Smedberg [:bsmedberg] 2005-04-21 12:48:43 PDT
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...
Comment 11 Darin Fisher 2005-04-21 14:52:29 PDT
> 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.
Comment 12 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-23 10:08:51 PDT
Created attachment 181633 [details] [diff] [review]
xpfe part

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 13 Darin Fisher 2005-04-23 11:45:30 PDT
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 14 Benjamin Smedberg [:bsmedberg] 2005-04-24 08:32:17 PDT
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 15 chris hofmann 2005-04-25 17:07:02 PDT
Comment on attachment 181386 [details] [diff] [review]
v1.1 patch

a=chofmann
Comment 16 Darin Fisher 2005-04-25 17:08:35 PDT
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.
Comment 17 Darin Fisher 2005-04-25 17:38:42 PDT
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.
Comment 18 Darin Fisher 2005-04-25 18:58:12 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-26 05:14:47 PDT
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)
Comment 20 Olivier Mengué 2005-04-26 08:17:07 PDT
(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.
Comment 21 Darin Fisher 2005-04-26 09:07:08 PDT
> 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 Justin Wood (:Callek) 2005-04-26 13:52:50 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-26 14:06:12 PDT
we care nowhere else either...
Comment 24 Asa Dotzler [:asa] 2005-04-26 14:06:51 PDT
Comment on attachment 181633 [details] [diff] [review]
xpfe part

a=asa
Comment 25 neil@parkwaycc.co.uk 2005-04-26 17:05:25 PDT
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 Christian :Biesinger (don't email me, ping me on IRC) 2005-04-27 04:59:39 PDT
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.
Comment 27 Martin Oberhuber 2011-12-20 08:37:07 PST
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!

Note You need to log in before you can comment on or make changes to this bug.