94004 - Offline: redirects not cached

Status: RESOLVED FIXED

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

Description

[Alex Bishop]

See bug 48845 for the background.

User-agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.3) Gecko/20010801

Offline browsing (in that you can seamlessly visit pages in your cache by typing
their URLs and/or following links) works great in most circumstances but seems
to have problems with redirects.

Step to reproduce:
1. Go to http://uk.yahoo.com/.
2. Hover above the 'Sport' link (under 'Recreation & Sport'). Note that the URL
is http://uk.yahoo.com/r/ei.
3. Click the link. You go to http://uk.yahoo.com/r/ei but are redirected to
http://uk.dir.yahoo.com/Recreation/Sport/.
4. Disconnect from the Internet (if possible) and go into offline mode.
5. Go to http://uk.yahoo.com/ again. The page is pulled from the disk cache.
6. Click the 'Sport' link.

Expected Result:
The browser seamlessly handles the redirect and you end up at the cached version
of http://uk.dir.yahoo.com/Recreation/Sport/ as if it was a normal link.

Actual Result:
Nothing happens.

This works in IE 5.5.

Comment 1

[Bradley Baetz (:bbaetz)]

Redirects aren't cached.

-> darin, because I don't thikn there was ever a bug filed on this.

Side issue: darin, should we cache non-cachable redirects with a no-cache
header, so that offline works with these type of responses?

Comment 2

[Darin Fisher]

that sounds like a good suggestion to me.

Comment 3

[benc]

summary changed for searchability.

Comment 4

[Darin Fisher]

the following makes sense to me:

300, 301: infinite freshness lifetime, subject to added response headers
302, 307: zero freshness lifetime, subject to added response headers

"subject to added response headers" means that response headers such as
Cache-control, Pragma, or Expires could modify the freshness lifetime and hence
the expiration time calculation.

303 and 305 will not have entries in the cache.
IMO this seems like a design consistent with rfc2616.

Comment 5

[Darin Fisher]

Attachment: some very preliminary hacking

Comment 6

[Darin Fisher]

my patch crashed on me when i tried to access the cached redirect, so there is
definitely more work needed on it.

Comment 7

[Darin Fisher]

Attachment: v1.0 patch (w/ hack to get around bug 101857)

Comment 8

[Darin Fisher]

some comments about this patch:

1- necessary to asynchronously process a cached redirect... otherwise, the
uriloader/docshell get horribly confused.

2- 303 and 305 are never cached.  300, 301, 302, and 307 are cached.

3- 300, 301 default to infinite expiration time, unless the server provides some
cache control headers.

4- 302, 307 default to expiring immediately, unless the server provides some
cache control headers.

5- work around for bug 101857 is to open a cache output stream and then close
it... there is not much cost to doing this, so i think it should be fine to land
this patch, and then go back and remove the OpenOutputStream call once bug
101857 is fixed.

Comment 9

[Gagan]

Comment on attachment 51286 [details] [diff] [review]
v1.0 patch (w/ hack to get around bug 101857)

r=gagan

Comment 10

[rpotts (gone)]

looks good to me.  sr=[e-mail]

my only comment/suggestion is that it looks like you can safely remove the 
'rv=mStatus' assignment (when NS_FAILED(mStatus)) in the snippet below...

+nsHttpChannel::HandleAsyncRedirect()
+{
+    nsresult rv;
+
+    LOG(("nsHttpChannel::HandleAsyncRedirect [this=%p]\n", this));
+
+    if (NS_FAILED(mStatus)) {
+        LOG(("channel appears to have been canceled\n"));

+        rv = mStatus;   <== THIS LOOKS UNNECESSARY

+    }
+    else {
+        rv = ProcessRedirection(mResponseHead->Status());
+        if (NS_SUCCEEDED(rv))
+            rv = NS_BINDING_REDIRECTED;
+        mStatus = rv;
+    }
+

Comment 11

[Darin Fisher]

rick, you're right... that looks like leftovers from a previous attempt i was
working on.  i'll clean it up and check in.  thanks for the reviews.

Comment 12

[Darin Fisher]

fixed-on-trunk

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this caused bug 102936.