758973 - HTTP status 308 response results in nsIChannelEventSink::REDIRECT_TEMPORARY passed to nsIChannelEventSink instead of nsIChannelEventSink::REDIRECT_PERMANENT

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

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

+++ This bug was initially created as a clone of Bug #714302 +++

See ContinueProcessRedirectionAfterFallback:

    if (mRedirectType == 301) // Moved Permanently
        redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
    else
        redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;

It is a good idea to grep through all the code (especially in netwerk/ and in content/, where XHR lives) to see all the cases where 301 is handled specially.

I know that before we agreed that, as far as Necko is concerned, there should be no difference in the various kinds of redirects, but we should make sure 307/301 are reported to users of Necko correctly so they can make their own decisions as to what distinctions to make between them.

(Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various statuses that are to be considered redirects in some cases, and it omits status 308.)

Comment 1

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

(BTW, it is confusing that 301 is permanent and 302 is temporary, but 307 is temporary and 308 is permanent.)

Comment 2

[Julian Reschke]

(In reply to Brian Smith (:bsmith) from comment #0)
> +++ This bug was initially created as a clone of Bug #714302 +++
> 
> See ContinueProcessRedirectionAfterFallback:
> 
>     if (mRedirectType == 301) // Moved Permanently
>         redirectFlags = nsIChannelEventSink::REDIRECT_PERMANENT;
>     else
>         redirectFlags = nsIChannelEventSink::REDIRECT_TEMPORARY;
> 
> It is a good idea to grep through all the code (especially in netwerk/ and
> in content/, where XHR lives) to see all the cases where 301 is handled
> specially.
> 
> I know that before we agreed that, as far as Necko is concerned, there
> should be no difference in the various kinds of redirects, but we should
> make sure 307/301 are reported to users of Necko correctly so they can make
> their own decisions as to what distinctions to make between them.

Ack. Will check and provide a patch.

> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

Indeed. That's the code where XHR requires redirects to be followed, which is problematic in itself.

Optimally. XHR would (a) allow callers to disable following redirects and (b) actually state what to do with 3xx codes in general.

Comment 3

[Julian Reschke]

(In reply to Brian Smith (:bsmith) from comment #0)
> ...
> (Also, FYI, the XMLHttpRequest Level 2 specification enumerates the various
> statuses that are to be considered redirects in some cases, and it omits
> status 308.)

-> <https://www.w3.org/Bugs/Public/show_bug.cgi?id=17222>

Comment 4

[Julian Reschke]

Attachment: proposed patch, work-in-progress

Adds checks for 308 to 2 more places. Added a function for classification in HttpBaseChannel.cpp; but I'm not sure how to use it in nsHttpResponseHead.cpp; maybe it needs to be moved somewhere else. Also, optimally, we had test coverage for this; do we have test cases where the permanence bis is tested somehow?

Comment 5

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

Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

I recommend that you make IsPermanentRedirect a static member of nsHttpResponseHead so that you can use it in the ComputeFreshnessLife as well as in nsHttpChannel.

The patch looks good to me, but I am going to ask Christian to look at it too in the hope that he might be able to suggest a convenient way to test it.

Comment 6

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

Comment on attachment 627859 [details] [diff] [review]
proposed patch, work-in-progress

+    if ((mStatus == 300) || (mStatus == 301) || (mStatus == 308)) {

this should probably use the function too. make the function a static member of nsHttp? (in nsHttp.h/cpp)

Comment 7

[Julian Reschke]

Attachment: Proposed patch

Moved as suggested.

I wonder whether we should do the same for IsSafeMethod and ShouldRewriteRedirectToGET (for consistency)?

Comment 8

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

Comment on attachment 642214 [details] [diff] [review]
Proposed patch

Review of attachment 642214 [details] [diff] [review]:
-----------------------------------------------------------------

If you want to move the existing methods for consistency then please do so in a separate patch and r+ me ahead of time for it.

::: netwerk/protocol/http/nsHttp.cpp
@@ +290,5 @@
> +bool
> +nsHttp::IsPermanentRedirect(PRUint32 httpStatus)
> +{
> +  return httpStatus == 301 || httpStatus == 308;
> +}   

Trailing whitespace.

Comment 9

[Julian Reschke]

(In reply to Brian Smith (:bsmith) from comment #8)
> If you want to move the existing methods for consistency then please do so
> in a separate patch and r+ me ahead of time for it.

See bug 782235.

Comment 10

[Julian Reschke]

Attachment: Proposed patch

Comment 11

[Julian Reschke]

Brian, can I set this to "r+" myself (the only change was the removal of trailing whitespace)?

Comment 12

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

Comment on attachment 651325 [details] [diff] [review]
Proposed patch

Review of attachment 651325 [details] [diff] [review]:
-----------------------------------------------------------------

When somebody gives you r+ with a request to fix some trivial issue (naming, whitespace, etc.), you can automatically r+ the new version of the patch when you attach it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

I don't see any Try results here, so I pushed. I'll land it if it goes green.
https://tbpl.mozilla.org/?tree=Try&rev=07e44b46b028

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #13)
> https://tbpl.mozilla.org/?tree=Try&rev=07e44b46b028

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c2b82b79a2

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/c4c2b82b79a2