748510 - Cache 3xx response bodies so we can fail them the same way as uncached.

Status: NEW

(Core :: Networking: Cache, defect, P5)

Description

[Jason Duell]

Attachment: xpcshell test: expects failed cached redirect to render 3xx body (currently fails)

Michal tells me at some point Bjarne (?) landed some code that makes us no longer cache 3xx response bodies, just the headers.

This means that if we are unable to complete the cached redirect (we hit CORRUPT, or a redirect observer decides to veto the redirect), we have nothing to show the user. (At the moment the browser simply stays on the previous page).   This makes cached redirects behave differently than uncached ones (see attached test).  So we may want to consider caching the bodies of 3xx responses again.  

This only affects top-level loads, which won't be blocked by CORS checks (and the CORRUPT case, once we fix things so we don't store them in the case any more, will be rare).  So it's not high-priority AFAICT.

Thoughts?

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #0)
> This only affects top-level loads, which won't be blocked by CORS checks
> (and the CORRUPT case, once we fix things so we don't store them in the case
> any more, will be rare).  So it's not high-priority AFAICT.
> 
> Thoughts?

What happens when we run into such an error with a 200 or 3xx response that a zero-length response body? It seems like either that does the wrong thing, and we need to fix higher-level code to handle the zero-length-body case as well as the no-body-available case, or we could just change the higher-level code to handle a missing response body the same way it handles a zero-length response body.

> (and the CORRUPT case, once we fix things so we don't store them in the case
> any more, will be rare).

We will always have to check for CORRUPT entries returned from the cache, because we may have cached the response before we added the code to reject them. I.e. we need to check for CORRUPT when receiving a response from the server, and we need to check for CORRUPT when receiving a response from the cache, but we don't need to do anything special to prevent storing them in the cache, because the other two checks will handle that.

Comment 2

[Jason Duell]

> What happens when we run into such an error with a 200 or 3xx response that 
> a zero-length response body?

For CORRUPTED_CONTENT, we display an error.  For CORS and other redirect observer vetoes, we display nothing, so yes, we're in the same boat.   I'm not sure how serious the issue is in practice, but we don't have good UI right now.

> We will always have to check for CORRUPT entries returned from the cache

Well, at least until the next cache version bump, yes.  I'm generally opposed to storing garbage in the cache, though.  If nothing else it wastes our time waiting for I/O just so we can hit the net anyway (ex: bug 748108).

Personally, I'm inclined to shelve this bug for now, until/unless we find we're hitting actual cases on the net where the lack of UI hurts us.   OTOH I can see that this is a fairly subtle thing to track down, so users might underreport it.

Comment 3

[Jason Duell]

>  we display nothing

By which I mean "no navigation": we display whatever was in the tab before the navigation.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jason Duell (:jduell) from comment #2)
> > We will always have to check for CORRUPT entries returned from the cache
> 
> Well, at least until the next cache version bump, yes.  I'm generally
> opposed to storing garbage in the cache, though.  If nothing else it wastes
> our time waiting for I/O just so we can hit the net anyway (ex: bug 748108).

I agree. My point is that we will never get to the point where we store NEW garbage into the cache because we do the checks at the time we parse the response from the server.

(In reply to Jason Duell (:jduell) from comment #3)
> >  we display nothing
> 
> By which I mean "no navigation": we display whatever was in the tab before
> the navigation.

And the address bar does not change, right?

If the address bar does change to show the redirected-to URL then this is a security issue.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #4)
> If the address bar does change to show the redirected-to URL then this is a
> security issue.

If the address bar changes to show the redirected-*from* URL (or really, if it changes at all when we don't change the content displayed) then this is a security issue.

Comment 6

[Jason Duell]

The address bar doesn't change.

Comment 7

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258