723100 - Request for caching HTTP content as a file may be silently ignored

Status: RESOLVED WONTFIX

(Core :: Networking: HTTP, defect)

Description

[Michal Novotny [:michal]]

+++ This bug was initially created as a clone of Bug #712032 +++

Bug #712032 was marked as fixed, but actually the problem isn't fixed. We just disabled cache compression which suppressed the oranges.

Comment 1

[Michal Novotny [:michal]]

Attachment: fix

This patch fixes the issue that we return file for cache entry that actually can't be used as a file because the data is encoded, compressed or the file is missing.

Anyway, I don't understand how we could end up having loremipsum_file.txt cached as compressed entry, because only 2 tests use this file (test_pluginstream_asfile.html and test_pluginstream_asfileonly.html) and in both cases SetCacheAsFile() should be called. I don't think that nsHttpChannel::SetCacheAsFile() could fail in these tests, so maybe something went wrong in nsPluginStreamListenerPeer::SetUpStreamListener() and SetCacheAsFile() wasn't called at all?

Comment 2

[Masatoshi Kimura [:emk]]

If the compressed resource is already stored in the cache, plug-ins cannot request the resource as an uncompressed file? It may break some plug-ins which want the resource as a file.
Is it impossible to doom the existing entry if SetCacheAsFile is called and the exisintg entry is compressed or encoded?

Comment 3

[Michal Novotny [:michal]]

(In reply to Masatoshi Kimura [:emk] from comment #2)
> If the compressed resource is already stored in the cache, plug-ins cannot
> request the resource as an uncompressed file? It may break some plug-ins
> which want the resource as a file.

They can. There is a backup solution on every place where SetCacheAsFile() is called. It might fail for whatever reason, e.g. the disk cache can be disabled...


> Is it impossible to doom the existing entry if SetCacheAsFile is called and
> the exisintg entry is compressed or encoded?

This won't work since SetCacheAsFile() is called too late to open a new entry after dooming the old one.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 593434 [details] [diff] [review]
fix

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

The changes seem reasonable to me. jduell would be a better reviewer than me though. Nick should also review this to make sure it fits with his understanding of how things should work.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4807,5 @@
> +        nsXPIDLCString buf;
> +        rv = mCacheEntry->GetMetaDataElement("uncompressed-len",
> +                                             getter_Copies(buf));
> +        if (NS_SUCCEEDED(rv))
> +            return NS_ERROR_NOT_AVAILABLE;

This logic duplicates the logic above; the duplicated logic should be factored out into a separate function.

::: netwerk/test/unit/test_bug712032.js
@@ +1,1 @@
> +do_load_httpd_js();

Include "cacheAsFile" in the name of this test file.

@@ +7,5 @@
> +    if (request_nr == 2) {
> +      try {
> +        var cachingChannel = request.QueryInterface(Ci.nsICachingChannel);
> +        cachingChannel.cacheAsFile = true;
> +        do_throw("cacheAsFile should fail");

cacheAsFile could fail for multiple reasons. It seems like only one reason is tested for (not stored as a file?). we should test the other reasons (compression, content-encoding).

Also, the changes to GetCacheFile are not tested.

Comment 5

[Jason Duell]

Comment on attachment 593434 [details] [diff] [review]
fix

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

I don't feel like I know the cache guts here well enough to properly review.  I suggest we deputize Nick to review Michal's cache patches.

I did look over it--see my (speculative and probably unhelpful :) comments below.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4742,5 @@
> +            if (NS_SUCCEEDED(rv))
> +                return NS_ERROR_NOT_AVAILABLE;
> +
> +            nsCOMPtr<nsIFile> file;
> +            rv = mCacheEntry->GetFile(getter_AddRefs(file));

If I follow this correctly, GetFile checks to see if data has been written yet, and tells the entry to use a file if so (or it fails if data is already written into a block file).  nsICachingChannel says this should only be called during OnStartRequest, so I assume this is fine.

I find it odd that we need to call GetFile--it seems like SetStoragePolicy ought to be doing this check for us?  I.e. it's strange that SetStoragePolicy just sets the STORE_ON_DISK_AS_FILE flag, without checking whether or not we're actually going to be able to use a separate file--it would be nice if it failed if we can't use a separate file.  Should we move the GetFile check into SetStoragePolicy?  That would seem cleaner.  But I'm not familiar enough with the cache to know if that makes sense.

I see GetFile holds the cache lock, and so does SetStoragePolicy.  I didn't look carefully enough to know if there's a way to make either of those functions not hold the lock, or if there's a way to determine success/failure w/o holding the lock and return, then update any locked data structures later on via an event on the cache thread.

@@ +4807,5 @@
> +        nsXPIDLCString buf;
> +        rv = mCacheEntry->GetMetaDataElement("uncompressed-len",
> +                                             getter_Copies(buf));
> +        if (NS_SUCCEEDED(rv))
> +            return NS_ERROR_NOT_AVAILABLE;

Yeah, call it something like "ResponseCanBeCachedAsFile".

@@ +4814,1 @@
>      return mCacheEntry->GetFile(cacheFile);

Once you've called GetFile successfully, the entry will be stored as a file, right? Do we want to set the storagePolicy to STORE_ON_DISK_AS_FILE in that case, or do we need to keep as whatever it was? It would be more intuitive to me, but perhaps there's a reason why it needs to remain unchanged.

Comment 6

[Jason Duell]

> >  the duplicated logic should be factored out into a separate function.
> 
> Yeah, call it something like "ResponseCanBeCachedAsFile".

(splinter does a crappy job of showing a comment that you're commenting on)

Comment 7

[Alex Keybl [:akeybl]]

After discussing with bsmedberg, we don't believe this is a regression specific to FF11 and is likely an uncommon user scenario. Untracking for FF11.

Comment 8

[u408661]

Comment on attachment 593434 [details] [diff] [review]
fix

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

Looks pretty good, just a couple things that Brian and Jason already pointed out in their reviews. Next round should do it for this one.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4807,5 @@
> +        nsXPIDLCString buf;
> +        rv = mCacheEntry->GetMetaDataElement("uncompressed-len",
> +                                             getter_Copies(buf));
> +        if (NS_SUCCEEDED(rv))
> +            return NS_ERROR_NOT_AVAILABLE;

I agree with Brian and Jason here, let's factor this logic out.

::: netwerk/test/unit/test_bug712032.js
@@ +7,5 @@
> +    if (request_nr == 2) {
> +      try {
> +        var cachingChannel = request.QueryInterface(Ci.nsICachingChannel);
> +        cachingChannel.cacheAsFile = true;
> +        do_throw("cacheAsFile should fail");

As Brian said, let's test all the possible failure reasons for both cacheAsFile and cacheFile, assuming it's possible (I don't see offhand why it wouldn't be).

Comment 10

[Patrick McManus [:mcmanus]]

seems like this is a patch that was almost complete. is this still desirable? Should it be wontfixed?

Comment 11

[Michal Novotny [:michal]]

This functionality was removed completely in bug 725993.