232385 - IRequest::INHIBIT_CACHING not implemented

Status: RESOLVED FIXED

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

Description

[William Bardwell]

User-Agent:       
Build Identifier: 1.5

IRequest has a load flag defined and documented as
inhibiting caching called INHIBIT_CACHING, but it doesn't
actually do anything, anywhere.  It would be very handy, for
me at least if it worked for http.  It looks like all that needs
to be done is to open the cache entry in read-only mode when
that flag is set.  (But there might be more required to fix it...)

004-01-27 23:41:06.000000000 -0500
@@ -1113,7 +1113,7 @@
 
     // Set the desired cache access mode accordingly...
     nsCacheAccessMode accessRequested;
-    if (offline)
+    if (offline || mLoadFlags & INHIBIT_CACHING)
         accessRequested = nsICache::ACCESS_READ; // have no way of writing to cache
     else if (mLoadFlags & LOAD_BYPASS_CACHE)
         accessRequested = nsICache::ACCESS_WRITE; // replace cache entry


Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Darin Fisher]

good catch.  we actually have never made use of the INHIBIT_CACHING flag in
Mozilla, so i guess that explains why it was never implemented :-(

your patch looks correct to me.

Comment 2

[William Bardwell]

Actually, to let reload/resynchronize stuff still work, I think that the
complete patch is:
--- nsHttpChannel.cpp   2004-01-28 11:58:56.000000000 -0500
+++ nsHttpChannel.cpp   2004-01-28 21:38:01.000000000 -0500
@@ -273,17 +273,18 @@
 
     LOG(("nsHttpChannel::Connect [this=%x]\n", this));
 
+    PRBool offline = PR_FALSE;
+    // are we offline?
+    nsCOMPtr<nsIIOService> ioService;
+    rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
+    if (NS_FAILED(rv)) return rv;
+
+    ioService->GetOffline(&offline);
+
     // true when called from AsyncOpen
     if (firstTime) {
         PRBool delayed = PR_FALSE;
-        PRBool offline = PR_FALSE;
     
-        // are we offline?
-        nsCOMPtr<nsIIOService> ioService;
-        rv = gHttpHandler->GetIOService(getter_AddRefs(ioService));
-        if (NS_FAILED(rv)) return rv;
-
-        ioService->GetOffline(&offline);
         if (offline)
             mLoadFlags |= LOAD_ONLY_FROM_CACHE;
 
@@ -308,7 +309,7 @@
         // inspect the cache entry to determine whether or not we need to go
         // out to net to validate it.  this call sets mCachedContentIsValid
         // and may set request headers as required for cache validation.
-        rv = CheckCache();
+        rv = CheckCache(offline);
         NS_ASSERTION(NS_SUCCEEDED(rv), "cache check failed");
 
         // read straight from the cache if possible...
@@ -770,7 +771,7 @@
     if (NS_FAILED(rv)) return rv;
 
     // install cache listener if we still have a cache entry open
-    if (mCacheEntry)
+    if (mCacheEntry && (mCacheAccess & nsICache::ACCESS_WRITE))
         rv = InstallCacheListener();
 
     return rv;
@@ -1111,7 +1112,7 @@
 
     // Set the desired cache access mode accordingly...
     nsCacheAccessMode accessRequested;
-    if (offline)
+    if (offline || mLoadFlags & INHIBIT_CACHING)
         accessRequested = nsICache::ACCESS_READ; // have no way of writing to 
cache
     else if (mLoadFlags & LOAD_BYPASS_CACHE)
         accessRequested = nsICache::ACCESS_WRITE; // replace cache entry
@@ -1209,7 +1210,7 @@
 // mCachedContentIsValid flag, and to configure an If-Modified-Since request
 // if validation is required.
 nsresult
-nsHttpChannel::CheckCache()
+nsHttpChannel::CheckCache(PRBool offline)
 {
     nsresult rv = NS_OK;
 
@@ -1261,8 +1262,8 @@
     if (NS_FAILED(rv)) return rv;
     buf.Adopt(0);
 
-    // If we were only granted read access, then assume the entry is valid.
-    if (mCacheAccess == nsICache::ACCESS_READ) {
+    // If we are offline, then assume the entry is valid.
+    if (offline) {
         mCachedContentIsValid = PR_TRUE;
         return NS_OK;
     }
@@ -1525,7 +1526,9 @@
     nsresult rv;
 
     NS_ENSURE_TRUE(mCacheEntry, NS_ERROR_UNEXPECTED);
-    NS_ENSURE_TRUE(mCacheAccess & nsICache::ACCESS_WRITE, NS_ERROR_UNEXPECTED)
;
+    // if only reading, nothing to be done here.
+    if (mCacheAccess == nsICache::ACCESS_READ)
+        return NS_OK;
 
     // Don't cache the response again if already cached...
     if (mCachedContentIsValid)


--- netwerk/protocol/http/src/nsHttpChannel.h   2004-01-28 12:06:00.000000000 -0500
+++ netwerk/protocol/http/src/nsHttpChannel.h   2004-01-28 12:06:18.000000000 -0500
@@ -132,7 +132,7 @@
     nsresult OpenCacheEntry(PRBool offline, PRBool *delayed);
     nsresult GenerateCacheKey(nsACString &key);
     nsresult UpdateExpirationTime();
-    nsresult CheckCache();
+    nsresult CheckCache(PRBool offline);
     nsresult ReadFromCache();
     nsresult CloseCacheEntry(nsresult status);
     nsresult InitCacheEntry();

Comment 3

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

could you attach patches as attachments, rather than as comments? that does not
clutter up the bug report as much, and allows marking review flags on it.

Comment 4

[Darin Fisher]

ok, i don't understand the additions to the patch.  please explain.

i think the right solution might be to just prevent writing to the cache when
INHIBIT_CACHING is selected.  in other words we just need to check the load
flags before trying to write to the cache.  maybe this can be done in
InitCacheEntry.

if you are inclined to write a new patch, please use the "Create a New
Attachment" link located above.  thx!

Comment 5

[William Bardwell]

Attachment: Patch as an attachment

The additional changes are to disable writing when it doesn't
have write access to the cache entry...
Thus, we are avoiding installing the Tee by not calling InstallCacheListener
when we don't have write access, and we only assume that CachedContentIsValid
if we are actually offline, not just because we only have READ access.
And we stop asserting that we have WRITE access in InitCacheEntry, and
just return early when we only have READ access.

Comment 6

[Darin Fisher]

Attachment: v2 patch

This is a slight variation on the previous patch...

If we are going to hit the net to fetch or validate a page, then if we cannot
write to the cache entry, then we should just close the cache entry.  This
avoids any logic changes in the response processing code.

Comment 7

[Darin Fisher]

Comment on attachment 143026 [details] [diff] [review]
v2 patch

doug: this is the dependency patch i mentioned that is needed if we want to be
able to disable HTTP caching of images.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143026 [details] [diff] [review]
v2 patch

sr=bzbarky

Comment 9

[Darin Fisher]

fixed-on-trunk

Comment 10

[Boris Zbarsky [:bzbarsky]]

This caused bug 237332

Comment 11

[Darin Fisher]

I've requested drivers' approval to backout this patch.  After looking at it
some more, it's plainly obvious that the change in CheckCache to early return
only if offline is the problem.  We needed to early return if we have read-only
access since the rest of that function only pertains to cache validation.  I
think the INHIBIT_CACHING flag needs to be honored differently.  I'd like to try
again with a new patch for the 1.8 alpha cycle.

Comment 12

[Darin Fisher]

patch backed out for 1.7 beta.

Comment 13

[William Bardwell]

I am confused by the diagnosis that it needs to return early from
CheckCache if we only have read-only access.  That does not
match what I was expecting for INHIBIT_CACHING.  I am hoping
that INHIBIT_CACHING means "don't save the result of a request", and
not "only use the cache".

INHIBIT_CACHING requires read-only access, but it still needs to
decide if the cache entry is good, or if it needs to request the
data again (but not save the result, just use it now.)

I think that the early close of the cache entry from the revision
of my patch may be the problem.  (But there might be other problems
with my original patch...) (I do not see 237332 with 1.5 with my patch,
havn't tried it with 1.7 with that patch.)

Comment 14

[Darin Fisher]

yes, the early close of the cache entry is the problem.

my point was that the early return from CheckCache that used to be conditional
on mCacheAccess == ACCESS_READ can be reached for more reasons than just us
being offline or the INHIBIT_CACHING flag being set.  in fact, the cache may
choose to give us readonly access based on coalescing of requests.  those
subsequent, coalesced requests need the early return from CheckCache.  the
earlier patches that replaces that condition by checking for offline or
INHIBIT_CACHING were also wrong because they caused some readonly cases to go
through the validation logic.

so, i think this needs rethinking.  perhaps forcing readonly access in
OpenCacheEntry in the INHIBIT_CACHING case is wrong.  perhaps we need to do that
after CheckCache returns.

Comment 15

[Darin Fisher]

sorry, this is the snipet from the first patch that i was referring to:

-    // If we were only granted read access, then assume the entry is valid.
-    if (mCacheAccess == nsICache::ACCESS_READ) {
+    // If we are offline, then assume the entry is valid.
+    if (offline) {
         mCachedContentIsValid = PR_TRUE;
         return NS_OK;
     }

i incorrectly said that you were allowing early return if INHIBIT_CACHING or
offline.  i should have re-read the patch before posting my comment!  the point
i was trying to make still holds though.  it's not sufficient to assume that
offline being true is the only reason to enter that early return.

Comment 16

[William Bardwell]

Attachment: Here is a reduced version of my original patch that should be safer...

So, this should just make INHIBIT_CACHING work, but leaves
all other behavior the same...there may be a more aggressive
change that would be better, but this does make INHIBIT_CACHING
work, and shouldn't break other stuff.

Comment 17

[alanjstr]

Comment on attachment 149067 [details] [diff] [review]
Here is a reduced version of my original patch that should be safer...

Original patch had SR.	Please review this and see if it is ready to be landed.
 Thanks.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149067 [details] [diff] [review]
Here is a reduced version of my original patch that should be safer...

This really needs Darin's OK.

Comment 19

[Darin Fisher]

> So, this should just make INHIBIT_CACHING work, but leaves
> all other behavior the same...

This is a great idea, but your patch does not completely hold to "[leaving] all
other behaviors the same."  You make some changes that do not equate to no-ops
when INHIBIT_CACHING is not specified.  That said, I'm willing to take this
patch on the 1.8 trunk since we are still in the alpha development phase.

Comment 20

[Darin Fisher]

Comment on attachment 149067 [details] [diff] [review]
Here is a reduced version of my original patch that should be safer...

r=darin

this looks good to me.	i believe that when INHIBIT_CACHING is not specified,
nothing will in effect be changed :-)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 149067 [details] [diff] [review]
Here is a reduced version of my original patch that should be safer...

>+    if (offline || mLoadFlags & INHIBIT_CACHING)

I'd prefer the part after || in its own parentheses.

sr=bzbarsky with that change.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.