289374 - Wrong behaviour when combining INHIBIT_CACHING with LOAD_BYPASS_CACHE

Status: RESOLVED FIXED

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

Description

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

In nsHttpChannel::OpenCacheEntry:
1208     if (offline || (mLoadFlags & INHIBIT_CACHING))
1209         accessRequested = nsICache::ACCESS_READ; // have no way of writing
to cache
1210     else if (mLoadFlags & LOAD_BYPASS_CACHE)
1211         accessRequested = nsICache::ACCESS_WRITE; // replace cache entry
1212     else
1213         accessRequested = nsICache::ACCESS_READ_WRITE; // normal browsing

So, if someone wants to bypass the cache for the load, and not affect any cache
entries, that won't work - INHIBIT_CACHING will make it request read access. It
seems like this would make CheckCache set If-Modified-Since headers, and maybe
just use the cache entry without validation in some cases.

Comment 1

[Darin Fisher]

yeah, good catch.  putting on my radar for 1.8, but please help out with a patch
if you have time.

Comment 2

[Darin Fisher]

Attachment: v1 patch

Comment 3

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

Comment on attachment 180013 [details] [diff] [review]
v1 patch

+	 if (mLoadFlags & LOAD_BYPASS_CACHE && !offline)

would it be clearer to parenthesize the (mLoadFlags & LOAD_BYPASS_CACHE),
similar to the if above (although it's || there)


I've been wondering... would it be better to make shift+reload while offline
fail the load? since we clearly can't actually reload the file from the
network...

Comment 4

[Darin Fisher]

> would it be clearer to parenthesize the (mLoadFlags & LOAD_BYPASS_CACHE),
> similar to the if above (although it's || there)

yes, I will make that change.


> I've been wondering... would it be better to make shift+reload while offline
> fail the load? since we clearly can't actually reload the file from the
> network...

hrm... I'm not sure.  I've okay with the current behavior that while in offline
mode we should never discard cache entries.  normal shirt-reload discards cache
entries, so unless we were to workaround that somehow, i'd prefer that we do not
implement shift-reload this way.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180013 [details] [diff] [review]
v1 patch

So what this does is just throw if someone tries to open a channel that is both
BYPASS_CACHE and INHIBIT_CACHING?

Comment 6

[Darin Fisher]

no, not at all.  it causes the caller of OpenCacheEntry (which is an internal,
private method on nsHttpChannel) to ignore the cache altogether.  remember, HTTP
is designed to function without a cache.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180013 [details] [diff] [review]
v1 patch

Ah, gotcha.  Looks good, then.

Comment 8

[Darin Fisher]

fixed-on-trunk