133015 - Info may indicate "not cached" although the page actually is in cache

Status: RESOLVED FIXED

(SeaMonkey :: Page Info, defect, P3)

Description

[Uwe Schröder]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win95; en-US; rv:0.9.9) Gecko/20020311
BuildID:    2002031104

Pages marked stale for "vary:" header handling are displayed as "not cached",
although they are actually in cache, set to force-revalidate, not to force-reload.

Reproducible: Always
Steps to Reproduce:
1. Go to the URL given above
2. Examine Page Info
3. Compare with about:cache-entry information

Actual Results:  Page Info displays "not cached", "no expiration set".

Expected Results:  Page Info should display memory or disk cache as the actual
source, and "must-revalidate" or something like that as expiration info.

This might be related to Bug 132957.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I think this happens because we try to open the cache entry for reading and the
cache revalidation fails or something....

We should look into implementing nsICacheVisitor and using that (like
about:cache does).  Not sure what the perf impact would be...

Comment 2

[Daniel Brooks [:db48x]]

bz's assessment is correct.

cache visitors suck, as I recall you have to visit each entry at least once to
figure out if what you're looking for is in there. how long does it take to
display about:cache? now do that each time you preview an image in the media
tab. sounds bad to me.

Comment 3

[Boris Zbarsky [:bzbarsky]]

gordon?  any ideas?  Is it possible to open a cache entry without triggering
revalidation using a scriptable API?

Comment 4

[Uwe Schröder]

I guess that's what
"about:cache-entry?client=HTTP&sb=1&key=http://server.foo/something" does - just
collect information about the current state of a cache entry, without actually
retrieving it.

Comment 5

[Boris Zbarsky [:bzbarsky]]

No, as a matter of fact that does exactly what page info does -- creates a cache
session and opens the cache entry (in exactly the same way as far as I can
tell).  See
http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/about/src/nsAboutCacheEntry.cpp#335

The only difference I see is the call to:

mCacheSession->SetDoomEntriesIfExpired(PR_FALSE);

reading over nsICacheSession.idl, this may be exactly what needs to be done
here.

Comment 6

[Antonio.Xu]

this bug is due to,mozilla cache page info in disk cache,but when  mozilla 
return entry,the page's expires time is less than current time,then mozilla 
will clean the cache entry and don't retrun it.i will research it,i think i can 
avoid time's estimation.mozilla really cache the page info in disk cache.
please assigned this bug to me,i'm sure i can fix it.

Comment 7

[Antonio.Xu]

Attachment: the patch can fix this bug,but it is a far-fetched patch.please rv my patch.

i found this bug is due to the cache service will clean the entry being
returned from disk cache after entry be returned by SearchCacheDevices(),i
create a patch which can fix this bug,but maybe have some problem,because this
method is runned by a lot class,so my fix maybe arise another problem,I hope
somebody can give me some advice.Please rv my patch!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Better patch, per comment #4

Antonio, your patch has one major drawback -- it breaks correct handling of
cache entry expiration completely.  With your patch, things would never expire
from cache and the browser would not go out to the network to refetch things
when it should.

Note the |&& request->WillDoomEntriesIfExpired()| clause in the if statement
you were changing... just setting doomEntriesIfExpired to "false" has the same
effect as your patch in this case.

db48x, would you review?

Comment 9

[Antonio.Xu]

yes,i think your patch right.

Comment 10

[Daniel Brooks [:db48x]]

Comment on attachment 79254 [details] [diff] [review]
Better patch, per comment #4

sure, I can r= that. it's identicle to the patch I haven't posted yet :)

r=db48x

Comment 11

[Darin Fisher]

Comment on attachment 79254 [details] [diff] [review]
Better patch, per comment #4

sr=darin

btw: HTTP also uses a session named "HTTP-memory-only"
this is used for HTTPS content and no-store content.  also, these session names
were never meant to be referenced outside the HTTP implementation :-(

Comment 12

[Boris Zbarsky [:bzbarsky]]

checked in on the trunk.