406869 - [FIX]SetCacheAsFile(PR_TRUE) on memory-cached loads is broken (doesn't throw)

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2pre) Gecko/2007120401 Minefield/3.0b2pre

Steps to reproduce:
1. Load part of the testcase from bug 406740:
     jar:https://bugzilla.mozilla.org/attachment.cgi?id=291410!/diagnostic.html 
2. View Source

Result: the View Source window appears, but it's empty.

"Save As" seems to be broken too.

Regression from bug 369814?

Comment 1

[Jesse Ruderman]

Oops, the testcase is from bug 406744, not bug 406740.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase that doesn't run evil Java

Comment 3

[Boris Zbarsky [:bzbarsky]]

This regressed between 2007-11-30-02 and 2007-12-01-02.

That corresponds to the checkins for bug 262116 and bug 345181.

What happens is that nsDownloader::OnStopRequest QIs the channel (which is an HTTP channel) to nsICachingChannel.  This succeeds.  Then it calls GetCacheFile(), which fails with NS_ERROR_NOT_IMPLEMENTED because the channel is using the memory cache device.  This error status is then propagated to OnDownloadComplete, etc.

Comment 4

[Boris Zbarsky [:bzbarsky]]

So the problem is that the HTTP channel is using the memory cache device but does NOT have the INHIBIT_PERSISTENT_CACHING load flag set.  Which means that SetCacheAsFile() returns success (incorrectly) and nsDownloader::OnStartRequest doesn't set up a temp file (which in this case it should).

Looks like a regression from bug 345181.

Comment 5

[Boris Zbarsky [:bzbarsky]]

nsHttpChannel::InitCacheEntry is never called in this case, since we never _write_ to cache.  All we do is read from it.

Comment 6

[Mike Schroepfer]

Bz do we need to fix this for b2 you think?

Comment 7

[Boris Zbarsky [:bzbarsky]]

The users of SetCacheAsFile() in our code are nsDownloader and the plugin host.  The former is only an issue for from-cache loads of things like jar:.  The latter will be a problem for from-cache loads over ssl of all sorts of plug-in stuff.  I'm just not sure how much this will bite in practice.

I don't think this should be a particularly hard fix, in any case, so I think we should definitely try to fix it for b2...

Comment 8

[Boris Zbarsky [:bzbarsky]]

So I see two main options:

1)  In OpenCacheEntry() (or the async callback, as relevant), set the
    INHIBIT_PERSISTENT_CACHING flag if the cache entry storage policy is
    STORE_IN_MEMORY
2)  Make SetStoragePolicy(STORE_ON_DISK_AS_FILE) fail if the current policy is
    STORE_IN_MEMORY or the cache entry is opened readonly (or both).

In either case, we should fix GetStoragePolicy() (which has never worked; luckily no one uses it).

I sort of prefer #2, I think.  Christian, what do you think?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

I prefer 2) as well. This was broken for no-store HTTP loads too, right?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Let's do it

Yeah, any time we cache in memory this would be broken.  I guess I should write a testcase with a no-store header or something, eh?  Sounds painful, since I have to guarantee a cache hit... :(

Comment 11

[Mike Schroepfer]

We wanted this for b2 right bz?

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in as requested for b2!

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yeah, we wanted this.  I just didn't have a chance to watch the tree until tonight.

vlad, thanks for checking this in!