922081 - HTTP cache v2: respect "browser.cache.disk.parent_directory" preference

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[fxtech]

thanks a lot for takin care about it

Comment 2

[Grant]

Hi there, any news on this? Thanks

Comment 3

[Honza Bambas (:mayhemer)]

This will be fixed when the new cache will be enabled.  So far, feel free to provide a patch your self.  I don't plan working on this as a top priority right now.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 5

[Michal Novotny [:michal]]

Comment on attachment 8373478 [details] [diff] [review]
v1

Review of attachment 8373478 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +929,5 @@
>    nsresult rv;
>  
>    nsCOMPtr<nsIFile> directory;
>  
> +  CacheObserver::ParentDirOverride(getter_AddRefs(directory));

I think CacheObserver::ParentDirOverride() should either return a clone or we need to clone it here since we append a leaf to this nsIFile a few lines below.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #5)
> Comment on attachment 8373478 [details] [diff] [review]
> v1
> 
> Review of attachment 8373478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +929,5 @@
> >    nsresult rv;
> >  
> >    nsCOMPtr<nsIFile> directory;
> >  
> > +  CacheObserver::ParentDirOverride(getter_AddRefs(directory));
> 
> I think CacheObserver::ParentDirOverride() should either return a clone or
> we need to clone it here since we append a leaf to this nsIFile a few lines
> below.

But we clone before adding the leaf.

Comment 7

[Honza Bambas (:mayhemer)]

To make it more clear:

914 nsresult
915 CacheFileIOManager::OnProfile()
916 {
917   LOG(("CacheFileIOManager::OnProfile() [gInstance=%p]", gInstance));
918 
919   MOZ_ASSERT(gInstance);
920 
921   nsresult rv;
922 
923   nsCOMPtr<nsIFile> directory;
924 

...

942 
943   if (directory) {

Here is the clone:

944     rv = directory->Clone(getter_AddRefs(gInstance->mCacheDirectory));
945     NS_ENSURE_SUCCESS(rv, rv);
946 
947     rv = gInstance->mCacheDirectory->Append(NS_LITERAL_STRING("cache2"));
948     NS_ENSURE_SUCCESS(rv, rv);
949   }
950 
951   return NS_OK;
952 }

Comment 8

[Michal Novotny [:michal]]

Right, I've looked into my tree where I have CacheIndex patch applied and it exchanges Clone() and Append(). The current code is wrong since for a very short time we have a wrong path (without the leaf) in mCacheDirectory between Append() and Clone(). Anyway, NS_GetSpecialDirectory() returns either a new file or a cloned file if it is cached, i.e. always returns a file that the caller can modify as needed. CacheObserver::ParentDirOverride() should do the same to be consistent. And call to Clone() should be removed from CacheFileIOManager::OnProfile() since it is not necessary.

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v2

- clones inside CacheObserver::ParentDirOverride
- clone at OnProfile removed

Comment 10

[Honza Bambas (:mayhemer)]

Michal, ping on review.  It's actually quite unfortunate this didn't make it for the trial regardless how simple the patch is, people complain!

Comment 11

[Dennis McCunney]

Add me as a user interested in seeing this.  Under Windows, I use browser.cache.disk.parent_directory to put the cache on a ramdisk.  (I have a fast broadband connection, and am not concerned about preserving cache between boots.)  Under Linux, I put it in POSIX shared memory in /dev/shm.  I want to be able to do the same for cache2.

Comment 12

[Honza Bambas (:mayhemer)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2275f8be60c9

Comment 13

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c486ad9a89ac

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c486ad9a89ac